Refactor SimpleCommandLine to avoid JVM exit, correctly handle context lifetime
Description
Environment
Activity
Ian Young February 1, 2023 at 5:13 PM
Documented the issue in the release notes under the section for https://shibboleth.atlassian.net/browse/MDA-275#icft=MDA-275, the change to the classes that avoids creating the non-daemon threads.
Ian Young February 1, 2023 at 4:31 PM
Reviewed this and raised https://shibboleth.atlassian.net/browse/MDA-275 to deal with the daemon thread issue. It’s better, I think, to fix that at the level of the individual stages than continuing with the old behaviour, which might in principle cause real non-daemon threads to be prematurely terminated. The change does need to be documented, though.
Ian Young January 31, 2023 at 6:31 AM
While this was a good cleanup, avoiding System.exit turns out to have a side-effect I hadn't anticipated, so this needs to be looked at again.
It turns out that in some configurations, there are thread pools being implicitly created with non-daemon threads. Although this is fine in the context of a web service style of MDA use, if you're calling the MDA as a CLI application it will never terminate.
Arguably, this is something we could document and therefore implicitly turn into "your configuration is incorrect", but that seems pretty harsh. It's probably worth using an explicit System.exit(0) in the CLI in the normal termination path to preserve the old behaviour.
It's probably also worth reviewing the classes involved in allocating these thread pools (I think PipelineDemultiplexerStage, SplitMergeStage, PipelineMergeStage are implicated) to see whether at least the executors they construct by default should use daemon threads, perhaps through a common singleton.
Ian Young October 20, 2022 at 10:58 AM
Done, commit 9e3450b62024e16e0896eded42a5eb36383f0b4d.
SimpleCommandLineis a very old piece of code and should be modernised to:use try-with-resources in constructing its Spring context
use exceptions rather than
System.exitfor, er, exceptional exitsno longer use a VM shutdown hook to clean up the context, instead relying on the try-with-resources
This won’t change the behaviour from the user perspective at all, but will make the code a lot more readable and maintainable.