dotnet / dotnet/dotnet-api-docs
Documentation example for `Process` is incorrect/misleading.
- Dominant language
- C#
- Stars
- 949
- Forks
- 1.7k
- Avg merge
- 3d 27m
- Merged PRs (30d)
- 49
Description
### Summary
Per https://github.com/dotnet/runtime/blob/d099f075e45d2aa6007a22b71b45a08758559f80/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs#L893-L919, 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.1 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](https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.process.beginoutputreadline?view=net-7.0) does not mention ownership, and perhaps I'm blind to some .NET's convention, but do look at the example on that very page:
```C#
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.
1 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.
3. Review examples and ensure that proper usage is demonstrated in the examples, and examples be free of resource leaks.
4. 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.
Contributor guide
Assessment
This issue has not been assessed yet.