I'm not sure why you say that it is better to swallow the exception, Ben. I checked the methods that call sendResponse and they seem to be able to handle IOExceptions fine. For example, in NettyServerCnxn.process, we call close upon IOException, which is exactly the behavior you mention you should have.
I'm thinking that in this case, if the channel is closed and it is null, we throw IOException. I'm trying to understand why that's bad course of action. -Flavio > On 01 Sep 2016, at 19:29, Benjamin Reed <br...@apache.org> wrote: > > i agree, the exception should not bubble up. if something bad happens we > should mark the connection as closed (if not already) and continue on. > elsewhere closed connections are cleaned up. (or at least they better be...) > > ben > > On Thu, Sep 1, 2016 at 11:02 AM, yuliya Feldman <yufeld...@yahoo.com.invalid >> wrote: > >> Thank you Ben and Patrick for the replies. >> The problem I see with Netty exception handling (or rather not handling) >> is that if something happens it bubbles up and main request processing >> thread is stopped which effectively halts whole ZK server operations. >> I will submit a JIRA on this (hopefully today). Either we should not >> bubble up any exception by IOException or ZK server should be stopped, as >> it is really hard to figure out without turning on tracing what really >> happened. >> ThanksYuliya >> >> From: Benjamin Reed <br...@apache.org> >> To: Patrick Hunt <ph...@apache.org> >> Cc: DevZooKeeper <dev@zookeeper.apache.org>; yuliya Feldman < >> yufeld...@yahoo.com>; "u...@zookeeper.apache.org" < >> u...@zookeeper.apache.org> >> Sent: Wednesday, August 31, 2016 10:47 PM >> Subject: Re: Issue with NettyServerCnxn.java >> >> if i remember correctly the case in sendResponse where it is catching the >> IOException is due to the fact that we are opportunistically trying to send >> something on a non-blocking channel. if it works, ok, but if we can't send >> because we are blocked then we will just send later. >> >> in the case of NIOServerCnxn there really shouldn't be any exceptions in >> sendResponse since it's just queuing. i think the catch is probably there >> so that the exception does not get propagated up and kill everything. >> >> ben >> >> On Wed, Aug 31, 2016 at 9:52 PM, Patrick Hunt <ph...@apache.org> wrote: >> >>> Hi Yuliya - my read is that sendResponse in NIOServerCnxn is logging, >>> then dropping, any Exceptions encountered during sendResponse. In other >>> words it's doing best effort response. Not sure if that is "correct", but >>> that's what it's currently doing in NIO. Surprisingly it's also hiding >> any >>> IOExceptions, which is part of the method signature as defined by >>> ServerCnxn. Some of the calling code is trying to handle IOException in >>> some cases which is odd... I suspect it was an oversight in >> ZOOKEEPER-597, >>> but I'm not sure. >>> >>> Ben any insight? >>> >>> Patrick >>> >>> On Tue, Aug 30, 2016 at 5:15 PM, yuliya Feldman < >>> yufeld...@yahoo.com.invalid> wrote: >>> >>>> Hello there, >>>> We have been extensively testing Netty connection versus NIIO and there >>>> are some issues that show up I wanted to get community response on. >>>> In the process of testing https://issues.apache. >>>> org/jira/browse/ZOOKEEPER-2509 fix we identified that sendResponse() >>>> method may try to do some operations after close() was invoked - as >>>> channel.close() in Netty is asynch. and subsequently lead to some NPE. >>>> NPE itself is not a good thing but the problems aggravates with the fact >>>> that propagation of NPE will lead to main processing thread exiting and >> at >>>> that point ZK server becomes unresponsive - since no requests will be >>>> processed anymore. >>>> In NIOServerCnxn.java in sendResponse() it is catching Exception and >> just >>>> logs a warning which was added as part of >> https://issues.apache.org/jira >>>> /browse/ZOOKEEPER-597 >>>> I am trying to understand what a behavior should be in case of any >>>> exception in sendResponse. >>>> Any insight would be highly appreciated >>>> Thanks,Yuliya >>>> >>>> >>> >> >> >> >>
