Command plugins should be able to opt-in to having the child processes killed when interrupted

[hassila]

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 somehow

I 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

1. Check out package-benchmark linked above
2. Build a simple benchmark with an infinite loop
3. Run the benchmark and abort with Ctrl-C

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)

swift-driver version: 1.62.15 Apple Swift version 5.7.1 (swiftlang-5.7.1.135.3 clang-1400.0.29.51)
Target: arm64-apple-macosx13.0
Darwin ice.local 22.3.0 Darwin Kernel Version 22.3.0: Mon Jan 30 20:38:37 PST 2023; root:xnu-8792.81.3~2/RELEASE_ARM64_T6000 arm64

[hassila]

Original case for package-benchmark was ordo-one/package-benchmark#74

[MaxDesiatov]

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.

[hassila]

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.

[tomerd]

I suspect this has to do with how the child process is spawned. In any case, this seems like a great API to add:

1. an API that allows plugin developers to register task / work for shutdown
2. an API to spawn child processes that automatically registers them for shutdown

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

[neonichu]

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.

[tomerd]

yes. also worth pointing out my suggestion above is for more than just child processes.

[hassila]

I use posix_spawn in both the command plugin and in its supporting tool:

https://github.com/ordo-one/package-benchmark/blob/f7d347b96e855b744bad417a463ab873b69c7a99/Plugins/BenchmarkCommandPlugin/BenchmarkCommandPlugin.swift#L271

and

https://github.com/ordo-one/package-benchmark/blob/f7d347b96e855b744bad417a463ab873b69c7a99/Plugins/BenchmarkTool/BenchmarkTool.swift#L260

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:

https://github.com/ordo-one/package-benchmark/blob/f7d347b96e855b744bad417a463ab873b69c7a99/Plugins/BenchmarkCommandPlugin/BenchmarkCommandPlugin.swift#L275

(status & 0xFF00) >> 8...

[neonichu]

Yah, I think we should really offer some kind of first-class API for spawning processes and this could be one of its features.

[adam-fowler]

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.

swift-package-manager/Sources/Basics/Cancellator.swift

Lines 67 to 72 in ce9f5b2

	signal(SIGINT, SIG_IGN)
	let interruptSignalSource = DispatchSource.makeSignalSource(signal: SIGINT)
	interruptSignalSource.setEventHandler { [weak self] in
	// cancel the trap?
	interruptSignalSource.cancel()

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

[hassila]

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

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....