std.process: Do not kill process group 2 in unittest

[helmutg]

When passing a Pid object to tryWait(), its processId will be changed to Pid.terminated, which happens to be -2. Passing this value to kill() will kill the process group 2 on Posix systems. If you run the build and test suite using this process group, the test case will kill the entire build.

Bug-Debian: https://bugs.debian.org/1089007

[dlang-bot]

Thanks for your pull request and interest in making D better, @helmutg! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#10586"

[CyberShadow]

Thanks, but wouldn't it make more sense to have kill throw ProcessException if it's passed a Pid with pid == terminated? That seems to be the intention of the test.

[helmutg]

Thanks, but wouldn't it make more sense to have kill throw ProcessException if it's passed a Pid with pid == terminated? That seems to be the intention of the test.

That is a very reasonable question. Thank you for the review. Is the kill function supposed to be able to kill process groups? Do process groups exist on Windows? Does existing code assume that kill kills process groups? Whether changing kill is better depends on the answers to these questions.

What I am proposing here is a minimum viable solution that makes gcc build rather than fail its build. If changing kill, we change behavior and may break other code that assumes the ability to kill process groups. That seems like a risky affair and it feels unlikely that such a patch would be backported into older gcc releases.

[CyberShadow]

Is the kill function supposed to be able to kill process groups?

No, I think it is designed to only kill child processes, created using other std.process functions.

Does existing code assume that kill kills process groups?

Seems highly unlikely. The unittest in question seems to assume that kill will throw a ProcessException if pid == terminated, but it passing previously seemed to be accidental and contingent on a process group with the number 2 not existing.

What I am proposing here is a minimum viable solution that makes gcc build rather than fail its build. If changing kill, we change behavior and may break other code that assumes the ability to kill process groups. That seems like a risky affair and it feels unlikely that such a patch would be backported into older gcc releases.

True. We can go forward with this change as well, though I would add a TODO to note that the condition may not be what was originally intended to be tested.

Let's see what other Phobos maintainers say as well. @schveiguy ?

not applicable.

[CyberShadow]

#10601

[helmutg]

#10601

My understanding is that the proposed MR obsoletes this MR if merged, but I am not convinced that it is eligible for backporting into older gcc releases.

[CyberShadow]

Why is that? What would make it eligible?

[helmutg]

Why is that? What would make it eligible?

Your change is changing semantics. Effectively, you'd have to provide a good argument for why it cannot break existing code that relies on the unintended behavior. You cannot provide such a proof. Hence, I do not expect your change to be backportable.

Keep in mind that there are two partially distinct bugs here:

1. Running the libphobos test suite kills the entire gcc build.
2. kill does not reliably raise an exception for invalid Pid objects.

The first of these is something we want to resolve in older releases, because they fail to build. It is not as clear that we also need to fix the latter in old releases.

[CyberShadow]

You cannot provide such a proof.

I think the fact that it is not possible to create Pid objects directly (and, therefore, intentionally construct a Pid object with an arbitrary negative osHandle) should be sufficient?

I can't think of a way to write a program that would be affected by this, without resorting to raw pointer manipulation, or abusing the invalid osHandle values to intentionally refer to PIDs -1 or -2.

[helmutg]

That sounds like a strong reason. I appreciate if you can try getting this ported to gcc versions 15, 14, 13, 12, 11 and 10.

[CyberShadow]

@ibuclaw Is that something we can do?

[schveiguy]

My understanding is that the proposed MR obsoletes this MR if merged

I think that is correct.