CryptoStream.Dispose calls Flush on read-only Stream in CryptoStreamMode.Read

[kekekeks]

class ReadOnlyStream : Stream
{
    public override void Flush() => throw new NotSupportedException();

    public override int Read(byte[] buffer, int offset, int count) => count;

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

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

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

    public override bool CanRead => true;

    public override bool CanSeek => false;

    public override bool CanWrite => false;

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

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

static void Main(string[] args)
{
    var aes = Aes.Create().CreateDecryptor();
    var crypto = new CryptoStream(new ReadOnlyStream(), aes, CryptoStreamMode.Read, false);
    crypto.Dispose();
}

I'm not sure if that's the intended behavior, but it makes no logical sense to me and also prevents me from reading encrypted payload from http requests in ASP.NET Core.

[bartonjs]

As discussed in the PR: FileStream, and perhaps other streams, have observable effects from calling Flush in a read-only mode; so this is a breaking change. The recommendation is to change the input streams to not throw on Flush, or to wrap them in a delegating stream class which just implements Flush as a no-op.

[jnm2]

@bartonjs Wait a minute, this is bad news for me. I've always implemented Flush for read-only streams by throwing NotSupportedException. Flush sounds like an inherently-mutating operation, not a universal semantic one like Dispose or Close. I'm curious for a reasonable example of a read-only stream which requires a Flush call.

If this is truly a misconception, it's probably a common enough misconception to call it out loudly at https://msdn.microsoft.com/library/system.io.stream.flush.aspx.

[kekekeks]

According to https://github.com/dotnet/corefx/blob/master/src/Common/src/CoreLib/System/IO/FileStream.cs#L725 it only changes its state if read-only stream can seek.

FlushInternalBuffer is the only thing called from Flush if CanWrite == false: https://github.com/dotnet/corefx/blob/master/src/Common/src/CoreLib/System/IO/FileStream.cs#L508

[jnm2]

My objection is not that Flush on a read-only stream can't do anything, but rather that you as the client of the read-only stream shouldn't be required or expected to call it. (CryptoStream is the client of the inner stream in this case.)

[jnm2]

In the silence of the current documentation, here's the common wisdom being passed around.

If it's actually true that Flush is expected to be called before Dispose, the status quo seems bad enough that the SDK should include an analyzer that warns you not to throw NotSupportedException when you implement Flush.

[bartonjs]

My comment is that CryptoStream has had this behavior for at least 12 years (I checked 2.0, I assume it had it since 1.1, making it almost 15 years), and that at least one Stream type (FileStream) has an observable effect from calling Flush. Therefore it is probable that someone has a dependency on Flush being called from CryptoStream on a custom stream type (and no one has a dependency on Flush not being called by CryptoStream).

If a new type were being built that had CryptoStream-like behaviors I wouldn't argue that it "should" call Flush (assuming it didn't write things). Merely that since it has always done so it needs to continue to do so, because debugging a side-effect having gone wrong is much harder to deal with than "I changed my code and I got an exception"