Avoid throwing an Exception in ToString when a process has already exited #43597

avodovnik · 2020-10-19T18:28:27Z

In cases where a process is killed, and ToString is called after that, an exception is thrown. This was due to the use of ProcessName which calls EnsureState - which in turn throws an exception. Instead, we now do a check for the state and fallback to the original base.ToString.

Fixes #1520

ghost · 2020-10-19T18:28:33Z

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

danmoseley

This seems right. HasExited may end up calling EnsureState(State.Associated), but that does no real work; it may also end up calling UpdateHasExited() and from that point it's pretty hard to reason about whether that might cause a problem, but given the name of the method, it's reasonable to assume it's hardened against the process having exited 😊

danmoseley · 2020-10-19T18:49:03Z

@dotnet/runtime-infrastructure possibly transient error in dotnet-install on two OSX legs.

2020-10-19T18:33:27.7004190Z dotnet-install: Downloading link: https://dotnetcli.azureedge.net/dotnet/Sdk/5.0.100-rc.2.20479.15/dotnet-sdk-5.0.100-rc.2.20479.15-osx-x64.tar.gz
2020-10-19T18:33:27.7135770Z curl: (7) Failed to connect to dotnetcli.azureedge.net port 443: Connection refused
2020-10-19T18:33:27.7143010Z dotnet-install: Cannot download: https://dotnetcli.azureedge.net/dotnet/Sdk/5.0.100-rc.2.20479.15/dotnet-sdk-5.0.100-rc.2.20479.15-osx-x64.tar.gz

(curl works for me)

I"ll rerun.

jaredpar · 2020-10-19T18:59:03Z

@danmosemsft looks like there is a big spike in SDK install failures in last 24 hours: 49 total across the dotnet org.

https://runfo.azurewebsites.net/search/timelines/?bq=started%3A%7E1&tq=Failed+to+install+dotnet+SDK+from+public+location

stephentoub · 2020-10-20T02:24:25Z

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

@@ -1343,7 +1343,7 @@ private void StopWatchingForExit()

        public override string ToString()
        {
-            if (Associated)
+            if (Associated && !HasExited)


What happens if the process exits after checking HasExited and before accessing ProcessName?

Good point. Catch?

That's a valid question! Is it worth just wrapping this in a try/catch then, or is that unlikely enough to not be a concern?

Where is the problematic exception coming from? i.e. what's the stack of the exception that's thrown if the process has exited?

@stephentoub it's coming from the call to ProcessName which then calls EnsureState ( https://github.com/dotnet/runtime/blob/098fdc5fe634067283369bbb51e7c439431e4105/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs#L512) - the state flag it uses means it'll call EnsureState again, this time checking that it has a non-exited process ID (https://github.com/dotnet/runtime/blob/098fdc5fe634067283369bbb51e7c439431e4105/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs#L951). That's where the exception gets thrown.

That's where the exception gets thrown.

Hmm, are you sure? It's passing in State.HaveProcessInfo, which doesn't include the bits for HaveNonExitedId, so I would not expect it to be making a call to EnsureState(State.HaveNonExitedId). Rather, I'd expect the exception is coming from here:

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

Lines 953 to 957 in 21121c3

_processInfo = ProcessManager.GetProcessInfo(_processId, _machineName);

if (_processInfo == null)

{

throw new InvalidOperationException(SR.NoProcessInfo);

}

with GetProcessInfo returning null and the call then explicitly throwing as a result (the error string thrown also matches the text I see in the exception.

Assuming that's true, then I think I'd prefer to see ToString just rewritten to not use ProcessName at all and avoid the throwing altogether:

public override string ToString() { string result = base.ToString(); if (Associated) { _processInfo ??= ProcessManager.GetProcessInfo(_processId, _machineName); if (_processInfo is not null) { string processName = _processInfo.ProcessName; if (processName.Length != 0) { result = $"{result} ({processName})"; } } } return result; }

@iSazonov I think, considering I need the base.ToString() in both results, it's fine if I call it in the beginning.

@stephentoub I feel a little bit "bad" setting _processInfo directly from a ToString() method, though. My thinking was the !HasExisted check would be good enough, since it then looks at if ((state & State.HaveProcessInfo) != (State)0). That way the processInfo gets set in the same place you'd expect it to.

Happy to go either way, though.

But yeah, if it exists immediately after that check, maybe it makes sense to do a try/catch.

I've gone ahead with the changes, and rebased while I'm at it.

In cases where a process is killed, and ToString is called after that, an exception is thrown. This was due to the use of ProcessName which calls EnsureState - which in turn throws an exception. Instead, we now do a check for the state and fallback to the original ToString. Fix dotnet#1520

danmoseley · 2020-11-23T23:39:37Z

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

+        [Fact]        
+        public void TestToString_OnExitedProcess()
+        {
+            var p = Process.Start("dotnet");


I am not sure we can be certain that "dotnet" is on the path for everyone running this test. I would follow the pattern of other tests, which is to use Process process = CreateProcessLong();. (This just makes a sleeping process) something like

var p = CreateProcessLong(); var name = p.ProcessName; Assert.Equal($"System.Diagnostics.Process ({name})", p.ToString()); p.Kill(); Assert.Equal("System.Diagnostics.Process", p.ToString());

?

otherwise looks good to me. thanks!

danmoseley · 2020-11-30T01:33:36Z

rerunning CI

ghost · 2020-11-30T01:33:46Z

Hello @danmosemsft!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

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

stephentoub · 2020-11-30T23:18:02Z

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

+            {
+                // Handle cases where a process would exit straight after our checks
+                // and/or make ProcessManager throw an exception.
+            }


Is this try/catch still needed? What might throw here if the process exits?

@stephentoub ProcessManager.GetProcessInfo can still throw an exception, IIRC - that's why I left the catch block there

am11 · 2021-01-07T18:35:09Z

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

@@ -460,6 +460,21 @@ public void TestHasExited()
            }
        }

+        [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
+        public void TestToString_OnExitedProcess()


Just curious, does this test fail on master branch? With .NET5, this program:

using System; using System.Diagnostics; class Program { static void Main() { Process p = Process.Start("bash", "-c \"sleep 100000\""); Console.WriteLine(p.ProcessName); p.Kill(); Console.WriteLine(p.ToString()); } }

displays:

bash System.Diagnostics.Process (bash)

on Linux and macOS, with return code 0.

@am11 sorry for the delay - yes, the above looks right, in the test we do:

Assert.True(p.WaitForExit(WaitInMS));

In your example, the behaviour is expected, because when you call .ToString() the process is actually still there and the runtime is able to get the information - hence no exception and the information in the output.

@avodovnik, ah I missed the WaitForExit. With Process.Start("bash", "--help"); and WaitForExit(10_000), it gives:

Unhandled exception. System.InvalidOperationException: Process has exited, so the requested information is not available. at System.Diagnostics.Process.EnsureState(State state) at System.Diagnostics.Process.get_ProcessName()

however, with subprocess Process.Start("bash", "-c \"sleep 1\"") and WaitForExit(10_000), it strangely returns subprocess name instead:

sleep System.Diagnostics.Process (sleep)

(and without WaitForExit, it returns bash in place of sleep as we've seen above)
does this PR change/improve this subprocess behavior?

Yes, the PR follows our own guidelines on not throwing exceptions in ToString and instead falls back to the default value without displaying the process name. As for the sleep case, the returned name is a result of whatever the underlying process manager returns for that PID, and I believe sleep is correct - it is, in fact, the current process running under that id.

danmoseley · 2021-01-27T04:37:33Z

Hello @avodovnik have you been able to look at the last comments here? would be great to get this done and merged.

carlossanlop · 2021-03-15T18:55:50Z

Ping @avodovnik , this has been stale for a couple of months. Any chance you can take a look at the latest feedback?

avodovnik · 2021-03-15T19:04:56Z

Ping @avodovnik , this has been stale for a couple of months. Any chance you can take a look at the latest feedback?

Yeah, I will try getting back to it tomorrow or day after!

carlossanlop

LGTM as it is.
Thanks for making this change, @avodovnik, and thanks @am11 for your review too.

carlossanlop

@avodovnik actually, the CI is old and the results are gone already. Can you please rebase your PR to the latest bits in main so we can ensure the unit tests are passing?

danmoseley · 2021-03-18T19:24:26Z

As far as I know, close/reopen does this for you..

avodovnik · 2021-03-18T21:44:20Z

I spent most of this morning trying to figure out if my rebase broke the build or it was already broken in main :D if CI does it automatically, that'd be great, but otherwise I'll see if I can make more progress tomorrow!

safern · 2021-03-18T22:06:38Z

if CI does it automatically, that'd be great, but otherwise I'll see if I can make more progress tomorrow!

CI should automatically merge with main for PR builds.

carlossanlop

@avodovnik the CI finished. The failures are caused by the new unit test, and seems to be affecting all platforms. Some examples:

Libraries Test Run release coreclr Linux x64 Debug

https://helix.dot.net/api/2019-06-17/jobs/a37efb42-7431-49de-8228-55e52170dc95/workitems/System.Diagnostics.Process.Tests/console

Discovering: System.Diagnostics.Process.Tests (method display = ClassAndMethod, method display options = None)
Discovered:  System.Diagnostics.Process.Tests (found 253 of 311 test cases)
Starting:    System.Diagnostics.Process.Tests (parallel test collections = on, max threads = 2)
  System.Diagnostics.Tests.ProcessStartInfoTests.ShellExecute_Nano_Fails_Start [SKIP]
    Condition(s) not met: "IsWindowsNanoServer"
Linux
  System.Diagnostics.Tests.ProcessTests.TestToString_OnExitedProcess [FAIL]
    Assert.Equal() Failure
                                     â†“ (pos 26)
    Expected: Â·Â·Â·.Diagnostics.Process
    Actual:   Â·Â·Â·.Diagnostics.Process (dotnet)
                                     â†‘ (pos 26)
    Stack Trace:
      /_/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs(477,0): at System.Diagnostics.Tests.ProcessTests.TestToString_OnExitedProcess()

Libraries Test Run release coreclr windows x86 Release

https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-43597-merge-0a57a815b1364942a1/System.Diagnostics.Process.Tests/console.db579eca.log?sv=2019-07-07&se=2021-04-07T21%3A51%3A29Z&sr=c&sp=rl&sig=hAYSM1qPcRCeRZJ0XAHLK%2FSjmOlZxnb0BAOA9pRX608%3D

Discovering: System.Diagnostics.Process.Tests (method display = ClassAndMethod, method display options = None)
Discovered:  System.Diagnostics.Process.Tests (found 253 of 277 test cases)
Starting:    System.Diagnostics.Process.Tests (parallel test collections = on, max threads = 2)
  System.Diagnostics.Tests.ProcessStartInfoTests.ShellExecute_Nano_Fails_Start [SKIP]
    Condition(s) not met: "IsWindowsNanoServer"
  System.Diagnostics.Tests.ProcessTests.TestToString_OnExitedProcess [FAIL]
    Assert.Equal() Failure
                                     � (pos 26)
    Expected: úúú.Diagnostics.Process
    Actual:   úúú.Diagnostics.Process (dotnet)
                                     � (pos 26)
    Stack Trace:
      /_/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs(477,0): at System.Diagnostics.Tests.ProcessTests.TestToString_OnExitedProcess()

Libraries Test Run release mono OSX x64 Debug

https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-43597-merge-f78bf3b5a1d547bea0/System.Diagnostics.Process.Tests/console.84799612.log?sv=2019-07-07&se=2021-04-08T00%3A00%3A39Z&sr=c&sp=rl&sig=QB4lWk99C3%2Fsrc4%2F0o4mGJs72xq0e8kfai8tVkI5mC0%3D

Discovering: System.Diagnostics.Process.Tests (method display = ClassAndMethod, method display options = None)
Discovered:  System.Diagnostics.Process.Tests (found 238 of 311 test cases)
Starting:    System.Diagnostics.Process.Tests (parallel test collections = on, max threads = 12)
  System.Diagnostics.Tests.ProcessStartInfoTests.ShellExecute_Nano_Fails_Start [SKIP]
    Condition(s) not met: "IsWindowsNanoServer"
  System.Diagnostics.Tests.ProcessTests.TestToString_OnExitedProcess [FAIL]
    Assert.Equal() Failure
                                     â†“ (pos 26)
    Expected: Â·Â·Â·.Diagnostics.Process
    Actual:   Â·Â·Â·.Diagnostics.Process (dotnet)
                                     â†‘ (pos 26)
    Stack Trace:
      /_/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs(477,0): at System.Diagnostics.Tests.ProcessTests.TestToString_OnExitedProcess()
      /_/src/mono/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.cs(378,0): at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
Darwin
  System.Diagnostics.Tests.ProcessTests.TestProcessRecycledPid [SKIP]
    Condition(s) not met: "IsStressModeEnabledAndRemoteExecutorSupported"
  System.Diagnostics.Tests.ProcessTests.CanBeFinalized [SKIP]
    Condition(s) not met: "IsPreciseGcSupported"
Finished:    System.Diagnostics.Process.Tests

jeffhandley · 2021-04-18T06:46:56Z

The failures are caused by the new unit test, and seems to be affecting all platforms.

@avodovnik Let us know if you need any help debugging the test failures. Thanks!

jeffhandley · 2021-05-09T05:22:30Z

@avodovnik There is now a merge conflict too. I'm going to mark this PR as a Draft for the time-being, but please let us know if you want some help getting this over the finish line. Thanks!

ghost · 2021-06-08T10:00:13Z

Draft Pull Request was automatically closed for inactivity. Please let us know if you'd like to reopen it.

Dotnet-GitSync-Bot added the area-System.Diagnostics.Process label Oct 19, 2020

danmoseley approved these changes Oct 19, 2020

View reviewed changes

runfoapp bot mentioned this pull request Oct 20, 2020

Failure to install dotnet SDK during build #43626

Closed

stephentoub reviewed Oct 20, 2020

View reviewed changes

adamsitnik added this to the 6.0.0 milestone Oct 20, 2020

avodovnik added 2 commits November 23, 2020 10:51

Changes as per PR Comments.

7857375

avodovnik force-pushed the dev/anvod/tostringfix branch from 098fdc5 to 7857375 Compare November 23, 2020 15:04

avodovnik requested a review from danmoseley November 23, 2020 15:06

danmoseley reviewed Nov 23, 2020

View reviewed changes

PR Suggestion to improve testing.

9dba39a

danmoseley closed this Nov 30, 2020

danmoseley reopened this Nov 30, 2020

danmoseley added the auto-merge label Nov 30, 2020

danmoseley reviewed Nov 30, 2020

View reviewed changes

src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs Outdated Show resolved Hide resolved

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

b535d34

danmoseley added auto-merge and removed auto-merge labels Nov 30, 2020

Update ProcessTests.cs

661b963

stephentoub reviewed Nov 30, 2020

View reviewed changes

am11 reviewed Jan 7, 2021

View reviewed changes

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

stephentoub removed the auto-merge label Mar 6, 2021

carlossanlop approved these changes Mar 18, 2021

View reviewed changes

carlossanlop reviewed Mar 18, 2021

View reviewed changes

danmoseley closed this Mar 18, 2021

danmoseley reopened this Mar 18, 2021

carlossanlop requested changes Mar 19, 2021

View reviewed changes

jeffhandley marked this pull request as draft May 9, 2021 05:22

ghost closed this Jun 8, 2021

iSazonov mentioned this pull request Jun 10, 2021

Avoid throwing in ToString() if the process has already exited #53979

Merged

ghost locked as resolved and limited conversation to collaborators Jul 8, 2021

This pull request was closed.

	_processInfo = ProcessManager.GetProcessInfo(_processId, _machineName);
	if (_processInfo == null)
	{
	throw new InvalidOperationException(SR.NoProcessInfo);
	}

Avoid throwing an Exception in ToString when a process has already exited #43597

Avoid throwing an Exception in ToString when a process has already exited #43597

Conversation

avodovnik commented Oct 19, 2020

ghost commented Oct 19, 2020

danmoseley left a comment

Choose a reason for hiding this comment

danmoseley commented Oct 19, 2020

jaredpar commented Oct 19, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmoseley Nov 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmoseley commented Nov 30, 2020

ghost commented Nov 30, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

am11 Jan 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

am11 Mar 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmoseley commented Jan 27, 2021

carlossanlop commented Mar 15, 2021

avodovnik commented Mar 15, 2021

carlossanlop left a comment

Choose a reason for hiding this comment

carlossanlop left a comment

Choose a reason for hiding this comment

danmoseley commented Mar 18, 2021

avodovnik commented Mar 18, 2021

safern commented Mar 18, 2021

carlossanlop left a comment

Choose a reason for hiding this comment

jeffhandley commented Apr 18, 2021

jeffhandley commented May 9, 2021

ghost commented Jun 8, 2021

danmoseley Nov 23, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

am11 Jan 7, 2021 •

edited

Loading

am11 Mar 18, 2021 •

edited

Loading