Open hassila opened 1 year ago
Original case for package-benchmark was https://github.com/ordo-one/package-benchmark/issues/74
I'd expect this to be the default behaviour and it feels weird to me it isn't. Maybe we should have an option that leaves child processes running instead, if anyone ever needs that for a plugin (I'm not convinced anyone does)?
The only concern then is changing this default behaviour, whether it's intentional or just a bug, after we had it this way for a couple of releases. But if there's a way to move towards stopping child processes by default, I personally would be in favor of that.
It'd be great as the default IMHO, it was a bit unexpected - just need to handle the case as we have where there are children spawned in multiple levels (plugin -> command_tool -> benchmark) and not just direct children.
I suspect this has to do with how the child process is spawned. In any case, this seems like a great API to add:
something like https://github.com/swift-server/swift-aws-lambda-runtime/blob/main/Sources/AWSLambdaRuntimeCore/Terminator.swift basically. SwiftPM has one already - https://github.com/apple/swift-package-manager/blob/main/Sources/Basics/Cancellator.swift, but it is not exposed to plugin authors and needs to be expose via the plugin API system
IIRC, being able to get all processes in a process group was one of the reasons for using posix_spawn
in TSC rather than Foundation.Process
.
yes. also worth pointing out my suggestion above is for more than just child processes.
I use posix_spawn
in both the command plugin and in its supporting tool:
and
If adding an API for spawning child processes and handling shutdown of them, please consider the use case described where the supporting tool that is executed by the command plugin also has need of spawning child processes in turn (so the whole tree of processes is terminated).
On a very much related note: Additional input to such API is the need for handling exit status from processes, currently it is very awkward if you want to extract the exit code from waitpid as you basically have to reimplement the C macro as command plugins can't depend on C modules to access the macros, please see comment and code at:
(status & 0xFF00) >> 8
...
Yah, I think we should really offer some kind of first-class API for spawning processes and this could be one of its features.
I have looked into this in a bit of detail as well in relation to running command plugins within VSCode. The first thing I found was SwiftPM only catches SIGINT to pass onto child processes. It should probably do the same for SIGTERM as well. I'm happy to add a PR for this if required. https://github.com/apple/swift-package-manager/blob/ce9f5b2d24d72c9217de6b7afb12b68989084790/Sources/Basics/Cancellator.swift#L67-L72
Secondly I was investigating the preview command from swift-docc-plugin
and that actually adds in SIGINT and SIGTERM handlers for the docc
process it spawned. So in the interim while there is a no support for managing child processes via SwiftPM @hassila you could do the same as swift-docc-plugin
.
https://github.com/apple/swift-docc-plugin/blob/6a8f81a21df0aef44513f5ce25b58ae758347789/Plugins/Swift-DocC%20Preview/SwiftDocCPreview.swift#L146
Secondly I was investigating the preview command from
swift-docc-plugin
and that actually adds in SIGINT and SIGTERM handlers for thedocc
process it spawned. So in the interim while there is a no support for managing child processes via SwiftPM @hassila you could do the same asswift-docc-plugin
. https://github.com/apple/swift-docc-plugin/blob/6a8f81a21df0aef44513f5ce25b58ae758347789/Plugins/Swift-DocC%20Preview/SwiftDocCPreview.swift#L146
Thanks @adam-fowler , we can try again (we did send signals to the children manually but got blocked by the sandbox as it looked, but maybe we just held it wrong, if DocC has it working it should work) - still would be nice to have infra for this....
Description
We have a command plugin for running benchmarks (https://github.com/ordo-one/package-benchmark) which works by having the command plugin run a supporting executable tool, which in turn runs the individual benchmark executable targets.
When it runs each benchmark, a progress bar is displayed. If a user has misconfigured a benchmark such that the ETA / runtime is excessive, they want to abort the run with
Ctrl-C
.What happens then is that the command plugin is killed, but the supporting tool and the benchmark that was running for a long time, will continue to run in the background for a potentially very long time (consuming tons of CPU).
We tried to hook things up with signals and manually killing the child processes, but the sandboxing seems to prohibit this.
So, I'd ask for some way for a command plugin to be able to kill all child processes (and their descendants) created by the command plugin when it exits.
Either: A) a way to (optionally) declare this for the command plugin and let SwiftPM kill them (would be awesome, as it seems this would be the desired behaviour for many command plugins) or B) Allow some way to do this even when sandboxed (and in fact, even when running with
--disable-sandbox
I couldn't get it to work for some reason as the child pid could not be found even though it seems to match and I believe the use of kill was correct, but maybe we were holding it wrong somehowI would argue that A) is the preferred solution as I think most command plugins would want this behaviour (to now leave stray child processes living forever)
Expected behavior
When a command plugin is aborted with e.g. Ctrl-C (SIGINT etc), child processes and their descendants should be killed of.
Actual behavior
Child processes continue to live.
Steps to reproduce
I don't think you need a reproducer for this specific case, but I can write one up if it helps you.
Swift Package Manager version/commit hash
Swift Package Manager - Swift 5.7.1
Swift & OS version (output of
swift --version && uname -a
)