netty / netty

Netty project - an event-driven asynchronous network application framework
http://netty.io
Apache License 2.0
33.35k stars 15.9k forks source link

HashedWheelTimer hides exception #9648

Open fbacchella opened 5 years ago

fbacchella commented 5 years ago

At this line: https://github.com/netty/netty/blob/3a48010c42615e2f5403b89d631ca126fa068cfb/common/src/main/java/io/netty/util/HashedWheelTimer.java#L673

All throwable are catched and logged with an netty logger at warn level. But throwable is a super class of Error and the javadoc says:

An Error is a subclass of Throwable that indicates serious problems that a reasonable application should not try to catch. Most such errors are abnormal conditions. The ThreadDeath error, though a "normal" condition, is also a subclass of Error because most applications should not try to catch it.

warn is a quite low level, that can hides more sever exceptions like NPE. I think a provided exception handler should be added here.

normanmaurer commented 4 years ago

@fbacchella what exception handler ?

fbacchella commented 4 years ago

Something provided by the user of the library as a callback, to be notified of unexpected exceptions.

carryxyh commented 4 years ago

Hi, how is this going?

Maybe don't need an exceptionHandler, you only need to handle exceptions that require special handling in the run method of TimerTask:

try {
     // do business logic..
} catch(Exception e) {
    // do your exception handle..
}

The role of hashedwheeltimer is actually the same as scheduledexecutor, which periodically triggers tasks. The specific exception logic should also be handled by the user. (Scheduledexecutor also does not provide exceptionhandler)

WDYT..? @fbacchella

fbacchella commented 4 years ago

That's the way I solved this problem. But if you don't know how netty behave, you can miss that. And again I'm not sure that catching throwable is a good idea.