-
Notifications
You must be signed in to change notification settings - Fork 5.4k
Description
runtime/src/libraries/System.Private.CoreLib/src/System/IO/Stream.cs
Lines 516 to 521 in 5859dad
| // To avoid a race condition with a stream's position pointer & generating conditions | |
| // with internal buffer indexes in our own streams that | |
| // don't natively support async IO operations when there are multiple | |
| // async requests outstanding, we will block the application's main | |
| // thread if it does a second IO request until the first one completes. | |
| SemaphoreSlim semaphore = EnsureAsyncActiveSemaphoreInitialized(); |
I understand the reasoning behind attempting to protect file pointer and buffer indexes from possible simultaneous asynchronous WriteAsync() and ReadAsync() calls (or multiple simultaneous of the same) and thus implicitly to the calling code enforce half-duplex IO. However, further reasoning about this implementation seems to lead to conclusion that a) attempted protection actually fails to achieve its goal; and b) can lead to unexpected deadlocks, especially for streams that do not have position pointer and support full-duplex io.
To the first point - implemented half-duplex failing to protect position pointer - the reasoning is as follows. For streams that have position pointer, such as FileStream, a caller issuing about-simultaneous calls for ReadAsync() and WriteAsync() having some presumed expectation about position pointer has no control over the referenced semaphore, and has no control which asynchronous call will get the semaphore first, and which one will be serialized second. As such, this synchronization code doesn't actually provide any guarantees what position will be read, and what position will be written. The only thing it prevents is read in the middle of write (sparse read) and vice versa, but that seems to be a moot point anyway since code already doesn't guarantee where read and write will occur, position-wise. And even then, sparse read and write can occur with synchronous Read() and Write() if caller is multi-threaded employing the same near simultaneous read/write calls on multiple threads. In that case read and write operations may be overlapping each other without any guarantees of eventual result.
If implementers do provide some level of guarantee in their synchronous read/write implementation, then it would be just as safe to allow full-duplex behavior in Stream's async default implementations.
Second point - unexpected deadlocks - comes about a very reasonable case of simply blindly proxying data between two streams (e.g. something like authenticating proxy where server receives request, reads authentication info, then opens a connection downstream, and simply connects two streams by blindly passing data between the two, with no guarantee which stream will be sending data first - e.g. with http client is the first to send, but with telnet, it's the server that sends the prompt). Using asynchronous implementation one would very much desire to simply write the proxying part as:
await Task.WhenAll(a.CopyToAsync(b), b.CopyToAsync(a));Yours truly did, and then spent days figuring out the deadlock, and once identifying deadlock, figuring out why a task issued by Stream.WriteAsync() was staying for minutes as Created and never being scheduled to run. The requirement that only one asynchronous outstanding task is allowed is neither documented nor apparent.
Furthermore, this half-duplex enforcement seems to be a lowest-common-denominator approach that ignores such streams as full-duplex network streams, pipe streams, and other non-seekable, position-pointer-less streams that may be non-transactional in nature and require ability to simultaneously read and write. Yes, I realize that there's a facility to simply override the implementation and provide own that will allow full-duplex - and that's what I'm currently going through in our codebase - but it seems that either Stream should not assume anything about implementer and allow full-duplex, with implementing classes recognizing their own limitations and enforcing half-duplex in their implementations (e.g. FileStream would implement its own position-pointer protection logic); or allow control of this behavior by accepting a flag in its constructor allowing deriving class to specify how it expects async default implementations to behave - and thus making the behavior not only controllable, but also apparent.