Consider removing Stream's semaphore

[stephentoub]

The sole field remaining in the base Stream class is a semaphore used to serlalize any Read/WriteAsync calls issued to run concurrently on the instance. This behavior is an holdover from the original behavior similarly employed by BeginRead/Write that dates back to their original introduction.

On the one hand, it's dubious to use such APIs on Stream concurrently when the Stream doesn't guarantee this behavior is supported, and it'd be nice to be able to remove the semaphore, not only to reduce the size of Stream but also to reduce the complexity of the code that uses it (BufferedStream also relies on this, and some other streams have their own variant of this, e.g. PipeStream). It's also problematic for Streams that do support concurrent reads and writes but that don't override the async methods, as in #36481, though the right solution there is to override and not just rely on the default async-over-sync implementations.)

On the other hand, some streams do explicitly support reading/writing concurrently, e.g. NetworkStream, SslStream, etc., and so it's reasonable someone might assume all streams could be used in this fashion, and this semaphore protects against that. In fact, some streams like FileStream have multiple modes, one of which does support concurrent operations, and the base implementation which is then inherited into the other mode provides some consistency/protection. It's also possible/likely someone somewhere is relying on the serialization this provides, e.g. calling WriteAsync multiple times without waiting on the previous one to complete, knowing the base implementation will handle queueing the operations for them.

We should consider removing the semaphore behavior, at least by default, potentially with a quirk to put it back, at least for a release or two. We could keep the base field to support such a quirk, or alternatively to avoid the extra state in every Stream instance, employ a ConditionalWeakTable to only utilize the semaphore functionality if it's really needed.

[geoffkizer]

It never really makes sense to try to do multiple Reads or Writes on a Stream concurrently.

Even the places you cite where we do support this are questionable, in my opinion. We are just blocking at some lower layer in order to guarantee some sort of arbitrary ordering between the concurrent operations. And even then, it's not as reliable as advertised -- see e.g. #44422.

I vote to get rid of this.

[stephentoub]

Even the places you cite where we do support this are questionable, in my opinion.

Sure, we would never add this if we were starting over today. It's a question of compat.

[stephentoub]

@jkotas, do you have any opinions on this? There's an obvious compat risk associated with it, but it's also generally unnecessary and surprising baggage we're carrying around.

[jkotas]

I vote to get rid of this.

+1

[stephentoub]

I've been waffling back and forth between whether to have a quirk for this or not. On the one hand, I'd really like to be able to delete all this cruft and shrink the size of Stream by removing the field. On the other hand, I do have nagging concerns about the compatibility impact, and whether for .NET 6 we should have an opt-in for the serialization and then in .NET 7 rip it all out. Opinions? It's also possible I'm making a mountain out of a mole hill and no one will miss the (poor) behavior.

[jkotas]

My default recomendation is to be reactive with adding quirks, not proactive. We can even add the quirk in servicing if we need to.

[stephentoub]

Ok. I'll be bold ;-)

[davidfowl]

Delete the locks !