StreamReader.Peek() on a DeflateStream returns -1 early

[Austin-Lamb]

Description

I have code that has a DeflateStream and uses a StreamReader on it - this code works fine in .NET Framework, .NET Core 3.1, and .NET 5 - but it breaks in .NET 6.

When trying to Peek() on the stream, internally StreamReader reads a block out that ends up being smaller than it requested, I'm pretty sure this is due to this breaking change. After that happens, it sets a flag called "_isBlocked" and starts returning -1 from Peek() which is not expected.

This seems like a bug in StreamReader?

Reproduction Steps

Construct a StreamReader over a DeflateStream
Read for a while, peeking as you go

Expected behavior

Peek() returns -1 only at the end of the stream, as it did before

Actual behavior

Peek() returns -1 early in the middle of the file.

Regression?

Yes, regressed in .NET 6

Known Workarounds

Can use StreamReader.EndOfStream instead of looking for -1 from Peek(), but this was still a difficult to hunt down bug on upgrading the .NET version of this library.

Configuration

No response

Other information

No response

[ghost]

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

Issue Details

---

Description

I have code that has a DeflateStream and uses a StreamReader on it - this code works fine in .NET Framework, .NET Core 3.1, and .NET 5 - but it breaks in .NET 6.

When trying to Peek() on the stream, internally StreamReader reads a block out that ends up being smaller than it requested, I'm pretty sure this is due to this breaking change. After that happens, it sets a flag called "_isBlocked" and starts returning -1 from Peek() which is not expected.

This seems like a bug in StreamReader?

Reproduction Steps

Construct a StreamReader over a DeflateStream
Read for a while, peeking as you go

Expected behavior

Peek() returns -1 only at the end of the stream, as it did before

Actual behavior

Peek() returns -1 early in the middle of the file.

Regression?

Yes, regressed in .NET 6

Known Workarounds

Can use StreamReader.EndOfStream instead of looking for -1 from Peek(), but this was still a difficult to hunt down bug on upgrading the .NET version of this library.

Configuration

No response

Other information

No response

Author:	Austin-Lamb
Assignees:	-
Labels:	area-System.IO
Milestone:	-

[stephentoub]

Can you please share a small, isolated repro?

[aromaa]

Sounds like this could be #61325 (comment).

[ghost]

This issue has been marked needs-author-action and may be missing some important information.

[Austin-Lamb]

This comes from a large project, I will see if I can extract a minimal repro - it may take me a couple days.

[Austin-Lamb]

@stephentoub, I have made a really small repro. Take this code:

using System;
using System.IO;
using System.IO.Compression;
using System.Net;
using System.Runtime.InteropServices;

Console.WriteLine($"Running in {RuntimeInformation.FrameworkDescription}");

var localPath = Path.GetTempFileName();
new WebClient().DownloadFile(@"https://github.com/dotnet/runtime/archive/refs/tags/v6.0.5.zip", localPath);

var numChars = 0;

using var archive = new ZipArchive(File.OpenRead(localPath), ZipArchiveMode.Read);
foreach (var entry in archive.Entries)
{
    using var sr = new StreamReader(entry.Open());
    
    while (sr.Peek() != -1)
    {
        char next = (char)sr.Read();
        numChars++;
    }
}

Console.WriteLine($"Read {numChars} characters in {archive.Entries.Count} zip entries");

When run on .NET 5 vs. 6, I get different numbers of characters read because Peek() returns -1 "early" sometimes in .NET 6, probably due to the breaking change I mentioned in the original post in this issue. I get this output:

Running in .NET 5.0.17
Read 878322628 characters in 69097 zip entries

vs.

Running in .NET 6.0.5
Read 645700952 characters in 69097 zip entries

[svick]

Here is a self-contained repro of this behavior:

using System;
using System.IO;

using var stream = new AStream();
using var reader = new StreamReader(stream);

// prints "42" once
while (reader.Peek() != -1)
{
    Console.WriteLine(reader.Read());
}

class AStream : Stream
{
    public override int Read(byte[] buffer, int offset, int count)
    {
        buffer[offset] = 42;
        return 1;
    }

    public override bool CanRead => true;

    public override bool CanSeek => true;

    public override bool CanWrite => throw new NotImplementedException();

    public override long Length => throw new NotImplementedException();

    public override long Position { get => throw new NotImplementedException(); set => throw new NotImplementedException(); }

    public override void Flush() => throw new NotImplementedException();

    public override long Seek(long offset, SeekOrigin origin) => throw new NotImplementedException();

    public override void SetLength(long value) => throw new NotImplementedException();

    public override void Write(byte[] buffer, int offset, int count) => throw new NotImplementedException();
}

Note that the documentation of Peek leaves me confused. It says that -1 can also be returned if the stream does not support seeking, but that would make Peek much less useful and it's not what the implementation does.

[stephentoub]

Thanks for the repros. I'll take a look...

[stephentoub]

StreamReader has a private _isBlocked field that it's using to track whether the last read on the underlying Stream returned fewer bytes than requested:

runtime/src/libraries/System.Private.CoreLib/src/System/IO/StreamReader.cs

Lines 58 to 64 in 336a5d2

	// Whether the stream is most likely not going to give us back as much
	// data as we want the next time we call it. We must do the computation
	// before we do any byte order mark handling and save the result. Note
	// that we need this to allow users to handle streams used for an
	// interactive protocol, where they block waiting for the remote end
	// to send a response, like logging in on a Unix machine.
	private bool _isBlocked;

It then uses this field in various ReadXx methods to determine whether to try to read more data from the underlying Stream or to just give back to the caller just the non-zero amount it gathered thus far. I think that's a misguided design but at least defensible. However, it's then also using that _isBlocked in Peek():

runtime/src/libraries/System.Private.CoreLib/src/System/IO/StreamReader.cs

Line 322 in 336a5d2

if (_isBlocked || ReadBuffer() == 0)

which while potentially "by design" is to me a flawed design.

I think the fix here at a minimum is to change the semantics of Peek to stop looking at _isBlocked at all.

@adamsitnik?

[mpervovskiy]

This becomes even worse with Http streams as Peek() is unpredictable there... and looks like this issue dwells in StreamReader for ages.
Repo:

using System;
using System.IO;
using System.Net.Http;

namespace ConsoleApp
{
    class Program
    {
        public static void Main(string[] args)
        {
            var client = new HttpClient();
            var stream = client.GetStreamAsync("https://en.wikipedia.org/wiki/Software_bug").Result;
            var reader = new StreamReader(stream);
            var r = 0;
            var i = -1;
            while (r >= 0)
            {
                i++;
                var b = reader.Peek();
                r = reader.Read();
                if (r != b)
                {
                    Console.WriteLine($"difference {i,6}: {r} {b}");
                }
            }

            Console.WriteLine("done");
        }
    }
}

Differences are happening at different places in the stream