[Analyzer Proposal]: Warn on use of StreamReader.EndOfStream in async methods

[stephentoub]

StreamReader.EndOfStream is a pernicious little property.
https://learn.microsoft.com/en-us/dotnet/api/system.io.streamreader.endofstream?view=net-8.0

It seems harmless, like it's just checking something quick and easy. And if there's already some data buffered in the StreamReader, it is: if there's remaining data to read, it's not at the end of the stream. But if there's no data buffered, the only way it can know if it's at the end of the stream is to try to read from the stream and wait until either at least a byte of data is available or it gets back eof. That is a synchronous blocking operation.

In the last month, I've seen several uses of EndOfStream along the lines of:

while (!reader.EndOfStream)
{
    string? line = await reader.ReadLineAsync();
    ...
}

This is problematic because while the developer obviously intended to do their I/O asynchronously, there's a really good chance most of it will end up being done synchronously as part of EndOfStream, making the API synchronously instead of asynchronously blocking. Separately, even if this were intended to be entirely synchronous, this doesn't actually need to use EndOfStream: ReadLine{Async} will give back null if at EOF, so that can be used instead of EndOfStream.

While there's rarely a good reason to use EndOfStream, I'm not sure it rises to the level of obsoleting it. But we should have an analyzer that warns on any use of it in an async method, as it's almost always going to be doing the wrong thing there.

[ghost]

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

Issue Details

---

StreamReader.EndOfStream is a pernicious little property.
https://learn.microsoft.com/en-us/dotnet/api/system.io.streamreader.endofstream?view=net-8.0

It seems harmless, like it's just checking something quick and easy. And if there's already some data buffered in the StreamReader, it is: if there's remaining data to read, it's not at the end of the stream. But if there's no data buffered, the only way it can know if it's at the end of the stream is to try to read from the stream and wait until either at least a byte of data is available or it gets back eof. That is a synchronous blocking operation.

In the last month, I've seen several uses of EndOfStream along the lines of:

while (!reader.EndOfStream)
{
    string? line = await reader.ReadLineAsync();
    ...
}

This is problematic because while the developer obviously intended to do their I/O asynchronously, there's a really good chance most of it will end up being done synchronously as part of EndOfStream, making the API synchronously instead of asynchronously blocking. Separately, even if this were intended to be entirely synchronous, this doesn't actually need to use EndOfStream: ReadLine{Async} will give back null if at EOF, so that can be used instead of EndOfStream.

While there's rarely a good reason to use EndOfStream, I'm not sure it rises to the level of obsoleting it. But we should have an analyzer that warns on any use of it in an async method, as it's almost always going to be doing the wrong thing there.

Author:	stephentoub
Assignees:	-
Labels:	area-System.IO, code-analyzer
Milestone:	-

[stephentoub]

Why is EndOfStream bad (ignoring redundancy from null returns)? Sure, that property getter will be synchronous, but then the await will bounce the task to the thread pool.

I don't understand the comment. EndOfStream itself calls a synchronous read method on the stream. By the time it returns, the damage is done.

[stephentoub]

Ah. I was not aware that it would perform a read

Yup, as I wrote above, "But if there's no data buffered, the only way it can know if it's at the end of the stream is to try to read from the stream and wait until either at least a byte of data is available or it gets back eof. That is a synchronous blocking operation."