*TimeOutHandlers

[Trustin Lee]

On Fri, Feb 13, 2009 at 6:11 PM, Christian Migowski
<chrismfwrd at gmail.com> wrote:
> two more (minor) bugs to squash:
> IdleStateHandler line 240 should be
> channelIdle(ctx, IdleState.WRITER_IDLE, lastWriteTime);
> IdleStateHandler line 274 should be
> channelIdle(ctx, IdleState.ALL_IDLE, lastIoTime);
> (in both the last parameter is wrong)

Doh! Sorry for not testing enough and copying and pasting.  Thanks again!

> BTW, I am no Java guru, but is there a special point in doing e.g.
> long lastWriteTime = IdleStateHandler.this.lastWriteTime;
> in the inner classes instead of just accessing it directly (I always
> seek to extend my Java knowledge).

It's because lastWriteTime can be changed by a different thread while
I check the idle state.  Just accessing this.lastWriteTime more than
once might give different values.

>>> 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.
>
> ok
>
>>> 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)
>
> agree, you are right
>
>>>  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. ;)
>
> but tools like FindBugs or PMD which do even more nitpicking than me
> complain when you omit the L ;-)

Aha.  Those false positives.. I agree it's sometimes annoying. :)

>>> - 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.
>
> yes, the channel info should be included, it is useful. I think it is
> sufficient to just change positions, put the getChannel().toString()
> at the end of the string (after all this is just for debugging and
> doesn't need to be thought about in great detail)

OK.

>>> 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. ;)
>
> It's extensibility is indeed quite cool.

Yes.  I was somewhat worried about the cost of instantiating event
objects, but it does worth to do so.

> Thanks a lot,

Anytime!

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