zookeeper-user mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Flavio Junqueira <...@apache.org>
Subject Re: Issue with NettyServerCnxn.java
Date Thu, 01 Sep 2016 19:10:26 GMT
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.


> On 01 Sep 2016, at 20:05, Michael Han <hanm@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 <fpj@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 <breed@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 <breed@apache.org>
>>>> To: Patrick Hunt <phunt@apache.org>
>>>> Cc: DevZooKeeper <dev@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 <phunt@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
>>>>> 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 -
>>>>>> channel.close() in Netty is asynch. and subsequently lead to some
>>>>>> 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
>>>>>> processed anymore.
>>>>>> In NIOServerCnxn.java in sendResponse() it is catching Exception
>>>> 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.

View raw message