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_r375061974
##########
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()) {
+ throw new RuntimeException("Already closed");
+ }
Review comment:
`RuntimeException` indicates unrecoverable programming error - such as
illegal arguments or unrecoverable errors. Our case here is not that dramatic,
it's recoverable by throwing an IOException if the state is `CLOSED` and it's
being set to another value. `CLOSED` state will signal `SenderThread` to
terminate and the benefit is we will be able to clean up resources - like the
sockets sender thread held, and more importantly, the event thread will also be
properly shutdown.
And since we are explicitly programming in this issue to prevent this error,
it does not sound like a logic error to me that worth to throw a
RuntimeException.
----------------------------------------------------------------
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
