[Analyzer Proposal]: Warn on use of StreamReader.EndOfStream in async methods #98834
Comments
stephentoub
added
area-System.IO
code-analyzer
|
Tagging subscribers to this area: @dotnet/area-system-io Issue DetailsStreamReader.EndOfStream is a pernicious little property. 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
added
the
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. |
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." |
adamsitnik
removed
the
untriaged
stephentoub
added
the
api-ready-for-review
Category: Reliability |
bartonjs
added
api-approved
|
I would like to work on this analyzer if that's okay :) |
|
I'm really looking forward to this feature. My WPF application is hangs because of this problem. I wrote the Chinese blog about this. And the demo code: var fooStream = new FooStream();
var streamReader = new StreamReader(fooStream);
while (!streamReader.EndOfStream)
{
var line = await streamReader.ReadLineAsync();
if (line is null)
{
break;
}
}
class FooStream : Stream
{
public FooStream()
{
_buffer = "123\r\n"u8.ToArray();
}
private readonly byte[] _buffer;
public override void Flush()
{
}
public override int Read(byte[] buffer, int offset, int count)
{
// 模拟卡顿
Thread.Sleep(10000);
if (count >= _buffer.Length)
{
count = _buffer.Length;
Array.Copy(_buffer, 0, buffer, offset, count);
}
return count;
}
public override long Seek(long offset, SeekOrigin origin)
{
return offset;
}
public override void SetLength(long value)
{
}
public override void Write(byte[] buffer, int offset, int count)
{
}
public override bool CanRead => true;
public override bool CanSeek => false;
public override bool CanWrite => false;
public override long Length => long.MaxValue;
public override long Position { get; set; }
}You can find all my demo code in github |
and others
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:
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.
The text was updated successfully, but these errors were encountered: