Do not close threadpool if termination fails #42774
Conversation
This commit changes the code so that the threadpool is not closed unless termination succeeds. Otherwise there can still be running tasks that rely on resources that are closed by closing the threadpool. Additionally, there is a test fix included for the NodeTests that ensures the submitted task is actually running prior to closing the node in the test. Closes elastic#42577
jaymode
added
>enhancement
:Core/Infra/Core
|
Pinging @elastic/es-core-infra |
|
I'm a bit on the fence about having closeable objects that get never closed in some cases. I was reviewing boolean whileRunning = false;
threadsOriginalContext = stashContext();
try {
creatorsContext.restore();
whileRunning = true;
in.doRun();
whileRunning = false;
} catch (IllegalStateException ex) {
if (whileRunning || threadLocal.closed.get() == false) {
throw ex;
}
// if we hit an ISE here we have been shutting down
// this comes from the threadcontext and barfs if
// our threadpool has been shutting down
}My interpretation of this code block is that we want to ignore exceptions that signal that the thread context is already closed unless the exception is thrown by the wrapped runnable. Should we move the call to |
I think that is the correct interpretation. Moving stashContext makes sense but there is still an issue especially when security is in place and that is due to the fact that running tasks could still be using the thread context and that means we still have this issue. I am not very happy with this fix to be honest either. One way to handle this could be ref counting but that introduces complexity for not much gain IMO |
There are a few tests within NodeTests that submit items to the threadpool and then close the node. The tests are designed to check how running tasks are affected during node close. These tests can cause CI failures since the submitted tasks may not be running when the node is closed and then execute after the thread context is closed, which triggers an unexpected exception. This change ensures the threads are running so we avoid the unexpected exception and can test these cases. The test of task submittal while a node is closing is also important so an additional but muted test has been added that tests the case where a task may be getting submitted while the node is closing and ensuring we do not trigger anything unexpected in these cases. Relates elastic#42774 Relates elastic#42577
There are a few tests within NodeTests that submit items to the threadpool and then close the node. The tests are designed to check how running tasks are affected during node close. These tests can cause CI failures since the submitted tasks may not be running when the node is closed and then execute after the thread context is closed, which triggers an unexpected exception. This change ensures the threads are running so we avoid the unexpected exception and can test these cases. The test of task submittal while a node is closing is also important so an additional but muted test has been added that tests the case where a task may be getting submitted while the node is closing and ensuring we do not trigger anything unexpected in these cases. Relates #42774 Relates #42577
There are a few tests within NodeTests that submit items to the threadpool and then close the node. The tests are designed to check how running tasks are affected during node close. These tests can cause CI failures since the submitted tasks may not be running when the node is closed and then execute after the thread context is closed, which triggers an unexpected exception. This change ensures the threads are running so we avoid the unexpected exception and can test these cases. The test of task submittal while a node is closing is also important so an additional but muted test has been added that tests the case where a task may be getting submitted while the node is closing and ensuring we do not trigger anything unexpected in these cases. Relates #42774 Relates #42577
This commit changes the code so that the threadpool is not closed
unless termination succeeds. Otherwise there can still be running tasks
that rely on resources that are closed by closing the threadpool.
Additionally, there is a test fix included for the NodeTests that
ensures the submitted task is actually running prior to closing the
node in the test.
Closes #42577