Default stage configurations should not create daemon threads
Basics
Logistics
Basics
Logistics
Description
The PipelineDemultiplexerStage, PipelineMergeStage and SplitMergeStage create a default ExecutorService using Executors.newSingleThreadExecutor(). This in turn creates a default thread pool containing a single non-daemon thread.
The executorService field is not cleaned up in any way on bean destruction, and there’s no guarantee that garbage collection will ever solve this problem through finalisation (itself a deprecated feature), these non-daemon threads will survive the shutdown of the bean context. As well as being a leak of a somewhat limited resource, this can cause normal exit of the application to be infinitely deferred, as this normally waits until only daemon threads remain.
The temptation is to shut down the ExecutionService in the stage’s bean destroy code, but it’s not at all clear to me that is the right answer in principle (it would make sense for the creator of the resource to be responsible for its lifecycle) or in practice (if the stage has been configured with an explicit ExecutionService, it will no longer have a reference to the original one). We could invent (and document) some complicated rules for this, but it seems like an epicycle to me.
A more plausible option would be to create an executor which doesn’t create threads at all: after all, the idea by default is just to execute the jobs sequentially, then “wait” for their completion.
If the stages were defined in terms of Executor instead of ExecutorService, this would be a five-liner utility class (there’s an example of this code in the Executor Javadoc).
We should investigate whether we can make do with Executor and then convert if that’s the case. Note that ExecutorService is a subtype of Executor so this wouldn’t affect existing configurations. However, we’d need to provide our own equivalent of the submit method (returning a Future) to make this work; I suspect that’s fairly simple to do.
Alternatively, implementing a direct ExecutorService as opposed to a direct Executor is probably not that much more difficult; I am a little surprised I haven’t been able to locate one already.
The PipelineDemultiplexerStage, PipelineMergeStage and SplitMergeStage create a default ExecutorService using Executors.newSingleThreadExecutor(). This in turn creates a default thread pool containing a single non-daemon thread.
The executorService field is not cleaned up in any way on bean destruction, and there’s no guarantee that garbage collection will ever solve this problem through finalisation (itself a deprecated feature), these non-daemon threads will survive the shutdown of the bean context. As well as being a leak of a somewhat limited resource, this can cause normal exit of the application to be infinitely deferred, as this normally waits until only daemon threads remain.
The temptation is to shut down the ExecutionService in the stage’s bean destroy code, but it’s not at all clear to me that is the right answer in principle (it would make sense for the creator of the resource to be responsible for its lifecycle) or in practice (if the stage has been configured with an explicit ExecutionService, it will no longer have a reference to the original one). We could invent (and document) some complicated rules for this, but it seems like an epicycle to me.
A more plausible option would be to create an executor which doesn’t create threads at all: after all, the idea by default is just to execute the jobs sequentially, then “wait” for their completion.
If the stages were defined in terms of Executor instead of ExecutorService, this would be a five-liner utility class (there’s an example of this code in the Executor Javadoc).
We should investigate whether we can make do with Executor and then convert if that’s the case. Note that ExecutorService is a subtype of Executor so this wouldn’t affect existing configurations. However, we’d need to provide our own equivalent of the submit method (returning a Future) to make this work; I suspect that’s fairly simple to do.
Alternatively, implementing a direct ExecutorService as opposed to a direct Executor is probably not that much more difficult; I am a little surprised I haven’t been able to locate one already.