[AMQ-7452] Fix "Timer already cancelled" on new connections when shared inactivity timer dies - #2206
[AMQ-7452] Fix "Timer already cancelled" on new connections when shared inactivity timer dies#2206jbonofre wants to merge 2 commits into
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. |
Per review feedback from cshannon: recovering from Error (e.g. OOM) is wrong because the JVM will be in a bad state. Instead, treat Error as fatal by default: log it and call System.exit(1). This removes the try/catch(IllegalStateException) recovery logic from AbstractInactivityMonitor and the associated regression tests, replacing the approach with an exit-on-error policy in SchedulerTimerTask via a new terminateOnError flag (defaulting to true). The catch block is also widened from Exception to Throwable for non-Error throwables.
|
I think the options are terminate, or throw an error that puts the broker in standby mode (formerly known as 'slave'). One benefit of that approach is that it draws attention to the OOM problem vs an automatic restart by a process monitor (systemd, kubernetes, wrapper, etc) after exiting. |
|
I think terminating makes the most sense, systemd (which I use a lot) monitors for errors and exit and can restart. So there' already attention drawn to it. I'm a -1 on any kind of standy for an error, we just need to shut down |
|
This is a critical component, I think having a unit test to confirm all our shutdown handlers and thread handling is correct for the JVM to shutdown when this occurs. Especially to confirm things like daemon v non-daemon threads, etc. |
I don't really know what you mean? the whole JVM will shut down...what is there to check? it's just going to kill the JVM including all threads regardless. I also don't know how you expect to test it because the JVM would die |
I think it's separate directly to this PR. Also I don't see an efficient way to have meaningful tests (as the JVM will be killed). |
The JVM won't exit if there is a non-daemon thread running. ActiveMQ sets daemon = false in several areas. ref: https://github.com/search?q=repo%3Aapache%2Factivemq+setDaemon&type=code edit: fixed the setDaemon(false) for non-daemon thread usage which blocks System.exit |
System.exit(1) kills the JVM, it doesn't matter what threads are running or not. It just halts the JVM. |
|
System.exit is going to go through an ordered shutdown it looks like but it will close everything. It looks like the only other way to avoid that is to use Runtime.getRuntime().halt(status) and that appears to short circuit things. So maybe we should just use Runtime.getRuntime().halt(status) to make sure we are not stuck shutting down |
|
One more option is to just stop catching Error entirely, it's a bit weird it was catching it at all. If we just don't catch it then the JVM would handle it and terminate, etc. |
| this.task.run(); | ||
| } catch (Error error) { | ||
| // Very bad error. Can't swallow this but can log it. | ||
| LOGGER.error("Scheduled task error", error); |
There was a problem hiding this comment.
I think the log should include the value of the terminateOnError flag here
|
@mattrpav and @jbonofre - the more I think about it, the more I think the better fix is to just not catch Error at all. We literally don't catch Error anywhere else, what are the odds this is the exact spot it fails? I have no idea why this is the one spot we would try and catch it. If we really want to handle Errors we should do it at a much higher level to handle errors anywhere else in the broker, and that is a much bigger scope outside of this. |
|
Digging around, other options may be to direct users to use: ..instead of trying to handle the system exit. The crux of the issue seems to be RuntimeException here killing the Timer, and we may not need to separate the error handling by checked/unchecked/Throwable/Error, etc. |
cshannon
left a comment
cshannon
left a comment
There was a problem hiding this comment.
@jbonofre - So I think we might just close this PR out. I talked to @mattrpav and have been going back and forth and I don't think this is necessary at all.
Conveniently, #2044 already solves the issue entirely.
The original issue was about OOM and causing things to continue. I think the real solution here is users should be setting the JVM arg to exit on out of memory if they want. Ie: -XX:+ExitOnOutOfMemoryError which #2044 does.
We already log then rethrow any Error which is good (we don't want to be catching Error and not rethrowing it). We could get rid of trying to catch Error entirely but I guess trying to log is ok.
|
Yes thanks for #2044 we should be good now |
3 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: treat Error as fatal in SchedulerTimerTask. A new terminateOnError flag (defaulting to true) causes the task to log the error and call System.exit(1) rather than rethrowing — preventing the broker from limping along in a potentially corrupted state after an OOM or similar JVM-level error. The exception catch is also widened from Exception to Throwable to cover non-Error throwables consistently.
Since SchedulerTimerTask now exits the JVM on Error, the shared timers can never silently enter a cancelled state while the broker continues running, so no recovery logic is needed in AbstractInactivityMonitor.