fix(suspendapp): avoid deadlock and align JVM behaviour on system.exit#3930
fix(suspendapp): avoid deadlock and align JVM behaviour on system.exit#3930tKe wants to merge 2 commits into
Conversation
Kover Report
|
|
@serras rebasing this, which includes job name fixes for the js tests, highlighted that the exitApp change has introduced a regression in then wasm/js signal handling as it now reports an exitCode of 255 not 128+sig. I'll include a fix in this rebase. |
2272659 to
3b13f57
Compare
There was a problem hiding this comment.
Should we perhaps just e.printStackTrace() here? I guess this is what was swallowing the IllegalArgumentException that was thrown for using SIGINT vs just INT.
Alternatively we do not catch it at all and just let it throw?
There was a problem hiding this comment.
It's the invocation of Signal(signal) that throws when the signal is unknown.
The Signal.handle invocation may also throw if the signal is reserved by the VM/OS.
It might be safer to e.printStackTrace or add a custom log warning that signal handling for SuspendApp may be impacted. Otherwise the cases where we don't cancel on signal as we haven't installed the handler would be hard to diagnose.
| Runtime.getRuntime().removeShutdownHook(hook) | ||
| try { | ||
| Runtime.getRuntime().removeShutdownHook(hook) | ||
| } catch (_: IllegalStateException) { |
There was a problem hiding this comment.
Does this occur? It should be gated by isShutdown, right?
There was a problem hiding this comment.
Unlikely though it is, there's the possibility the runtime shutdown process has been started but our shutdown hook thread has not yet started or set the isShutdown flag. Attempting to remove hooks once the shutdown process has started will fail.
| Signal.handle(Signal(signal)) { prev -> | ||
| runBlocking { action() } | ||
| Signal.handle(Signal(signal)) { sig -> | ||
| runBlocking { action(sig.number) } |
There was a problem hiding this comment.
This violates the "exit code formula" Exit Code=128+Signal Number.
The exit code formula Exit Code=128+Signal Number is not being respected here since now we set 2 for SIGINT, and 15 for SIGTERM. This seems consistently implemented in the other platforms.
There was a problem hiding this comment.
This is consistent with the other platforms where the onSigInt/onSigTerm blocks receive the signal number (i.e. 2 / 15). We could name the parameter of the lambda for clarity and avoid using unqualified code in most places but I didn't want to make too many tweaks outside of the main area of impact.
The other platforms handlers tend to do the sig+128 maths to convert an exit code in the block passed to onSigInt/onSigTerm.
i.e. { code -> exitAfter(128 + code) { block() } }
For the JVM, we let the JVM handle the exit code calculation based on the default signal handler so we needn't do the calculation here (although we could theoretically call System.halt we don't know if other shutdown hooks have finished or not, nor have control of the ordering)
| delay(0.5.seconds) | ||
| sendSignal(Signal.SIGTERM) | ||
| } | ||
| // TODO: inconsistent exit codes across platforms, for now just check it's not a success |
There was a problem hiding this comment.
Seems like an inconsistency introduced in this PR? See other comment https://github.com/arrow-kt/arrow/pull/3930/changes#r3519183429.
There was a problem hiding this comment.
Currently, I believe the inconsistency is on JVM still exiting with the correct signalled exit code (128+sig) but the other platforms are exiting with exit code 255 due to the timeout exception.
JvmSpec:waitAndTimeout exitCode == 143
JsSpec:waitAndTimeout exitCode == 255
WasmJsSpec:waitAndTimeout exitCode == 255
NativeSpec:waitAndTimeout exitCode == 255
|
@tKe couple of small questions and remarks. Feels like a relatively large change, but it mostly tries to prevent hanging indefinitely which I guess is okay. What is a bit strange to me is that now silently in those cases the shutdown will not happen. Tagging @alexandru since he recently also worked here and is familiar with semantics of shutdown and would like to hear his thoughts if he has time to chime in. |
|
Thanks for the review @nomisRev. I've addressed or replied to your feedback. It is a large change, but ultimately the shutdown hook not cancelling the app on It also aligns the platform handling of Only Otherwise the change only impacts when SuspendApp isn't on a non-daemon thread (which would be very unexpected usage) as it would otherwise have unregistered the shutdown hook before it would be invoked due to last non-daemon thread terminating. For the expected usage patterns that aren't calling |
|
@nomisRev are you OK with the reviewed code? I’d like to merge this one, since it seems it fixes an important problem. |
3 participants
fixes #3846
This fix essentially sidesteps the deadlock by not attempting to cancel/graceful-shutdown when the hook is invoked for any reason other than SIGINT/SIGTERM.
The rationale is that
System.exitequivalents on other platforms doesn't trigger cancellation of the SuspendApp, and the other hook invocation cause (last non-daemon thread terminated) is not expected through the normal usage of SuspendApp (that is, as the entry point on the main thread for a service/application).Instead, this fixes the Signal handling, captures the signal, and limits triggering cancellation to only when signalled.