Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 5 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6c65eb7
Avoid throwing an Exception in ToString
avodovnik Oct 19, 2020
7857375
Changes as per PR Comments.
avodovnik Nov 23, 2020
9dba39a
PR Suggestion to improve testing.
avodovnik Nov 26, 2020
b535d34
Update src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs
danmoseley Nov 30, 2020
661b963
Update ProcessTests.cs
danmoseley Nov 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 20 additions & 5 deletions src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1343,15 +1343,30 @@ private void StopWatchingForExit()

public override string ToString()
{
if (Associated)
string result = base.ToString();

try
{
string processName = ProcessName;
if (processName.Length != 0)
if (Associated)
{
return string.Format(CultureInfo.CurrentCulture, "{0} ({1})", base.ToString(), processName);
_processInfo ??= ProcessManager.GetProcessInfo(_processId, _machineName);
if (_processInfo is not null)
{
string processName = _processInfo.ProcessName;
if (processName.Length != 0)
{
result = $"{result} ({processName})";
}
}
}
}
return base.ToString();
catch
{
// Handle cases where a process would exit straight after our checks
// and/or make ProcessManager throw an exception.
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


return result;
}

/// <devdoc>
Expand Down
15 changes: 15 additions & 0 deletions src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,6 +460,21 @@ public void TestHasExited()
}
}

[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
public void TestToString_OnExitedProcess()
Copy link
Member

@am11 am11 Jan 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

@am11 am11 Mar 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

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

// Ensure ToString does not throw an exception, but still returns
// a representation of the object.
Assert.Equal("System.Diagnostics.Process", p.ToString());
}

[Fact]
public void HasExited_GetNotStarted_ThrowsInvalidOperationException()
{
Expand Down