You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by Patrick Hunt <ph...@apache.org> on 2016/09/01 04:52:23 UTC
Re: Issue with NettyServerCnxn.java
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 <yufeldman@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
>
>
Re: Issue with NettyServerCnxn.java
Posted by yuliya Feldman <yu...@yahoo.com.INVALID>.
Submitted: [1]
[1]: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
From: Benjamin Reed <br...@apache.org>
To: DevZooKeeper <de...@zookeeper.apache.org>; yuliya Feldman <yu...@yahoo.com>
Sent: Thursday, September 1, 2016 8:38 PM
Subject: Re: Issue with NettyServerCnxn.java
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 <yu...@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 <ha...@cloudera.com>
> To: UserZooKeeper <us...@zookeeper.apache.org>
> Cc: dev@zookeeper.apache.org; yuliya Feldman <yu...@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 <fp...@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 <ha...@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 <fp...@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
> > >> <yufeldman@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 <de...@zookeeper.apache.org>; yuliya Feldman <
> > >>>> yufeldman@yahoo.com>; "user@zookeeper.apache.org" <
> > >>>> user@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 <
> > >>>>> yufeldman@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.
>
>
>
>
Re: Issue with NettyServerCnxn.java
Posted by Benjamin Reed <br...@apache.org>.
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 <yu...@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 <ha...@cloudera.com>
> To: UserZooKeeper <us...@zookeeper.apache.org>
> Cc: dev@zookeeper.apache.org; yuliya Feldman <yu...@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 <fp...@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 <ha...@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 <fp...@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
> > >> <yufeldman@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 <de...@zookeeper.apache.org>; yuliya Feldman <
> > >>>> yufeldman@yahoo.com>; "user@zookeeper.apache.org" <
> > >>>> user@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 <
> > >>>>> yufeldman@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.
>
>
>
>
Re: Issue with NettyServerCnxn.java
Posted by yuliya Feldman <yu...@yahoo.com.INVALID>.
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 <ha...@cloudera.com>
To: UserZooKeeper <us...@zookeeper.apache.org>
Cc: dev@zookeeper.apache.org; yuliya Feldman <yu...@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 <fp...@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 <ha...@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 <fp...@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
> >> <yufeldman@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 <de...@zookeeper.apache.org>; yuliya Feldman <
> >>>> yufeldman@yahoo.com>; "user@zookeeper.apache.org" <
> >>>> user@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 <
> >>>>> yufeldman@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.
Re: Issue with NettyServerCnxn.java
Posted by yuliya Feldman <yu...@yahoo.com.INVALID>.
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 <ha...@cloudera.com>
To: UserZooKeeper <us...@zookeeper.apache.org>
Cc: dev@zookeeper.apache.org; yuliya Feldman <yu...@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 <fp...@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 <ha...@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 <fp...@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
> >> <yufeldman@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 <de...@zookeeper.apache.org>; yuliya Feldman <
> >>>> yufeldman@yahoo.com>; "user@zookeeper.apache.org" <
> >>>> user@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 <
> >>>>> yufeldman@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.
Re: Issue with NettyServerCnxn.java
Posted by Michael Han <ha...@cloudera.com>.
Make sense to me.
On Thu, Sep 1, 2016 at 12:10 PM, Flavio Junqueira <fp...@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 <ha...@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 <fp...@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
> >> <yufeldman@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 <de...@zookeeper.apache.org>; yuliya Feldman <
> >>>> yufeldman@yahoo.com>; "user@zookeeper.apache.org" <
> >>>> user@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 <
> >>>>> yufeldman@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.
Re: Issue with NettyServerCnxn.java
Posted by Michael Han <ha...@cloudera.com>.
Make sense to me.
On Thu, Sep 1, 2016 at 12:10 PM, Flavio Junqueira <fp...@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 <ha...@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 <fp...@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
> >> <yufeldman@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 <de...@zookeeper.apache.org>; yuliya Feldman <
> >>>> yufeldman@yahoo.com>; "user@zookeeper.apache.org" <
> >>>> user@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 <
> >>>>> yufeldman@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.
Re: Issue with NettyServerCnxn.java
Posted by Flavio Junqueira <fp...@apache.org>.
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 <ha...@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 <fp...@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
>> <yufeldman@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 <de...@zookeeper.apache.org>; yuliya Feldman <
>>>> yufeldman@yahoo.com>; "user@zookeeper.apache.org" <
>>>> user@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 <
>>>>> yufeldman@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.
Re: Issue with NettyServerCnxn.java
Posted by Flavio Junqueira <fp...@apache.org>.
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 <ha...@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 <fp...@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
>> <yufeldman@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 <de...@zookeeper.apache.org>; yuliya Feldman <
>>>> yufeldman@yahoo.com>; "user@zookeeper.apache.org" <
>>>> user@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 <
>>>>> yufeldman@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.
Re: Issue with NettyServerCnxn.java
Posted by Michael Han <ha...@cloudera.com>.
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 <fp...@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
> <yufeldman@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 <de...@zookeeper.apache.org>; yuliya Feldman <
> >> yufeldman@yahoo.com>; "user@zookeeper.apache.org" <
> >> user@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 <
> >>> yufeldman@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.
Re: Issue with NettyServerCnxn.java
Posted by Michael Han <ha...@cloudera.com>.
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 <fp...@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
> <yufeldman@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 <de...@zookeeper.apache.org>; yuliya Feldman <
> >> yufeldman@yahoo.com>; "user@zookeeper.apache.org" <
> >> user@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 <
> >>> yufeldman@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.
Re: Issue with NettyServerCnxn.java
Posted by Flavio Junqueira <fp...@apache.org>.
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 <yufeldman@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 <de...@zookeeper.apache.org>; yuliya Feldman <
>> yufeldman@yahoo.com>; "user@zookeeper.apache.org" <
>> user@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 <
>>> yufeldman@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
>>>>
>>>>
>>>
>>
>>
>>
>>
Re: Issue with NettyServerCnxn.java
Posted by Flavio Junqueira <fp...@apache.org>.
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 <yufeldman@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 <de...@zookeeper.apache.org>; yuliya Feldman <
>> yufeldman@yahoo.com>; "user@zookeeper.apache.org" <
>> user@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 <
>>> yufeldman@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
>>>>
>>>>
>>>
>>
>>
>>
>>
Re: Issue with NettyServerCnxn.java
Posted by Benjamin Reed <br...@apache.org>.
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 <yufeldman@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 <de...@zookeeper.apache.org>; yuliya Feldman <
> yufeldman@yahoo.com>; "user@zookeeper.apache.org" <
> user@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 <
> > yufeldman@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
> >>
> >>
> >
>
>
>
>
Re: Issue with NettyServerCnxn.java
Posted by Benjamin Reed <br...@apache.org>.
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 <yufeldman@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 <de...@zookeeper.apache.org>; yuliya Feldman <
> yufeldman@yahoo.com>; "user@zookeeper.apache.org" <
> user@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 <
> > yufeldman@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
> >>
> >>
> >
>
>
>
>
Re: Issue with NettyServerCnxn.java
Posted by yuliya Feldman <yu...@yahoo.com.INVALID>.
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 <de...@zookeeper.apache.org>; yuliya Feldman <yu...@yahoo.com>; "user@zookeeper.apache.org" <us...@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 <
> yufeldman@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
>>
>>
>
Re: Issue with NettyServerCnxn.java
Posted by yuliya Feldman <yu...@yahoo.com.INVALID>.
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 <de...@zookeeper.apache.org>; yuliya Feldman <yu...@yahoo.com>; "user@zookeeper.apache.org" <us...@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 <
> yufeldman@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
>>
>>
>
Re: Issue with NettyServerCnxn.java
Posted by Benjamin Reed <br...@apache.org>.
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 <
> yufeldman@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
>>
>>
>
Re: Issue with NettyServerCnxn.java
Posted by Benjamin Reed <br...@apache.org>.
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 <
> yufeldman@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
>>
>>
>