Implement child process reaping. #10720

peterhuene · 2019-02-07T01:50:24Z

This PR implements child process reaping in the event of termination
of a CLI command.

On Windows, the child process is added to a job object that is set to
terminate the child (and its tree) upon the termination of the parent
dotnet process. On Windows 7 and Server 2008, the dotnet process cannot
already be associated with a job object for the reaping to occur. On
later Windows versions, a nested job will be created so the reaping will
still occur. After the child process exits, the job object is closed
without terminating the remaining processes in the job; this allows for
the child process to spawn additional processes that outlive the child.

On POSIX operating systems, a SIGTERM is intercepted and forwarded on to
the child process only. Like the SIGINT forwarding, it is up to the
child process to decide what to do with the SIGTERM signal (the default
is to abort).

Additionally, this fix further expands upon the previous fix to dotnet run to properly handle SIGINT so that all child processes now benefit
from the fixed behavior. This means MSBuild-forwarding commands like
dotnet build now behave as if MSBuild were directly being executed with
respect to Ctrl-C handling.

Fixes #7426.

This commit implements child process reaping in the event of termination of a CLI command. On Windows, the child process is added to a job object that is set to terminate the child (and its tree) upon the termination of the parent dotnet process. On Windows 7 and Server 2008, the dotnet process cannot already be associated with a job object for the reaping to occur. On later Windows versions, a nested job will be created so the reaping will still occur. After the child process exits, the job object is closed without terminating the remaining processes in the job; this allows for the child process to spawn additional processes that outlive the child. On POSIX operating systems, a SIGTERM is intercepted and forwarded on to the child process only. Like the SIGINT forwarding, it is up to the child process to decide what to do with the SIGTERM signal (the default is to abort). Additionally, this fix further expands upon the previous fix to `dotnet run` to properly handle SIGINT so that all child processes now benefit from the fixed behavior. This means MSBuild-forwarding commands like `dotnet build` now behave as if MSBuild were directly being executed with respect to Ctrl-C handling. Fixes #7426.

This commit deletes the `DepsJsonCommandFactory`, `ProjectDependenciesCommandFactory`, and `PublishedPathCommandFactory` types as they are not instantiated from product code.

peterhuene · 2019-02-07T02:04:46Z

I will likely still implement one more thing for this PR: sending SIGTERM on POSIX to the parent process group if it matches the pid of the parent (i.e. the parent is in its own process group). That way POSIX behaves a little more like Windows in terms of it trying to reap the entire tree. There will still be the inherent difference that SIGKILL will do no reaping at all on POSIX, whereas terminating the process on Windows will reap.

nguerrera · 2019-02-07T02:21:22Z

IIRC, msbuild has its own handler that kills child processes.

If so, how do these interact with each other? Should they use the same code/approach?

nguerrera · 2019-02-07T02:24:51Z

https://github.com/Microsoft/msbuild/blob/e70a3159d64f9ed6ec3b60253ef863fa883a99b1/src/Utilities/ProcessExtensions.cs#L14

peterhuene · 2019-02-07T02:28:37Z

It seems that invokes when a task times out.

Ours shouldn't interfere with that as we also don't kill any process unless the parent dotnet process terminates on Windows or is sent SIGTERM on POSIX (which doesn't kill, just forwards the signal).

cc @rainersigwald though.

peterhuene · 2019-02-07T02:29:47Z

Looks like something is holding file locks on two Windows CI legs 😞

peterhuene · 2019-02-07T02:31:27Z

No wait, the copy failed because of this Operation did not complete successfully because the file contains a virus or potentially unwanted software.. This is dotnet-msbuild.Tests.dll 🤔.

src/Microsoft.DotNet.Cli.Utils/NativeMethods.cs

rainersigwald

This seems good to me.

The behavior on a primary-MSBuild-node crash will be slightly different from full-framework MSBuild (with no job object), but it's not really worse in my understanding:

full framework

Successful build (new nodes created)

Build starts
worker nodes launched
build completes
primary node shuts down
Workers live until idle timeout, then shut down

Successful build (node reuse)

Build starts
Connects to worker nodes
build completes
primary node shuts down
Workers live until idle timeout, then shut down

Crash (new nodes)

Build starts
Worker nodes launched
Primary node crashes
Worker nodes attempt to communicate with primary, fail, shut down

Crash (node reuse)

Build starts
Connects to worker nodes
Primary node crashes
Worker nodes attempt to communicate with primary, fail, shut down

With this

Successful build (new nodes created)

Build starts
worker nodes launched
build completes
primary node shuts down
job object kill disabled
Workers live until idle timeout, then shut down

Successful build (node reuse)

Build starts
Connects to worker nodes not in job
build completes
primary node shuts down
Workers live until idle timeout, then shut down

Crash (new nodes)

Build starts
Worker nodes launched
Primary node crashes
job object killed, killing workers and tool children

Crash (node reuse)

Build starts
Connects to worker nodes
Primary node crashes
job object killed, killing any tool children and new nodes but not reused workers or their children
Worker nodes attempt to communicate with primary, fail, shut down

peterhuene · 2019-02-07T19:29:20Z

@rainersigwald to be clear, the termination would only occur if the root dotnet process is rudely terminated; if the child (primary) MSBuild process crashes or is otherwise rudely terminated, dotnet will gracefully terminate with a non-zero exit code, but will not reap any processes. dotnet is only guarding against its own termination (or a graceful SIGTERM on POSIX); if the direct child is killed then there is no behavior change from how things work today.

This commit implements sending SIGTERM to the process group if the current process is the root of a group. This ensures that all descendant processes receive the SIGTERM signal. If the current process is not at the root (e.g. a child of another root), then only the direct child will be sent the SIGTERM signal.

peterhuene · 2019-02-07T21:17:50Z

Ok I've implemented sending SIGTERM to the current group (if the current process is the root of the group). This ensures that all descendants of dotnet receive the SIGTERM signal when a SIGTERM is sent to dotnet.

This should now be fully ready for review.

src/Microsoft.DotNet.Cli.Utils/ProcessReaper.cs

There's a race in `DotnetUnderTest.FullPath` on Windows that may result in the path to dotnet to have a `.exe.exe` suffix when running tests in parallel. The root of the problem is that the method is not thread safe and relies on static state to calculate the result; the state may have already changed as a result of another thread of execution. Rather than lock or otherwise change this code, this fix simply removes the dependency on the static state to do the calculation. The resulting method is still not thread safe, but now no longer matters if the full path is calculated concurrently.

danmoseley · 2019-02-09T01:31:59Z

@tmds since you have worked on process killing stuff in CoreFX, could you possibly cast a quick eye over this one too?

tmds · 2019-02-12T09:10:24Z

src/Microsoft.DotNet.Cli.Utils/ProcessReaper.cs

+
+            // First, try to SIGTERM our process group
+            // If the pgid is not the same as pid, then this process is not the root of the group
+            if (NativeMethods.Posix.getpgid(currentPid) == currentPid)


There is no code that calls `setpgid. Why deal with this specifically?

If this process is a child of another that doesn't setpgid (e.g. another .NET process that spawns dotnet), then this check will be false. It was to avoid a failing call to kill for the group. If you think that's unnecessary and I should just call kill and check for ESRCH for the group, I'm fine with changing it.

I'm wondering when this would be true, so my comment was more suggesting to remove the whole if block.

Most shells setpgid though, right?

Ah, right. If I start dotnet from a bash script though, they are not the same.
This may cause subtle differences in behaviour.
If we remove this, things will still work, and should be consistent independent of being process group leader or not.

So unfortunately setpgid must occur before the exec* call (for valid reasons), and I'm not going to duplicate the complex process creation code that the framework does just for this fix.

As such, I think I'll remove the pg kill.

So unfortunately setpgid must occur before the exec* call (for valid reasons), and I'm not going to duplicate the complex process creation code

Setting it before calling Process.Start would be enough.
That said, I think we should keep it simple and not change the process group unless we need to.

Doesn't setpgid require a fork prior to being called? I thought the framework only forks as a result of Process.Start (seems to happen in StartCore in the Unix implementation.

Or did you meant to setpgid the parent dotnet process before spawning the child? Given that executing from a shell usually means we're already in our own process group, setpgid would be ineffectual. In the case where the dotnet process is not in its own group, then it would probably be unwise to break out.

The only consistent way to do it would to be setpgid the child prior to the exec, which we can't do unless CoreFX implements dotnet/corefx#17412.

Or did you meant to setpgid the parent dotnet process before spawning the child?

That is what I meant, so the child would inherit it from the parent.

In the case where the dotnet process is not in its own group, then it would probably be unwise to break out.

The only consistent way to do it would to be setpgid the child prior to the exec, which we can't do unless CoreFX implements dotnet/corefx#17412.

It's more tricky than I thought. If the dotnet process would change its own pgid, it would no longer receive signals (e.g. SIGINT) from the console. So setpgid needs to occur between fork and exec of the child. And the parent needs to take care of propagating the signals to the children.

Plus, no managed code can safely run between fork and exec. If we ever need something there, we’d need API changes in the BCL. (Édit: I see that was already pointed out.)

src/Microsoft.DotNet.Cli.Utils/NativeMethods.cs

tmds · 2019-02-12T09:16:47Z

src/Microsoft.DotNet.Cli.Utils/ProcessReaper.cs

+                }
+            }
+
+            if (sendChildSIGTERM && NativeMethods.Posix.kill(_process.Id, NativeMethods.Posix.SIGTERM) != 0)


If you want to be safe for 'pid recycling', you can add a WaitForExit(0) and do nothing if the process has already exited.

I'll add that. Thanks!

tmds · 2019-02-12T09:19:50Z

@danmosemsft @peterhuene LGTM, added a few minor comments.

danmoseley · 2019-02-12T12:12:03Z

Thanks @tmds. @peterhuene, @tmds is from RedHat and helps us with Linux subtleties.

peterhuene · 2019-02-12T16:14:19Z

Very much appreciated for the additional review! I'll put up another PR with the feedback changes.

steven-solomon · 2019-04-27T18:17:05Z

Is it the case that this fix won't ship until some 3.0.1.X release?

peterhuene · 2019-04-27T18:32:33Z

Hi @steven-solomon. That's correct. This fix is available in the current 3.0.100 SDK previews. Unfortunately, it doesn't meet the servicing bar to back port it to previous releases.

steven-solomon · 2019-04-27T18:53:12Z

@peterhuene Thank you for clarifying.

Peter Huene added 2 commits February 6, 2019 17:43

Delete unused code.

70e0913

This commit deletes the `DepsJsonCommandFactory`, `ProjectDependenciesCommandFactory`, and `PublishedPathCommandFactory` types as they are not instantiated from product code.

peterhuene added this to the 3.0.1xx milestone Feb 7, 2019

peterhuene requested a review from a team February 7, 2019 01:50

nguerrera reviewed Feb 7, 2019

View reviewed changes

src/Microsoft.DotNet.Cli.Utils/NativeMethods.cs Show resolved Hide resolved

rainersigwald approved these changes Feb 7, 2019

View reviewed changes

nguerrera approved these changes Feb 7, 2019

View reviewed changes

src/Microsoft.DotNet.Cli.Utils/ProcessReaper.cs Show resolved Hide resolved

peterhuene merged commit 14add78 into dotnet:master Feb 8, 2019

peterhuene deleted the fix-dotnet-run-termination branch February 8, 2019 02:09

tmds reviewed Feb 12, 2019

View reviewed changes

src/Microsoft.DotNet.Cli.Utils/NativeMethods.cs Show resolved Hide resolved

tmds reviewed Feb 12, 2019

View reviewed changes

EEParker mentioned this pull request Mar 28, 2019

Enhancement: Cleaning up child node process after ending debug EEParker/aspnetcore-vueclimiddleware#8

Closed

pakrym mentioned this pull request Jan 31, 2020

Adding child processes to job objects prevents them from creating job objects of their own dotnet/sdk#10150

Closed

KalleOlaviNiemitalo mentioned this pull request Jun 20, 2023

Regression - ctrl-c handling commit from old dotnet/cli repo not merged into this repo dotnet/sdk#33417

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement child process reaping. #10720

Implement child process reaping. #10720

peterhuene commented Feb 7, 2019

peterhuene commented Feb 7, 2019 •

edited

Loading

nguerrera commented Feb 7, 2019

nguerrera commented Feb 7, 2019

peterhuene commented Feb 7, 2019

peterhuene commented Feb 7, 2019

peterhuene commented Feb 7, 2019

rainersigwald left a comment

peterhuene commented Feb 7, 2019

peterhuene commented Feb 7, 2019 •

edited

Loading

danmoseley commented Feb 9, 2019

tmds Feb 12, 2019

peterhuene Feb 12, 2019 •

edited

Loading

tmds Feb 12, 2019

peterhuene Feb 12, 2019

tmds Feb 12, 2019

peterhuene Feb 13, 2019 •

edited

Loading

tmds Feb 14, 2019

peterhuene Feb 14, 2019 •

edited

Loading

tmds Feb 15, 2019

nguerrera Feb 15, 2019 •

edited

Loading

tmds Feb 12, 2019

peterhuene Feb 12, 2019

tmds commented Feb 12, 2019

danmoseley commented Feb 12, 2019

peterhuene commented Feb 12, 2019

steven-solomon commented Apr 27, 2019

peterhuene commented Apr 27, 2019

steven-solomon commented Apr 27, 2019

Implement child process reaping. #10720

Implement child process reaping. #10720

Conversation

peterhuene commented Feb 7, 2019

peterhuene commented Feb 7, 2019 • edited Loading

nguerrera commented Feb 7, 2019

nguerrera commented Feb 7, 2019

peterhuene commented Feb 7, 2019

peterhuene commented Feb 7, 2019

peterhuene commented Feb 7, 2019

rainersigwald left a comment

Choose a reason for hiding this comment

full framework

Successful build (new nodes created)

Successful build (node reuse)

Crash (new nodes)

Crash (node reuse)

With this

Successful build (new nodes created)

Successful build (node reuse)

Crash (new nodes)

Crash (node reuse)

peterhuene commented Feb 7, 2019

peterhuene commented Feb 7, 2019 • edited Loading

danmoseley commented Feb 9, 2019

Choose a reason for hiding this comment

peterhuene Feb 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterhuene Feb 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterhuene Feb 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguerrera Feb 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmds commented Feb 12, 2019

danmoseley commented Feb 12, 2019

peterhuene commented Feb 12, 2019

steven-solomon commented Apr 27, 2019

peterhuene commented Apr 27, 2019

steven-solomon commented Apr 27, 2019

peterhuene commented Feb 7, 2019 •

edited

Loading

peterhuene commented Feb 7, 2019 •

edited

Loading

peterhuene Feb 12, 2019 •

edited

Loading

peterhuene Feb 13, 2019 •

edited

Loading

peterhuene Feb 14, 2019 •

edited

Loading

nguerrera Feb 15, 2019 •

edited

Loading