Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/99#discussion_r90763391
--- Diff: src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java ---
@@ -716,7 +716,12 @@ public void process(WatchedEvent event) {
// Convert WatchedEvent to a type that can be sent over the wire
WatcherEvent e = event.getWrapper();
- sendResponse(h, e, "notification");
+ try {
+ sendResponse(h, e, "notification");
+ } catch (IOException ex) {
+ LOG.debug("Problem sending to " + getRemoteSocketAddress(),
ex);
+ close();
--- End diff --
>> It was not closing (I think) before as exception was swallowed since
sendResponse in NIOServerCnxn was not throwing IOException
Yes, I think the connection was not closing before in cases of exception
thrown from `NIOServerCnxn.sendResponse` which swallows everything. The change
in this PR changes the behavior by closing the connection in case of exceptions
occur in sendResponse. I am leaning towards the old behavior of NOT closing the
connection, because the connection looks pretty innocent - in fact
`NIOServerCnxn.sendResponse` does not involve any socket IO I believe, it just
queuing stuff to be send over sockets. So if something goes wrong, we just do
our best effort by logging what's wrong - rather than trying mess up with
sockets which seems out of responsibilities of `NIOServerCnxn.sendResponse`.
Similarly since `NIOServerCnxn.sendResponse` does not directly involve sockets,
there should not be any leaks in case sendResponse screw up.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
