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.
