Stream's *Async() methods are forced half-duplex that can lead to unexpected deadlocks.

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.

FWIW, this behavior has been around since the earliest days of .NET (the XxAsync methods just inherited it from the Begin/EndXx methods they wrapped). It's still here not because we like it (I don't), but because of concerns about significant breaks / race conditions occurring if it were to be removed.

@stephentoub

@stephentoub, I kind of figured as much, and to that end that's why I described my reasoning why it doesn't seem to provide virtually any protection in the first place to begin with. So it kind of seems like it doesn't really provide anything and is already racy, and deadlocky to add on top.

At minimum, it seems warranted to describe its half-duplex nature in relevant MSDN entries, and in implementers note section of documentation of Stream class describe default behavior and guide that if there's possibility of multiple async operations, that the implementers should also override and provide their own implementation of WriteAsync() and ReadAsync() to accomplish their task without use of the base's implementation. Else currently it's a hidden trap that's expensive to troubleshoot.

But beyond documentation update, I'm wondering if it's possible to simply keep backward compatibility, while at the same time also having a correct and robust implementation for full-duplex, and providing means of enabling the alternative full-duplex functionality. E.g. provide a new overload of constructor with parameter fullDuplex = false as default, and allowing deriving class to override it to set it to true thus signaling to those functions to use full-duplex branch of implementation? Or possibly decorate deriving class with an attribute to signal the same?

If you do go that route and one way or another have a way to support full-duplex, may I suggest implementing it such that multiple successive WriteAsync() calls don't actually race with each other, but simply detect that there's already an outstanding async write task(s) issued, and simply tack on new pending task to the end of list (and same idea for ReadAsync() as well?). This would allow for a simpler usage in case calling code can issue a bunch of writes without having to await each one, and still be sure that writes occur in the intended order. Not that it's impossible to do it now (init var to completed task, then in some loop that issues writes simply reference that var with .ContinueWith() and finally await just the last task issued. With honored order, that code becomes a bit simpler and more efficient, IMHO.

@stephentoub

Is this true for all streams or just some? I was under impression that at least NetworkStream and SslStream can be duplex. Am I wrong @stephentoub ?

@stephentoub

Is this true for all streams or just some? I was under impression that at least NetworkStream and SslStream can be duplex. Am I wrong @stephentoub ?

The semaphore is in the base implementation of the async methods, and so only applies to streams that don't override them. Both streams you mention override them.

Don't really know is my problem the same as described, but I have small reproduction code.
In my original case I'm running child process to which I'm passing data into it's InputStream and if the process for some reason doesn't read the data the main process stucks in the deadlock even if the passed cancellation token is fired.
Funny thing that if we will set breakpoint on the stream.WriteAsync line we won't see any deadlocks, instead we will got an expected exception about cancelled task (obviously because token cancelled before executing the method).
When we will get deadlock is depends on the size of buffer, in passed code it will be first time.

using System;
using System.IO;
using System.IO.Pipes;
using System.Threading;
using System.Threading.Tasks;

namespace DeadlockSample
{
    class Program
    {
        static async Task Main(string[] args)
        {
            var buffer = new byte[10000];
            using var ts = new CancellationTokenSource();
            using var pipeServer = new AnonymousPipeServerStream(PipeDirection.Out, HandleInheritability.Inheritable);
            using var pipeClient = new AnonymousPipeClientStream(PipeDirection.In, pipeServer.GetClientHandleAsString());

            int i = 10;
            while( i --> 0)
            {
                Console.WriteLine("Send frame async");
                await Send(pipeServer, buffer, ts.Token, TimeSpan.FromMilliseconds(100));
            }
        }        

        public static async ValueTask Send(Stream stream, ReadOnlyMemory<byte> bytes, CancellationToken cancellationToken, TimeSpan timeout)
        {
            using var ts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);
            ts.CancelAfter(timeout);
            try
            {
                await stream.WriteAsync(bytes, ts.Token);
            }
            catch (Exception ex)
            {
                Console.WriteLine("Stream send error. Exception: {0}", ex);
            }
        }
    }
}

@flaksirus

How do you consume the data @flaksirus? The pipeClient does not seems to be used. Eventually, you will fill up socket buffer and then everything will get stuck. If you are running on Linux, you can use strace to look at OS system calls.

@wfurt

@wfurt This one is just an example. In original case I use FFmpeg. So from my code I'm expecting that WriteAsync method would throw exception after cancelation requested.

@flaksirus

@flaksirus So am I correct in understanding that you launch FFmpeg as child process, and use a single stream to write to and read from FFmpeg? If so, then the answer is maybe, depending on whether the Stream derived class that you use (specifically, does it override Stream.WriteAsync and Stream.ReadAsync methods), how data is produced/consumed in terms of timing, and how read/write calls are done (sequentially or parallel).

It's hard to tell from your code snippet because it's incomplete for this problem. You only gave one stream's WriteAsync() call, and it is not clear what's reading it, and how the other stream is written to and/or read from. Is pipeServer representing a stream that is connected to FFmpeg's stdin, and pipeClient is a stream that's connected to FFmpeg's stdout that you would be reading somewhere else in the program? I.e. are both streams unidirectional? If so, then it's likely not this issue. But it could be just a general timing and buffer filling fault in that you may be writing too much and not reading to keep data moving, or trying to read without having written enough, and in both cases that could leak to a deadlock, especially if you have a single flow (thread) of execution.

@LBensman

@LBensman I'm using FFmpeg by creating simple Process and writing data to StandardInput.BaseStream via WriteAsync (cause it's binary data). I am also subscribed on both OutputDataReceived and ErrorDataReceived just to log. And actually I realised that I don't really know which Stream exactly is used (UPD as I see from sources it is a FileStream and it does override WriteAsync). Problem acquires when FFmpeg process can not connect to output (RTSP server) e.g. when there is no RTSP server at all, so it's not reading data from input stream and I'm still expecting that fired CancellationToken would throw an except instead then just freezing the thread.

@flaksirus

@flaksirus Without diving into the code to see what actual stream type is used for .BaseStream and how it handles cancellations, my guess is that it's "blocked" trying to finish writing the whole buffer and if FFmpeg is not reading its stdin, you got deadlock in this case, or at least until FFmpeg gives up on connecting to RTSP server and exits, at which point it'll signal that stdin is closed and at that time write should return with error or count of bytes written - don't recall exactly).

@LBensman

@LBensman yes, the same was on my mind. But I was expecting that cancellationToken will help me to avoid that deadlock.

@flaksirus

@flaksirus , Yeah, I think you'd have to dive into that class' implementation to see how the cancellation token is handled...

Just ran into the deadlock situation when writing tests and providing a mock for a NetworkStream. Took me a while to find this issue.

Looks like others ran into this issue and found the post useful, so giving it additional lease on life to prevent bot from cleaning it up. I'm assuming it's still an issue in latest framework, though haven't checked.

Navigation Menu

Repository navigation

Stream's *Async() methods are forced half-duplex that can lead to unexpected deadlocks. #36481

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	// 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();

Stream's *Async() methods are forced half-duplex that can lead to unexpected deadlocks. #36481

Description

Activity

stephentoub commented on May 15, 2020

LBensman commented on May 15, 2020

wfurt commented on May 18, 2020

stephentoub commented on May 18, 2020

flaksirus commented on Jul 2, 2020

wfurt commented on Jul 2, 2020

flaksirus commented on Jul 2, 2020

LBensman commented on Jul 6, 2020

flaksirus commented on Jul 6, 2020

LBensman commented on Jul 6, 2020

flaksirus commented on Jul 6, 2020

LBensman commented on Jul 6, 2020

Dimension4 commented on Feb 16, 2021

LBensman commented on Aug 18, 2025

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions