hanm commented on a change in pull request #1235: ZOOKEEPER-3706:
ZooKeeper.close() would leak SendThread when the netw…
URL: https://github.com/apache/zookeeper/pull/1235#discussion_r375060653
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java
##########
@@ -1121,6 +1125,9 @@ private void startConnect(InetSocketAddress addr) throws
IOException {
LOG.warn("Unexpected exception", e);
}
}
+ if (!state.isAlive()) {
Review comment:
I think this will work.
I would recommend instead of doing the check here, we add a set of new
`synchronized` methods to `SenderThread` where we get and set the states of the
`SenderThread.state`. Then, we can move the checks in the `setState` method,
where we can put constraints on how the connection state may transit. For
example, we can enforce in that method that a `CLOSED` state can't be transit
to a different state, which will prevent the problem we are seeing in this
issue (that setting a state to `CONNECTING` after being in `CLOSED` state).
This has additional benefits as currently, there are multiple write paths to
the `SenderThread.state`, and by consolidate read and write access of the state
in these synchronized methods, we can be sure there will not be future race
conditions w.r.t accessing this state variable.
I also thought about an alternative - instead of synchronizing on
`SenderThread`, we could wrap the state variable in an `AtomicReference` and
then provide get/set/compareAndSet methods on top of the atomic reference. This
however is less flexible to allow us to control how the state could transit, so
it still looks to me that synchronizing on `SenderThread` is better.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
