ThreadPool and ThreadContext are not closeable #43249
Conversation
This commit changes the ThreadContext to just use a regular ThreadLocal over the lucene CloseableThreadLocal. The CloseableThreadLocal solves issues with ThreadLocals that are no longer needed during runtime but in the case of the ThreadContext, we need it for the runtime of the node and it is typically not closed until the node closes, so we miss out on the benefits that this class provides. Additionally by removing the close logic, we simplify code in other places that deal with exceptions and tracking to see if it happens when the node is closing. Closes elastic#42577
jaymode
added
>non-issue
:Core/Infra/Core
|
Pinging @elastic/es-core-infra |
| ensureOpen(); | ||
| throw ex; | ||
| } | ||
| } |
There was a problem hiding this comment.
nice to get rid of this hack
|
@jpountz I've updated this PR to address concerns regarding the creation of short lived thread locals in non test code |
| @@ -146,8 +146,6 @@ protected SSLService getSslService() { | |||
| @Before | |||
| public void cleanup() throws IOException { | |||
| if (threadContext != null) { | |||
| threadContext.stashContext(); | |||
| threadContext.close(); | |||
| threadContext = null; | |||
| } | |||
There was a problem hiding this comment.
we could remove the if statement?
There was a problem hiding this comment.
Good catch. I removed it and made it an after method, which makes more sense to me than a Before method.
| @@ -140,19 +124,13 @@ public StoredContext stashContext() { | |||
| DEFAULT_CONTEXT.putHeaders(Map.of(Task.X_OPAQUE_ID, context.requestHeaders.get(Task.X_OPAQUE_ID))); | |||
| threadLocal.set(threadContextStruct); | |||
| } else { | |||
| threadLocal.set(null); | |||
| threadLocal.set(DEFAULT_CONTEXT); | |||
There was a problem hiding this comment.
Your change sets the default context here, while master sets it to null and then checks for null at read time to return the default context instead. These approaches look equivalent to me and I think your change makes things simpler but I wonder whether I'm missing something?
There was a problem hiding this comment.
Prior to this change, it does make a difference in that the CloseableThreadLocal class does more work including executing a section of code that is synchronized across all threads when returning the initial value for the thread local unless the initial value is null; if the initial value is null, nothing gets stored and the synchronized code is skipped. However, once a value has been set the move to setting it null no longer bypasses the synchronized code. The regular ThreadLocal class always executes code when first accessing with an initial value but the code that is executed is not synchronized across all threads so we can make things simpler and just reference the default context instead of a null value.
This change handles the rejection of task submission in NodeTests#testCloseRaceWithTaskExecution. This test intentionally tries to cause an unexpected failure during shutdown of a node but does not handle the expected exception when the executor is shutdown before the runnable is submitted. Relates elastic#43249
This commit changes the ThreadContext to just use a regular ThreadLocal over the lucene CloseableThreadLocal. The CloseableThreadLocal solves issues with ThreadLocals that are no longer needed during runtime but in the case of the ThreadContext, we need it for the runtime of the node and it is typically not closed until the node closes, so we miss out on the benefits that this class provides. Additionally by removing the close logic, we simplify code in other places that deal with exceptions and tracking to see if it happens when the node is closing. Closes #42577
This change handles the rejection of task submission in NodeTests#testCloseRaceWithTaskExecution. This test intentionally tries to cause an unexpected failure during shutdown of a node but does not handle the expected exception when the executor is shutdown before the runnable is submitted. Relates #43249
This commit changes the ThreadContext to just use a regular ThreadLocal
over the lucene CloseableThreadLocal. The CloseableThreadLocal solves
issues with ThreadLocals that are no longer needed during runtime but
in the case of the ThreadContext, we need it for the runtime of the
node and it is typically not closed until the node closes, so we miss
out on the benefits that this class provides.
Additionally by removing the close logic, we simplify code in other
places that deal with exceptions and tracking to see if it happens when
the node is closing.
Closes #42577