Named pipes hang when written asynchronously w/o async flag

[AArnott]

When creating named pipes on .NET or .NET Core, unless I use PipeOptions.Asynchronous, async I/O operations done later end up hanging the I/O operations. Even if I pass in a CancellationToken, these async I/O methods do not cancel when demanded.

IMO the async flag should be simply an optimization -- not something that if set incorrectly causes a hang.
But even if a hang is acceptable, surely the CancellationToken should be honored, no?

Minimal repro: XUnitTestProject5.zip
You'll see the test passes, but if you change PipeOptions used in the repro to None, the test hangs.

Source code:

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

#nullable enable

public class UnitTest1
{
    private readonly CancellationToken TimeoutToken = new CancellationTokenSource(5000).Token;

    private readonly string PipeName = Guid.NewGuid().ToString();
    private const PipeOptions Options = PipeOptions.Asynchronous; // FAILS when set to None
    private const PipeDirection Direction = PipeDirection.InOut;

    [Fact]
    public async Task Test1()
    {
        var serverTask = RunServer();
        using var clientPipe = new NamedPipeClientStream(".", PipeName, Direction, Options);
        await clientPipe.ConnectAsync(this.TimeoutToken);

        Task readTask = ReadBlockAsync(clientPipe, 3, this.TimeoutToken);
        await clientPipe.WriteAsync(new byte[] { 1, 2, 3 }, this.TimeoutToken);
        await clientPipe.FlushAsync(this.TimeoutToken);
        await Task.WhenAll(readTask, serverTask);
    }

    private async Task RunServer()
    {
        using var serverPipe = new NamedPipeServerStream(PipeName, Direction, maxNumberOfServerInstances: 1, PipeTransmissionMode.Byte, Options);
        await serverPipe.WaitForConnectionAsync(this.TimeoutToken);

        Task readTask = ReadBlockAsync(serverPipe, 3, this.TimeoutToken);
        await serverPipe.WriteAsync(new byte[] { 4, 5, 6 }, this.TimeoutToken);
        await serverPipe.FlushAsync(this.TimeoutToken);
        await readTask;
    }

    private static async Task<ReadOnlyMemory<byte>> ReadBlockAsync(Stream stream, int length, CancellationToken cancellationToken)
    {
        byte[] buffer = new byte[length];
        int bytesRead = 0;
        while (bytesRead < length)
        {
            int bytesJustRead = await stream.ReadAsync(buffer.AsMemory(bytesRead), cancellationToken);
            if (bytesJustRead == 0)
            {
                throw new Exception("Lost connection.");
            }

            bytesRead += bytesJustRead;
        }

        return buffer;
    }
}

[stephentoub]

async I/O operations done later end up hanging the I/O operations

That's the problem, though: if you don't pass the Asynchronous flag, then the operations can't be done with async I/O (at the Windows level). That also means we don't have an overlapped with which to call CancelIoEx. So when Asynchronous isn't used, these async methods just check the cancellation token upfront, and then queue a work item that does the synchronous operation.

[AArnott]

OK, so if this is a Windows-OS level limitation, that's fine. I just wanted to make sure.
Also, I wonder if this is documented on the .NET API level. Other I/O APIs (like the FileStream ctor) mention that the async flag needn't match how it's used, but that if it mismatches then perf regressions can result. I expected the same would be true for named pipes, but apparently it's critical to not mismatch for pipes.

[stephentoub]

Other I/O APIs (like the FileStream ctor) mention that the async flag needn't match how it's used
apparently it's critical to not mismatch for pipes

FileStream has the same issue. It's just harder to see with FileStream, as they're most commonly used on Windows with disk-based files, where reads and writes don't block indefinitely and are usually very fast.

[poizan42]

You can call CancelSynchronousIo to cancel synchronous io. Seems like it might need some api work to support using it though as the base BeginRead / BeginWrite functions on Stream currently have no way of knowing how to cancel the operation.

[stephentoub]

You can call CancelSynchronousIo to cancel synchronous io

Maybe. What's not clear is how much overhead the tracking would add. Locks would need to be used to ensure that the right I/O was canceled (and not some other I/O the thread moved on to running completely unrelated to the original work), and the thread handle would need to be retrieved before each operation (though potentially as a pseudo handle we could play tricks to avoid that). Still, yes, it could be prototyped and measured.

[poizan42]

Doesn't seem to need any locks in the common path, CancellationTokenRegistration.Dispose will wait until any outstanding callbacks are completed, but only uses a lock if there are any callbacks in flight.

Proof of concept here (for AsyncRead only, and I'm currently just testing with a FileStream over a pipe handle): https://github.com/poizan42/coreclr/tree/cancelable-pipe-poc

Some testing: https://github.com/poizan42/dotnet-TestSyncStreamCancel (only the TestPipeCancel project currently does anything)

I will try to do some benchmarking.

[stephentoub]

Thanks for sharing.

Doesn't seem to need any locks

poizan42/coreclr@0544212#diff-de1f7994963a3dc0f1068ff2ff1c3c2aR348-R352 is a spin lock. It's also not clear to me from the docs what guarantees if any CancelSynchronousIo makes, especially since it states "other operations can continue toward completion before they are actually canceled"... as such it makes me very nervous to block the cancellation callback waiting for the actual operation to complete; while it'd be bad enough blocking two threads, the second thread is likely to be the one calling Cancel. Also seems like this could lead to deadlock; maybe contrived, but imagine if an override of Read was the one calling Cancel.

[poizan42]

Well, I meant no locks in the path when not cancelling, performance when cancelling is probably less important. But I agree that the cancellation callback needs more work, for now it was mostly for testing the potentiel impact on existing code not using cancellation.

[poizan42]

I have rebased the PoC on dotnet/runtime with some improvements (especially to OnCancelReadWrite), it is here now: https://github.com/poizan42/dotnet-runtime/tree/cancelable-pipe-poc

Using the FileStreamPerformance project from https://github.com/poizan42/dotnet-TestSyncStreamCancel I find

Old

Method	Mean	Error	StdDev	Gen 0	Gen 1	Gen 2	Allocated
ReadAll	521.8 ms	5.28 ms	4.94 ms	3000.0000	-	-	10.94 MB
ReadAllCancelable	522.2 ms	3.48 ms	3.25 ms	3000.0000	-	-	10.94 MB

Adding CancellationToken to semaphore wait and checking in RunReadWriteTaskWhenReady continuation + adding fields to ReadWriteTask + adding onCancelReadWriteDelegate.

Method	Mean	Error	StdDev	Gen 0	Gen 1	Gen 2	Allocated
ReadAll	522.6 ms	2.87 ms	2.69 ms	4000.0000	-	-	12.5 MB
ReadAllCancelable	544.3 ms	6.31 ms	4.92 ms	7000.0000	-	-	23.44 MB

New

Method	Mean	Error	StdDev	Gen 0	Gen 1	Gen 2	Allocated
ReadAll	526.2 ms	4.49 ms	4.20 ms	4000.0000	-	-	12.5 MB
ReadAllCancelable	552.5 ms	4.32 ms	4.04 ms	7000.0000	-	-	23.44 MB

Any performance penalty in the case of a None CancellationToken completely drowns out in the noice. Most of the cost in the case of using a real cancelable CancellationToken actually comes from passing it along to the wait for the _asyncActiveSemaphore. We of course pay a bit in memory usage for the two extra fields on ReadWriteTask, but a real implementation could use a different classs when given a cancelable CancellationToken.

There is one extra collection happening after the changes when using a None CancellationToken, this a bit surprisingly appears the moment the cached static delegate field onCancelReadWriteDelegate is added whether it is used or not. Being a single static field I doubt this has any effect outside of the artificial benchmark.

[JeremyKuhne]

Triage: @stephentoub Do you have any further comments on @poizan42's updates? Is the deadlock risk addressed?