Process.WaitForExit: don't wait for standard streams when process was killed. #48101

tmds · 2021-02-10T12:44:23Z

This is a common pattern:

process.Kill();
process.WaitForExit();

When the process has redirected standard output or standard error
WaitForExit will block until all descendants of the child process (that
inherited these streams) have also terminated.

This change will make WaitForExit no longer wait for these descendants
when the process was killed first.

Additionally, when the user has cancelled the reading by calling
CancelOutputRead and CancelErrorRead, WaitForExit will no longer
wait for the streams either.

@danmoseley @stephentoub @adamsitnik @eiriktsarpalis ptal

… killed. This is a common pattern: process.Kill(); process.WaitForExit(); When the process has redirected standard output or standard error WaitForExit will block until all descendants of the child process (that inherited these streams) have also terminated. This change will make WaitForExit no longer wait for these descendants when the process was killed first. Additionally, when the user has cancelled the reading by calling CancelOutputRead and CancelErrorRead, WaitForExit will no longer wait for the streams either.

ghost · 2021-02-10T12:44:32Z

Tagging subscribers to this area:
See info in area-owners.md if you want to be subscribed.

Issue Details

This is a common pattern:

process.Kill();
process.WaitForExit();

When the process has redirected standard output or standard error
WaitForExit will block until all descendants of the child process (that
inherited these streams) have also terminated.

This change will make WaitForExit no longer wait for these descendants
when the process was killed first.

Additionally, when the user has cancelled the reading by calling
CancelOutputRead and CancelErrorRead, WaitForExit will no longer
wait for the streams either.

@danmoseley @stephentoub @adamsitnik @eiriktsarpalis ptal

Author:	tmds
Assignees:	-
Labels:	`area-System.Diagnostics.Process`
Milestone:	-

stephentoub · 2021-02-10T12:57:58Z

What's the reason for the proposed change?

tmds · 2021-02-10T13:20:55Z

What's the reason for the proposed change?

To better match with expectation that WaitForExit will return timely after a successful Kill.

The current behavior can introduce unexpected long blocking.

Users that are aware call WaitForExit with a timeout. If they pick the timeout that is 'too small', it may return before the process terminates and cause some unexpected behavior that occurs very infrequently and is hard to trace back.

tmds · 2021-02-10T14:37:51Z

The tests aren't passing on Windows.

Assert.Equal() Failure\r\n ↓ (pos 0)\r\nExpected: Sleep child started\r\nActual: The system cannot find the file specified···\r\n ↑ (pos 0)

I think it doesn't find "sleep".

danmoseley · 2021-02-10T15:21:20Z

Is the current behavior the same on both Windows and Unix? What is the .NET Framework behavior? If we are matching that, it seems this change could be significantly breaking.

tmds · 2021-02-10T15:56:02Z

Behavior is the same on Windows and Unix.

The change is meant to be non-breaking:
The WaitUntilEOF in WaitForExit is meant as a way to ensure we've consumed all stdout/stderr from the app. We're using the Kill call from the user as an indication he's no longer interested.
For backwards-compatibility, we're faking a final null DataReceived event.

tmds · 2021-02-10T19:49:16Z

src/libraries/System.Diagnostics.Process/src/System/Diagnostics/AsyncStreamReader.cs

+            if (Interlocked.CompareExchange(ref _operationState, OperationStateReading, state) != state)
+            {
+                return;
+            }


Why does BeginReadLine ever makes calls to FlushMessageQueue?
I think that is ReadBufferAsync's responsibility?

Good question. I looked at the file history but it has only been modified twice: When it was introduced to .NET Core, and then when it was refactored to fix some async cancellation issues.

Maybe it's an optimization because in most cases, EOF is reached quite quickly? What do you think, @jozkee @adamsitnik ?

carlossanlop

Thanks for your PR, @tmds. I left some feedback for you to consider.

@jozkee @adamsitnik let's talk about this PR in our next triage meeting to discuss any potential unintended breaking changes, and if we are ok to take them.

carlossanlop · 2021-03-22T17:52:59Z

src/libraries/System.Diagnostics.Process/src/System/Diagnostics/AsyncStreamReader.cs

+            if (Interlocked.CompareExchange(ref _operationState, OperationStateReading, state) != state)
+            {
+                return;
+            }


Good question. I looked at the file history but it has only been modified twice: When it was introduced to .NET Core, and then when it was refactored to fix some async cancellation issues.

Maybe it's an optimization because in most cases, EOF is reached quite quickly? What do you think, @jozkee @adamsitnik ?

carlossanlop · 2021-03-22T21:09:14Z

src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs

+            };
+            process.BeginOutputReadLine();
+            process.BeginErrorReadLine();
+            string childOutput = await childOutputTcs.Task.TimeoutAfter(30_000);


@tmds the TimeoutAfter extension method has been removed in this PR.

You will have to convert it to a WaitAsync extension method invocation instead.

carlossanlop · 2021-03-22T21:10:42Z

src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs

+
+            Task asyncWaitTask = useAsyncAPI ? process.WaitForExitAsync() :
+                                               Task.Run(() => process.WaitForExit());
+            await asyncWaitTask.TimeoutAfter(30_000);


The sleep child is meant to quit after 10 minutes. Is there a point in waiting 30 seconds? Seems like a lot.

jeffhandley · 2021-05-09T04:03:12Z

@carlossanlop, @jozkee, @adamsitnik -- please take a look at this in your next triage meeting to decide if we should take the behavioral change.

jeffhandley · 2021-05-28T02:01:53Z

@adamsitnik, @carlossanlop, @jozkee -- merge conflicts have emerged on this, but we should decide if we want to take the behavioral change before we both resolving them.

jeffhandley · 2021-07-23T20:53:04Z

@adamsitnik This PR is assigned to you for follow-up/decision before the RC1 snap.

stephentoub · 2021-09-01T13:47:31Z

@jeffhandley, @adamsitnik, what is the plan here? Last comment is from July talking about doing something with this prior to the RC1 snap, which has already happened.

adamsitnik

Hello @tmds

First of all, please accept my apologies for a very big delay.

To be honest with you, I am always hesistant to review PRs that try to change something non-trivial without disucssing it first.
If we discuss things first, and agree that given scenario needs to be fixed, you won't waste your time by working on a fix. As a maintainer I am going to have some context (the review will be easier) and I am also going to have the chance to see what other users think about it (the more upvotes, the higher priority on my TODO list).
If we don't and it turns out that current implementation works as expected, we are both not happy about the outcome: you have spent time on a change that won't get merged, while I have to reject a PR knowing that someone had good intentions and spent some time working on it.

In this particular PR we have two changes:

make WaitForExit no longer wait for these descendants
when the process was killed first

Currently, we wait for these descendants only when the user does not provide any timeout value, meaning that the user can wait up to infinity. I don't believe that this behavior should be changed. It's by design.

runtime/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Windows.cs

Lines 183 to 188 in ef85762

    
           // If we have a hard timeout, we cannot wait for the streams 
        
           if (milliseconds == Timeout.Infinite) 
        
           { 
        
               _output?.EOF.GetAwaiter().GetResult(); 
        
               _error?.EOF.GetAwaiter().GetResult(); 
        
           }

runtime/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs

Lines 213 to 217 in ef85762

    
           if (exited && milliseconds == Timeout.Infinite) // if we have a hard timeout, we cannot wait for the streams 
        
           { 
        
               _output?.EOF.GetAwaiter().GetResult(); 
        
               _error?.EOF.GetAwaiter().GetResult(); 
        
           }

when the user has cancelled the reading by calling
CancelOutputRead and CancelErrorRead

I am suprised that Cancel methods don't actualy cancel anything. It seems that we set the flag to true:

runtime/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/AsyncStreamReader.cs

Lines 83 to 85 in ef85762

    
           internal void CancelOperation() 
        
           { 
        
               _cancelOperation = true;

But don't stop reading:

runtime/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/AsyncStreamReader.cs

Lines 218 to 219 in ef85762

    
           // Keep going until we're out of data to process. 
        
           while (true)

The actual async read operation cancellation is requested by Dispose method:

runtime/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/AsyncStreamReader.cs

Lines 256 to 258 in ef85762

    
           public void Dispose() 
        
           { 
        
               _cts.Cancel();

Would it not be simpler to just actually use the _cancelOperation flag in all while(true) loops and call _cts.Cancel(); from CancelOperation()?

But even with that, I am not sure if the stream used by async reader supports cancellation as on Windows we pass isAsync: false to the FileStream ctor:

runtime/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Windows.cs

Line 639 in ef85762

    
           _standardInput = new StreamWriter(new FileStream(parentInputPipeHandle!, FileAccess.Write, 4096, false), enc, 4096);

May I ask you to: create a new issue that describes the cancellation problem and send a new PR that addresses that? Once we have that PR, I am going to take a look at the Windows part and ensure that we are using a stream that supports cancellation. Once we get there, the issue can be closed (and the fix included in .NET 7).

tmds · 2021-09-09T14:32:48Z

Currently, we wait for these descendants only when the user does not provide any timeout value, meaning that the user can wait up to infinity. I don't believe that this behavior should be changed. It's by design.

By design means this is intentional.
If a user kills a process, why would he then want to wait up to infinity to read its output?

This is how I came to making this PR:

msbuild has some code like this:

process.Kill();
bool exited = process.WaitForExit(timeout);

The timeout is here to avoid the the infinite wait when a grand child still holds the terminal.

The msbuild repo also had a long standing open issue where things went wrong for an unclear reason that was hard to reproduce.

The root cause was a bug in the exited false path, which was very unlikely, but not impossible, to occur.

dotnet-issue-labeler bot added the area-System.Diagnostics.Process label Feb 10, 2021

tmds mentioned this pull request Feb 10, 2021

When sharing the terminal with child nodes, wait for the children to terminate before exiting ourselves. dotnet/msbuild#6053

Merged

Update FlushMessageQueue to handle concurrent flusing

7e7a206

tmds commented Feb 10, 2021

View reviewed changes

runfoapp bot mentioned this pull request Feb 12, 2021

Test Failure : System.Net.Security.Tests.SslStreamNetworkStreamTest.SslStream_ClientCertificate_SendsChain #48091

Closed

Base automatically changed from master to main March 1, 2021 09:07

carlossanlop requested review from jozkee, adamsitnik and carlossanlop March 15, 2021 19:03

carlossanlop reviewed Mar 22, 2021

View reviewed changes

carlossanlop assigned tmds Mar 22, 2021

carlossanlop added this to the Future milestone Mar 22, 2021

jeffhandley assigned carlossanlop May 9, 2021

jeffhandley assigned adamsitnik and unassigned tmds Jul 3, 2021

terrajobst added the community-contribution Indicates that the PR has been added by a community member label Jul 19, 2021

jeffhandley unassigned carlossanlop Jul 23, 2021

adamsitnik reviewed Sep 9, 2021

View reviewed changes

adamsitnik closed this Sep 9, 2021

ghost locked as resolved and limited conversation to collaborators Oct 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Process.WaitForExit: don't wait for standard streams when process was killed. #48101

Process.WaitForExit: don't wait for standard streams when process was killed. #48101

tmds commented Feb 10, 2021

ghost commented Feb 10, 2021

stephentoub commented Feb 10, 2021 •

edited

Loading

tmds commented Feb 10, 2021

tmds commented Feb 10, 2021

danmoseley commented Feb 10, 2021

tmds commented Feb 10, 2021

tmds Feb 10, 2021

carlossanlop Mar 22, 2021

carlossanlop left a comment •

edited

Loading

carlossanlop Mar 22, 2021

carlossanlop Mar 22, 2021

carlossanlop Mar 22, 2021

jeffhandley commented May 9, 2021

jeffhandley commented May 28, 2021

jeffhandley commented Jul 23, 2021

stephentoub commented Sep 1, 2021

adamsitnik left a comment

tmds commented Sep 9, 2021

	// If we have a hard timeout, we cannot wait for the streams
	if (milliseconds == Timeout.Infinite)
	{
	_output?.EOF.GetAwaiter().GetResult();
	_error?.EOF.GetAwaiter().GetResult();
	}

	if (exited && milliseconds == Timeout.Infinite) // if we have a hard timeout, we cannot wait for the streams
	{
	_output?.EOF.GetAwaiter().GetResult();
	_error?.EOF.GetAwaiter().GetResult();
	}

	// Keep going until we're out of data to process.
	while (true)

Process.WaitForExit: don't wait for standard streams when process was killed. #48101

Process.WaitForExit: don't wait for standard streams when process was killed. #48101

Conversation

tmds commented Feb 10, 2021

ghost commented Feb 10, 2021

stephentoub commented Feb 10, 2021 • edited Loading

tmds commented Feb 10, 2021

tmds commented Feb 10, 2021

danmoseley commented Feb 10, 2021

tmds commented Feb 10, 2021

tmds Feb 10, 2021

Choose a reason for hiding this comment

carlossanlop Mar 22, 2021

Choose a reason for hiding this comment

carlossanlop left a comment • edited Loading

Choose a reason for hiding this comment

carlossanlop Mar 22, 2021

Choose a reason for hiding this comment

carlossanlop Mar 22, 2021

Choose a reason for hiding this comment

carlossanlop Mar 22, 2021

Choose a reason for hiding this comment

jeffhandley commented May 9, 2021

jeffhandley commented May 28, 2021

jeffhandley commented Jul 23, 2021

stephentoub commented Sep 1, 2021

adamsitnik left a comment

Choose a reason for hiding this comment

tmds commented Sep 9, 2021

stephentoub commented Feb 10, 2021 •

edited

Loading

carlossanlop left a comment •

edited

Loading