zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From hanm <...@git.apache.org>
Subject [GitHub] zookeeper pull request #99: ZOOKEEPER-2549 Add exception handling to sendRes...
Date Sat, 03 Dec 2016 19:32:42 GMT
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message