Runtime: Stream.Flush docs should be explicit about not throwing NotImplementedException even if CanWrite == false

[pjanotti]

This is related to issue dotnet/runtime#21450 in which a read-only stream was throwing NotImplementedException on Flush. Per discussion on the thread of associated issue streams should not do that, however, documents are not explicit about it.

[karelz]

@JeremyKuhne @ianhays does it sound reasonable to you?

If yes, we would need wording suggestion and list of changed docs, then move it to docs repo.

[JeremyKuhne]

Makes sense to me.

[ianhays]

Yeah, I don't like NotImplemenedException in this scenario. NotSupportedException would be fine and that should be up to the designer, but NotImplementedException is pretty unhelpful.

[pjanotti]

@ianhays I don't want to drag on a small issue like this but I can't resist :-D my feeling is that Flush only should throw in case of an actual error while doing some state "flushing", otherwise it should simply be an empty method. FlushAsync is already like that. It avoids the case reported by user on first issue.

[ianhays]

I don't want to drag on a small issue like this but I can't resist :-D

By all means, drag it on :) The more discussion, the better (usually).

In all the streams I've written, I've followed the pattern that Flush should be a no-op on ReadOnly streams, with the exception that sometimes it also does some state validation e.g. checking that the stream is open. I believe that to be a best practice that maximizes the interchangeability of the Stream.

However, I'm hesitant to make a blanket statement that Flush should never throw for a ReadOnly stream, because there may be scenarios in which a NotSupportedException makes sense. I think it would really just depend on the exact wording that we use in the amendment.

[pjanotti]

Agreed: using some wording that makes clear that some types taking a Stream may call Flush even if the Stream is ReadOnly will make clear the consequences of blanketing throwing exceptions in Flush: your ReadOnly stream will not work with them.

[ianhays]

some wording that makes clear that some types taking a Stream may call Flush even if the Stream is ReadOnly

Exactly what I had in mind. We are in agreement then. All that's left now is:

we would need wording suggestion and list of changed docs

[pjanotti]

Here is my starter proposal:

At https://docs.microsoft.com/en-us/dotnet/api/system.io.stream.canwrite?view=netframework-4.7

Add one sentence to the existing paragraph so in the end it will read:

If a class derived from Stream does not support writing, a call to Write, BeginWrite, or WriteByte throws a NotSupportedException. In such cases Flush is typically implemented as an empty method to ensure full compatibility with other Stream types since it is valid to flush a read-only stream.

At https://docs.microsoft.com/en-us/dotnet/api/system.io.stream.flush?view=netframework-4.7

Add the paragraph below right after the first paragraph (under remarks):

In a class derived from Stream that does not support writing, Flush is typically implemented as an empty method to ensure full compatibility with other Stream types since it is valid to flush a read-only stream.

[pjanotti]

@karelz @ianhays last week I assumed that you guys were at \BUILD... perhaps now it is a good time to get back to this :-)

[karelz]

@ianhays is OOF sick today. @JeremyKuhne can you please help review the docs change? We can then move it into docs repo. Thanks!

@pjanotti thanks for the ping! Definitely useful ;)

[JeremyKuhne]

I'm OK with the verbiage, not sure on procedure for moving forward.

[karelz]

Either move the bug, or file a new one in dotnet/docs - post the text change there, link this issue as "it was approved by area experts"
@pjanotti do you want to create it?

[pjanotti]

@karelz ok, I will do it (maybe tomorrow)

[pjanotti]

This is now tracked by https://github.com/dotnet/docs/issues/2171