i agree. propagating up is the best. i was looking at NIOServerCnxn rather than checking above. behavior will change slightly since failing on a PING will cause another attempt at sending a response. i'm not sure how much that will change thing, but it is a code path that we have never hit with NIOServerCnxn.
ben On Thu, Sep 1, 2016 at 1:17 PM, yuliya Feldman <yufeld...@yahoo.com.invalid> wrote: > Yes, > This is what I plan - propagate IOException up. We could convert other > exceptions to IOException as well and propagate up. > > Thank you guys for replies.Yuliya > > From: Michael Han <h...@cloudera.com> > To: UserZooKeeper <u...@zookeeper.apache.org> > Cc: dev@zookeeper.apache.org; yuliya Feldman <yufeld...@yahoo.com>; > Patrick Hunt <ph...@apache.org> > Sent: Thursday, September 1, 2016 12:17 PM > Subject: Re: Issue with NettyServerCnxn.java > > Make sense to me. > > On Thu, Sep 1, 2016 at 12:10 PM, Flavio Junqueira <f...@apache.org> wrote: > > > I guess that's precisely what I'm proposing we avoid, I think we should > > propagate up as an IOException, which the signature of the abstract > method > > already suggests we should be doing. If what I'm saying makes any sense, > we > > should instead remove the catch Exception block at the end of > > NIOServerCnx.sendResponse. > > > > -Flavio > > > > > On 01 Sep 2016, at 20:05, Michael Han <h...@cloudera.com> wrote: > > > > > > I think it is not just about IOException - the current > > > NIOServerCnxn.sendResponse swallows any exception it caught (including > > the > > > NPE this thread the related JIRA is talking about.). On the other hand, > > the > > > NettyServerCnx.sendResponse only catches IOException, so there is a > > > discrepancy in terms of the behaviors of catching exception. Probably > > > making NettyServerCnx.sendResponse catches every exception is the > > solution > > > here? > > > > > > On Thu, Sep 1, 2016 at 11:57 AM, Flavio Junqueira <f...@apache.org> > > wrote: > > > > > >> 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 > > >>>>>> > > >>>>>> > > >>>>> > > >>>> > > >>>> > > >>>> > > >>>> > > >> > > >> > > > > > > > > > -- > > > Cheers > > > Michael. > > > > > > > -- > Cheers > Michael. > > > >
