[AMQ-7452] Fix "Timer already cancelled" on new connections when shared inactivity timer dies#2206
Conversation
…ed inactivity timer dies
The static READ_CHECK_TIMER and WRITE_CHECK_TIMER in AbstractInactivityMonitor are
shared across all transport instances. Java's Timer permanently marks itself cancelled
when its internal thread exits due to an unhandled Error thrown by a TimerTask.
SchedulerTimerTask rethrows Error subclasses, so an OutOfMemoryError in any timer task
kills the shared timer thread while the static field remains non-null. All subsequent
schedule() calls on new connections then throw IllegalStateException ("Timer already
cancelled"), which propagates to TransportConnector.onAcceptError() and is logged as
WARN "Could not accept connection: Timer already cancelled."
Fix: wrap the schedule() calls in startConnectCheckTask() and startMonitorThreads()
in try/catch(IllegalStateException). On catch, the dead timer is cancelled (to let its
thread exit), replaced with a fresh instance, and the task is rescheduled. The guard
condition in startConnectCheckTask() is also extended with || READ_CHECK_TIMER == null
to cover the case where the field is unexpectedly null when CHECKER_COUNTER > 0.
Add three regression tests to InactivityMonitorTest covering:
- READ_CHECK_TIMER cancelled with CHECKER_COUNTER == 0 (startConnectCheckTask path)
- READ_CHECK_TIMER cancelled with CHECKER_COUNTER > 0 (production scenario)
- WRITE_CHECK_TIMER cancelled (startMonitorThreads path)
Tests use Wait.waitFor() instead of Thread.sleep() for reliability on slow CI.
|
I think this is the wrong approach to the fix because we are trying to clean up something that shouldn't happen and should be fatal. Generally Errors like OOM should be considered fatal and we should be just terminating the JVM. If we don't handle it and re-throw it the broker is going to be in a really bad state so continuing to try and run after OOM doesn't make sense to me. The SchedulerTimerTask already catches Errors, but then simply logs and rethrows them which is odd. My question is do we really want to keep trying to run if there was OOM? Probably not. I don't see a good reason to continue running if we get an Error, so I feel like we could just default to exiting the JVM on errors like this: cshannon@bfcd1c0 We could narrow the scope to VirtualMachineError but generally any thrown Error is probably very bad and we should treat it as fatal. |
2 participants
The static READ_CHECK_TIMER and WRITE_CHECK_TIMER in AbstractInactivityMonitor are shared across all transport instances. Java's Timer permanently marks itself cancelled when its internal thread exits due to an unhandled Error thrown by a TimerTask. SchedulerTimerTask rethrows Error subclasses, so an OutOfMemoryError in any timer task kills the shared timer thread while the static field remains non-null. All subsequent schedule() calls on new connections then throw IllegalStateException ("Timer already cancelled"), which propagates to TransportConnector.onAcceptError() and is logged as WARN "Could not accept connection: Timer already cancelled."
Fix: wrap the schedule() calls in startConnectCheckTask() and startMonitorThreads() in try/catch(IllegalStateException). On catch, the dead timer is cancelled (to let its thread exit), replaced with a fresh instance, and the task is rescheduled. The guard condition in startConnectCheckTask() is also extended with || READ_CHECK_TIMER == null to cover the case where the field is unexpectedly null when CHECKER_COUNTER > 0.
Add three regression tests to InactivityMonitorTest covering:
Tests use Wait.waitFor() instead of Thread.sleep() for reliability on slow CI.