*TimeOutHandlers

[Trustin Lee]

On Fri, Feb 13, 2009 at 6:06 AM, Christian Migowski
<chrismfwrd at gmail.com> wrote:
> Lets hope we can turn the page to the next one soon, just a few more
> comments on this one:
>
> 1) obvious bug (NPE, will always happen (at least in rev 869)): add
> allIdleTimeoutTask = new AllIdleTimeoutTask(ctx);
> after line 146 in IdleStateHandler

Fixed. :)

> 2) the HashedWheelTimer should get some additional userfriendly
> constructors, because nobody wants to know what tickDuration and
> ticksPerWheel are good for. >;-D
> I am thinking of something like
> HashedWheelTimer(HashedWheelTimer.QUITE_EXACT) which resembles to
> HashedWheelTimer(1, MILLISECONDS,100) and
> HashedWheelTimer(HashedWheelTimer.RESOURCE_EFFICIENT) which could be
> like HashedWheelTimer(1, SECONDS,10)

As an alternative, I added constructors which doesn't require
ticksPerWheel, and therefore a user can choose the precision of the
timer pretty easily.  (e.g. new HashedWheelTimer(1, SECONDS))  The
default ticksPerWheel is 512, which should work fine in general.  BTW
I think most people should be fine with using the default constructor,
which has the sensible default.

> 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)

>  3) upsetting nit-picking :-)
> - now the default timeunit has changed to seconds in the handler
> constructors, the parameters could be int instead of long again
> (making the necessary code smaller by saving three "L" characters :D)

Fixed.  BTW you don't need to add L, it will be automatically converted. ;)

> - IMHO the toString methods of the various ChannelEvents should print
> what they are first, then the Channel information  (I need to scroll
> to see what was just logged)

I agree that this indeed is annoying.  Let me come up with the best
string format.  Any suggestion would be appreciated.  I think the
channel UUID needs to be included anyway because it will be very
helpful for analyzing a large log file.

> 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. ;)

> otherwise: good job!

Thanks!  Please keep me updated with your feed back. :)

— Trustin Lee, http://gleamynode.net/