Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WaitForConnectionAsync does not respond to cancellation #40289

Closed
jaredpar opened this issue Aug 3, 2020 · 8 comments · Fixed by #53340
Closed

WaitForConnectionAsync does not respond to cancellation #40289

jaredpar opened this issue Aug 3, 2020 · 8 comments · Fixed by #53340

Comments

@jaredpar
Copy link
Member

jaredpar commented Aug 3, 2020

The documentation for NamedPipeServerStream.WaitForConnectionAsync(CancellationToken) claims the following:

Cancellation requests using the cancellation token will only work if the NamedPipeServerStream object was created with a pipe option value of PipeOptions.Asynchronous or if the cancellation occurs before the WaitForConnectionAsync method is called.

This does not appear to be true though when .NET Core is run in Unix environments. The following code snippet will hang waiting for the WaitForConnectionAsync method to complete on Unix. It does not seem to respect the cancellation request. The same code running on Windows though will properly cancel the WaitForConnectionAsync.

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

class Program
{
    static async Task Main(string[] args)
    {
        using var mre = new ManualResetEvent(initialState: false);
        var pipeName = Guid.NewGuid().ToString();
        var cts = new CancellationTokenSource();
        var task = Task.Run(async () =>
        {
            try
            {
                using var server = new NamedPipeServerStream(
                    pipeName,
                    PipeDirection.InOut,
                    maxNumberOfServerInstances: 10,
                    PipeTransmissionMode.Byte,
                    PipeOptions.Asynchronous);
                var waitTask = server.WaitForConnectionAsync(cts.Token);

                // The sync portion of WaitForConnectionAsync is the only part that 
                // considers the CancellationToken on Unix. Signal other thread that
                // it's time to cancel.
                mre.Set();
                await waitTask;
                Console.WriteLine($"After await waitTask");
            }
            catch (Exception ex)
            {
                Console.WriteLine($"Exception thrown {ex.Message}");
            }
        });

        mre.WaitOne();
        cts.Cancel();
        Console.WriteLine("Token cancelled");
        await task;
        Console.WriteLine("await task complete");
    }
}

Looking at the WaitForConnectionAsync implementation on Unix it seems that the cancellation is simply not considered after a quick check at the start of the method.

        public Task WaitForConnectionAsync(CancellationToken cancellationToken)
        {
            CheckConnectOperationsServer();
            if (State == PipeState.Connected)
            {
                throw new InvalidOperationException(SR.InvalidOperation_PipeAlreadyConnected);
            }

            return cancellationToken.IsCancellationRequested ?
                Task.FromCanceled(cancellationToken) :
                WaitForConnectionAsyncCore();

            async Task WaitForConnectionAsyncCore() =>
               HandleAcceptedSocket(await _instance!.ListeningSocket.AcceptAsync().ConfigureAwait(false));
        }

Is the documentation simply out of date here and should be using and not or?

@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Aug 3, 2020
@jaredpar
Copy link
Member Author

jaredpar commented Aug 4, 2020

Played with this a bit more today and even if this is a doc bug there still seems to be a bit of a problem here because there is no reliable way to break a NamedPipeServerStream out of WaitForConnectionAsync.

Given the current implementation of NamedPipeServerStream the WaitForConnectAsync method will only complete under the following circumstances (at least as far as I can tell / experimentally demonstrate):

  1. A valid connection is established from a NamedPipeClientStream.
  2. All NamedPipeServerStream for that pipe name are disposed or finalized.

The second issue can be demonstrated with the following repro:

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

class Program
{
    static async Task Main(string[] args)
    {
        await RunRepro($"example-pipe");
    }
    static async Task RunRepro(string pipeName)
    {
        var server1 = CreateServer();
        var server1Task = RunServer(server1);

        using var client1 = new NamedPipeClientStream(pipeName);
        await client1.ConnectAsync();
        await server1Task;
        Console.WriteLine("After await server1Task");

        var server2 = CreateServer();
        var server2Task = RunServer(server2);
        server2.Dispose();

        // Uncomment this line and the await below will complete
        // server1.Dispose();

        // This will hang indefinitely because even disposing server2 is not enough
        // to break it out of the WaitForConnectionAsync method
        await server2Task;

        Console.WriteLine("After await server2Task");

        async Task RunServer(NamedPipeServerStream server)
        {
            try
            {
                await server.WaitForConnectionAsync();
            }
            catch
            {
                Console.WriteLine("Error waiting for connection");
            }
        }

        NamedPipeServerStream CreateServer() => new NamedPipeServerStream(
            pipeName,
            PipeDirection.InOut,
            maxNumberOfServerInstances: NamedPipeServerStream.MaxAllowedServerInstances,
            PipeTransmissionMode.Byte,
            PipeOptions.Asynchronous | PipeOptions.WriteThrough);
    }
}

This is a bit of a problem because it means once you enter WaitForConnectionAsync there really isn't a way to reliably stop waiting.

Am I missing a better way to fore the WaitForConnectionAsync to exit here when I need to tear down the server side of the connection?

@stephentoub
Copy link
Member

stephentoub commented Aug 4, 2020

@scalablecory, do we expose any way to cancel an asynchronous socket accept operation other than disposing of the socket?

@stephentoub
Copy link
Member

(Fixing this correctly will likely require the AcceptAsync overload approved-but-not-yet-implemented in #33418.)

@stephentoub stephentoub added the bug label Aug 4, 2020
@scalablecory
Copy link
Contributor

@scalablecory, do we expose any way to cancel an asynchronous socket accept operation other than disposing of the socket?

Nope, we need a new API for that.

@carlossanlop carlossanlop added this to the Future milestone Aug 6, 2020
@carlossanlop carlossanlop removed the untriaged New issue has not been triaged by the area owner label Aug 8, 2020
@petsuter
Copy link

Fixing this correctly will likely require the AcceptAsync overload approved-but-not-yet-implemented in #33418.

In that ticket AcceptAsync now seems to be listed under:

These were implemented in #47229

Does this mean WaitForConnectionAsync can now be fixed to handle cancellation?

@stephentoub
Copy link
Member

In that ticket AcceptAsync now seems to be listed under:

These were implemented in #47229

These were not implemented. Not sure why they're listed as such. @scalablecory, @geoffkizer

@geoffkizer
Copy link
Contributor

These were not implemented. Not sure why they're listed as such.

Copy-paste error. I've fixed it.

Note these APIs are (and were) still listed in the initial section containing APIs that need implementing.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 27, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 29, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants