[ https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15718612#comment-15718612 ]
ASF GitHub Bot commented on ZOOKEEPER-2549: ------------------------------------------- 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. > As NettyServerCnxn.sendResponse() allows all the exception to bubble up it > can stop main ZK requests processing thread > ---------------------------------------------------------------------------------------------------------------------- > > Key: ZOOKEEPER-2549 > URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549 > Project: ZooKeeper > Issue Type: Bug > Components: server > Affects Versions: 3.5.1 > Reporter: Yuliya Feldman > Assignee: Yuliya Feldman > Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, > ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549-4.patch, ZOOKEEPER-2549.patch, > ZOOKEEPER-2549.patch, zookeeper-2549-1.patch > > > As NettyServerCnxn.sendResponse() allows all the exception to bubble up it > can stop main ZK requests processing thread and make Zookeeper server look > like it is hanging, while it just can not process any request anymore. > Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , > convert them to IOException and allow it propagating up -- This message was sent by Atlassian JIRA (v6.3.4#6332)