Documentation example for `Process` is incorrect/misleading.

[LBensman]

Summary

Per

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

Lines 893 to 919 in d099f07

	// Only call close on the streams if the user cannot have a reference on them.
	// If they are referenced it is the user's responsibility to dispose of them.
	try
	{
	if (_standardOutput != null && (_outputStreamReadMode == StreamReadMode.AsyncMode || _outputStreamReadMode == StreamReadMode.Undefined))
	{
	if (_outputStreamReadMode == StreamReadMode.AsyncMode)
	{
	_output?.CancelOperation();
	_output?.Dispose();
	}
	_standardOutput.Close();
	}
	if (_standardError != null && (_errorStreamReadMode == StreamReadMode.AsyncMode || _errorStreamReadMode == StreamReadMode.Undefined))
	{
	if (_errorStreamReadMode == StreamReadMode.AsyncMode)
	{
	_error?.CancelOperation();
	_error?.Dispose();
	}
	_standardError.Close();
	}
	if (_standardInput != null && !_standardInputAccessed)
	{
	_standardInput.Close();

, the closure and disposal of StandardInput, StandardOutput, and StandardError stream is not straight forward, and ownership of said streams depends on the usage of the said streams.

Documentation, however, does not reflect this nuanced complexity, leaving the consumer in the dark: the examples of usage appear to be incorrect, leading to potential leak and exhaustion of resources.

Specifics

Per referenced code, including the code comment, it appears that ownership of standard streams depends on if and how such streams were used. If I read the code comment and referenced code snippet correctly, Process instance owns the standard streams, until and unless those streams were used in non-async operation.¹ So if .StandardOutput property is referenced, the caller takes ownership away from Process and assumes the ownership going forward. If stream is consumed via BeginOutputReadLines(), then Process retains ownership of stream and responsible for its disposal.

The documentation, however, does not reflect this information. Process.StandardOutput documentation does not mention ownership, and perhaps I'm blind to some .NET's convention, but do look at the example on that very page:

using System;
using System.IO;
using System.Diagnostics;

class StandardOutputExample
{
    public static void Main()
    {
        using (Process process = new Process())
        {
            process.StartInfo.FileName = "ipconfig.exe";
            process.StartInfo.UseShellExecute = false;
            process.StartInfo.RedirectStandardOutput = true;
            process.Start();

            // Synchronously read the standard output of the spawned process.
            StreamReader reader = process.StandardOutput;
            string output = reader.ReadToEnd();

            // Write the redirected output to this application's window.
            Console.WriteLine(output);

            process.WaitForExit();
        }

        Console.WriteLine("\n\nPress any key to exit.");
        Console.ReadLine();
    }
}

The line StreamReader reader = process.StandardOutput; references the stream, thus taking ownership. Yet reader is not used in using auto-disposing construct, nor does example have a call to reader.Close() to release the reader.

Thus, it appears that the example in the documentation falls victim to this ambiguity and the example actually has a resource leak due to failure to dispose reader and stream.

Same for Error and Input equivalents.

¹ It's confusing a bit with modern TPL-based async usage -- async here appears to imply BeginOutputReadLines() and similar, and not Stream.ReadAsync() that most would otherwise understand as asynchronous.

Summary of issue and Recommendations

I can think of few considerations here:

1. Amend documentation to explicitly clarify ownership rules to reflect code behavior in all relevant documentation pages.
   a. Clarify meaning of asynchronous to be less confusing w.r.t. modern async with TPL -- that TPL async in this context is a synchronous mode access.
2. Review examples and ensure that proper usage is demonstrated in the examples, and examples be free of resource leaks.
3. Reconsider this ownership ambiguity and make Process own all of its related resources. It seems that the streams (and their readers) are intrinsically linked to Process, and once Process is closed and disposed of, the streams are no longer meaningfull. It would make sense that Process disposal implies standard streams disposal as single coherent operation. I'm aware that this proposed change may violate existing contracts and may not be feasible (though, as mentioned, if streams are no longer meaningful, it doesn't seem like it can violate and otherwise valid use case, but I can't be certain here, so leave it up to you to evaluate this).

Scope

I've reviewed code and documentation for releases 7, 8, and 9. I didn't review others, but suspect those are affected as well.

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-diagnostics-process
See info in area-owners.md if you want to be subscribed.

[jozkee]

Its very likely number 3 will break something, although, I can't think of a case where you want to keep holding StandardOutput or StandardError after you are done with the Process.

@LBensman do you want to go further and edit the documentation? Adding a paragraph in Remarks should be good to clarify this nuance.