*TimeOutHandlers

[Trustin Lee]

On Sat, Feb 14, 2009 at 7:37 AM, Christian Migowski
<chrismfwrd at gmail.com> wrote:
> I just can't stop ;) (to my defense: i am belaboring this issue
> because it offers me a good "hook" to learn and understand more about
> Netty which i intend to use for some rather important projects)

:)

> On Fri, Feb 13, 2009 at 6:55 AM, Trustin Lee <tlee at redhat.com> wrote:
>>> The creation of the timer could be even integrated in the constructor
>>> of the timeout handlers.
>>
>> This can be somewhat confusing.  For example, a user can create
>> thousands of HashedWheelTimer threads if he or she doesn't understand
>> what is created inside the timeout / idle state handlers.  So, I'd
>> like to keep it as it is.  Instead, I'm going to implement a shared
>> resource explosion detector, which gives a user warning when expensive
>> resources are created too much (e.g. 1000 HashedWheelTimers or 1000
>> ChannelFactories)
>
> The integrated Timer could be made static (as another alternative,
> i.e. timer = staticTimer; if used so) in the timeout handlers to avoid
> the scenario you described. This would simplify the timeout handler
> api (the timer is just needed for the timeout handlers so could be
> created internal)

Multiple Netty JARs might be running in different classloaders of a
container, and some of them might need to be reloaded.  How do we stop
the timer in this case?

>>> 4) personal wish
>>> any chance that the IdleStateEvent gets promoted to a "proper"
>>> UpstreamEvent (i.e. there will be a channelIdle() method in
>>> ChannelUpstreamHandler)?
>>
>> I think I can create IdleStateAwareChannelHandler in the
>> org.jboss.netty.handler.timeout package, which extends
>> SimpleChannelHandler instead.  I think this is the beauty of Netty's
>> flexible event model and this should work for you. ;)
>
> Also for the sake of of a simple and convenient api i still think it
> should be in SimpleChannelHandler instead of having an additional
> class (IdleStateAwareChannelHandler). Sure, if a user doesn't have a
> timeout handler in its pipeline the channelIdle method will never be
> called, but this is the same for all the DownstreamEvents (resp. their
> methods) that do not apply their current "whatever_this_netty_term_is"
> (you know, e.g. channelBound and channelUnbound will never be called
> if it is a ClientBootStrap pipeline).

This is not the point of this discussion, but to be as correct as
possible: there's no method that is not supposed to be called with the
current design (except for bindRequested() for a client side channel,
but this is a very exceptional case).  For example, a user can call
unbind() or disconnect() on a client side channel, and it will cause
unbindRequested() or disconnectRequested() to be invoked and
eventually close the channel.  If you were dealing with a
reconnectable transport, you would have been able to reconnect without
creating a new channel by calling unbind() or disconnect().  However,
it's not ready yet.

> And detecting idleness isn't such an uncommon use pattern which could
> be "advertised" in one of Nettys most important API classes. (The
> ReadTimeoutHandler is only there to be capable of handling it in an
> SimpleChannelHandler instance, no?)

There are two points of view for an idle state.  One sees it as an
exceptional situation and the other sees it as a part of the protocol.
 ReadTimeoutHandler and WriteTimeoutHandler serves the first point of
view and IdleStateHandler serves the second.  And you might want to
mix the two, like adding IdleStateHandler and WriteTimeoutHandler (not
ReadTimeoutHandler probably though :)

People who use ReadTimeoutHandler or WriteTimeoutHandler will probably
close the connection when ReadTimeoutException or
WriteTimeoutException because it is not allowed for a remote peer to
send a message too late (possible DoS) or to receive a message too
late (also possible DoS.)  People who use IdleStateHandler will
probably send some heartbeat message to keep the connection alive and
take some additional action (e.g. closing the channel) if no response
for the heartbeat message is received (using WriteTimeoutHandler or
directly accessing the Timer.)

So, it's not only there to be capable of handling it in a
SimpleChannelHandler instance but there are two types of protocols (or
developer stereotypes? :) for Netty to serve.

Anyway, I think this is not very important for this discussion again
because the point here is the matter of making timeouts and idle state
detection optional or not, and you are making some good points.  BTW,
you can handle whatever event type in handleUpstream /
handleDownstream. ;)

> But after all it's your framework and I want to state explicitly that
> the API is quite good already as it is (it just could be even better
> ;)).

I'm somewhat worried about the bloated API and code duplication in
each transport implementations, and that's why I'm hesitating.  Let me
think a little bit more about this and make a decision.  I might be
able to find some place where I can inject this very common aspect. ;)

Thanks for your ongoing effort to make Netty better, and I really
appreciated it.  Please keep it up! :)

Cheers,
Trustin