Return Ok on kill if process has already exited

[ChrisDenton]

This will require an FCP from @rust-lang/libs-api.

Fixes #112423. See that issue for more details.

[rustbot]

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

[Amanieu]

@rfcbot fcp merge

[rfcbot]

Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se

Concerns:

• breaking-change resolved by Return Ok on kill if process has already exited #112594 (comment)

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[BurntSushi]

@rfcbot concern breaking-change

I am somewhat concerned by the breaking change here. The routine here explicitly documented that an InvalidInput error would be returned if kill was called on a process that had already terminated. I do agree that an Ok(()) here probably makes more sense, and this seems like a sensible resolution to the linked bug report. But it is also a breaking change.

Do we have any user stories of folks that might be relying on the previous behavior that would be impacted by this change? My sense is that this is probably fine, but I think we should try to think about potential breakage here before making the change. A crater run might be helpful here too, although I'd be surprised if it provoked the necessary conditions to detect a breaking change here.

[ChrisDenton]

Yeah, the problem is that, despite the documentation, InvalidInput is only returned after a call to wait() and even then only on Unix. EDIT: I should add that in the meeting it was felt that calling kill after wait is pointless as documented and would be strange.

[m-ou-se]

(Mostly repeating what Chris said above.)

I am somewhat concerned by the breaking change here. The routine here explicitly documented that an InvalidInput error would be returned if kill was called on a process that had already terminated.

@BurntSushi For all reasonable uses of .kill(), that documentation is wrong. On Unix, kill() before wait()/try_wait() doesn't return Err(InvalidInput), but returns Ok(()), even for already terminated processes. On Windows, .kill() on a terminated process returns a different error.

The only case where .kill() currently returns Err(InvalidInput) is on Unix when calling .kill() after calling .wait(), which is probably just a bug in the code, as that would never do anything useful.

(Similarly, calling .id() after .wait() also seems broken, as that id could then already have been re-used. Perhaps we should make .id() panic instead of having it return a 'dangling' pid. But that's a separate issue I guess.)

[BurntSushi]

@m-ou-se ah I get it now! Thanks for explaining a bit more. That makes sense.

@rfcbot resolve breaking-change

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[bors]

⌛ Testing commit 4309954 with merge 4ee3254b6cd7de9d9a27affc914fab13f88d5a77...

[bors]

💔 Test failed - checks-actions

[Amanieu]

Hm, does android not have echo? The known_command function appears to assume it does.

Android 13 definitely has /system/bin/echo (which is in PATH). However it may not be present on older versions, such as the Android emulator we are using in CI.

Perhaps you could try something using shell_cmd instead?

[Amanieu]

@bors r+

[bors]

📌 Commit 9e5f61f has been approved by Amanieu

It is now in the queue for this repository.

[bors]

⌛ Testing commit 9e5f61f with merge dfe0683...

[bors]

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing dfe0683 to master...

[rust-timer]

Finished benchmarking commit (dfe0683): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.7%	[2.1%, 3.3%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.4%	[-2.5%, -2.3%]	2
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 655.506s -> 653.976s (-0.23%)