apache
[CURATOR-557] ServiceCacheImpl does not close ExecutorService
ServiceCacheImpl does not close the ExecutorService instance created from the ThreadFactory:
CloseableExecutorService::CloseableExecutorService(ExecutorService) call this(executorService, false) which sets shutdownOnClose to false.
this has an impact from callers, eg ServiceProviderImpl, which only allow only a ThreadFactory to be specified.
Originally reported by rnaude, imported from: ServiceCacheImpl does not close ExecutorService
- status: Open
- priority: Major
- resolution: Unresolved
- imported: 2025-01-21
It's closed by the PathChildrenCache instance - https://github.com/apache/curator/blob/master/curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/PathChildrenCache.java#L380 - did you mean something else?
maybe i missed something here:
ServiceCacheImpl(ServiceDiscoveryImpl
PathChildrenCache receives the CloseableExecutorService with shutdownOnClose = false, thus not closing the executor?
OK - I'll have a look at that when I get a chance (or another committer can).
have attached a patch:
- ServiceCacheImpl should tell CloseableExecutorService to close the executor when creating it from ThreadFactory
- CloseableExecutorService now await the shutdown of the executor
- expose setting an ExecutorService on ServiceCacheImpl via ServiceProviderBuilder / ServiceProviderImpl
- Makes sense, since the CloseableExecutorService that is created there is the exclusive owner of the new ExecutorService.
- Might be a good idea, but maybe should allow users to pick their own wait on close time
- If it can be useful for you, why not
with regards to the wait on close time. can you propose a solution? it is hidden away from normal use and not easily exposed. not sure if curator provides a global / singleton configuration interface? if so, then it can be added there.
We could introduce new method CloseableExecutorService.awaitTermination or similar that passes through the time to the ExecutorService. So we get similar pattern of usage to the ExecutorService.shutdownNow and awaitTermination. That's just an idea.
We could also pass the termination time through the constructor of CloseableExecutorService. But I like it less.
Thinking about it again, it won't be easy to use the CloseableExecutorService.awaitTermination, because it is an internal component of the service provider.
CloseableExecutorService is wrapped by PathChildrenCache which is wrapped by ServiceCacheImpl which is wrapped by ServiceProviderImpl. We would need to add each one of them awaitTermination.
I suggest to leave the CloseableExecutorService.close() method as it is (no call to ExecutorService.awaitTermination from there), and in case that the users want to do ThreadExecutor.awaitTermination then they should create their own ExecutorService and build the ServiceProviderImpl upon it (using your new API in ServiceProviderBuilder). Thus, after calling close() on the service provider the users still have control and can call ExecutorService.shutdownNow and awaitTermination however they want.
In my opinion, the users should use the ThreadFactory only if they don't need to await the termination of threads. If they do need, they still can get back the control as mentioned.
We still need 1 and 3.
Sounds good Shay Shimony. We can even deprecate the troublesome version (threadFactory(ThreadFactory threadFactory)).
OK Jordan Zimmerman.
Roelof Naude, does it make sense to you? Would you open a pull request with the fix or should I?
Shay Shimony sure. can provide a patch.
in short:
- deprecate use of ThreadFactory
- fix close issue
- ExecutorService addition to ServiceProvider?
Roelof Naude sounds right, thanks, but don't need new patch. I will create pull request based on your patch later today.
