ZOOKEEPER-4309 QuorumCnxManager's ListenerHandler thread leak #1705
Conversation
|
Writing a test for this means both mocking or adding more state into |
|
hi guy, |
|
in order to explain what's the issue I've created a test application to show it: https://issues.apache.org/jira/browse/ZOOKEEPER-4309?focusedCommentId=17354802&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17354802 |
There was a problem hiding this comment.
Overall LGTM
I left a suggestion, if you want to take it into consideration please @franz1981
otherwise I am happy with merging the patch as it is
| @@ -957,8 +957,13 @@ public void run() { | |||
| new ListenerHandler(address, self.shouldUsePortUnification(), self.isSslQuorum(), latch)) | |||
| .collect(Collectors.toList()); | |||
|
|
|||
| ExecutorService executor = Executors.newFixedThreadPool(addresses.size()); | |||
| listenerHandlers.forEach(executor::submit); | |||
| final ExecutorService executor = Executors.newFixedThreadPool(addresses.size()); | |||
There was a problem hiding this comment.
what about creating a shared ExecutorService, as a field in this class ?
this way it will be clearer the life cycle of this pool.
also, probably it would be worth to not use a pool in case of a list of addresses of size 1, but this may be a follow up work
There was a problem hiding this comment.
what about creating a shared ExecutorService, as a field in this class ?
I was thinking the same, but then I've found https://github.com/franz1981/zookeeper/blob/b4f9aab099880ba8ef08eaff697debe6cdeae057/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Leader.java#L456-L468 and see that other parts in the ZK code just do it right without any hook to check for Thread "liveness".
I believe having a shared pluggable executor service could worth for embedded use cases, where people would like to keep control on thread creation, instead.
There was a problem hiding this comment.
what about creating a shared ExecutorService, as a field in this class ?
this way it will be clearer the life cycle of this pool.also, probably it would be worth to not use a pool in case of a list of addresses of size 1, but this may be a follow up work
we can use a global shared thread pool just as what we do in spring. For example we use @component annotation to register a pool.
however there's a question we need figure it out if the pool is io computed pool or a cpu computed pool.
I think that is io computed pool because we use to accept ticket there.
@eolivelli
There was a problem hiding this comment.
let's stick with this patch.
we can improve things later
|
@eolivelli Any news on this? |
I'm a OSS lover (beside my actual job), so would be happy to help on this :) |
|
Planning to look into this on Wednesday (unless somebody beats me to it, of course). |
|
@hanm @ztzg @eolivelli happy that the fix is sound: as said probably worth considering some centralized way to create thread(s)/thread pools for a follow-up work on this |
https://issues.apache.org/jira/browse/ZOOKEEPER-4309 Author: franz1981 <nigro.fra@gmail.com> Reviewers: Enrico Olivelli <eolivelli@apache.org>, Michael Han <hanm@apache.org>, Damien Diederen <ddiederen@apache.org> Closes #1705 from franz1981/ZOOKEEPER-4309 (cherry picked from commit 7855045) Signed-off-by: Enrico Olivelli <eolivelli@apache.org>
|
committed to master branch and to branch-3.7. thank you ! |
https://issues.apache.org/jira/browse/ZOOKEEPER-4309 Author: franz1981 <nigro.fra@gmail.com> Reviewers: Enrico Olivelli <eolivelli@apache.org>, Michael Han <hanm@apache.org>, Damien Diederen <ddiederen@apache.org> Closes apache#1705 from franz1981/ZOOKEEPER-4309
https://issues.apache.org/jira/browse/ZOOKEEPER-4309 Author: franz1981 <nigro.fra@gmail.com> Reviewers: Enrico Olivelli <eolivelli@apache.org>, Michael Han <hanm@apache.org>, Damien Diederen <ddiederen@apache.org> Closes apache#1705 from franz1981/ZOOKEEPER-4309 Co-authored-by: franz1981 <nigro.fra@gmail.com>
https://issues.apache.org/jira/browse/ZOOKEEPER-4309