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

Cancellation token #164

Closed
wants to merge 5 commits into from
Closed

Conversation

zivillian
Copy link
Contributor

I've added an optional CancellationToken parameter to all async methods. Cancellation is handled in SocketExtension.SendToAsync and SocketExtension.ReceiveMessageFromAsync.

I've also added some missing ConfigureAwait(false) and removed some unneeded async/await

fixes #140

@CLAassistant
Copy link

CLAassistant commented Sep 15, 2021

CLA assistant check
All committers have signed the CLA.

@lextm
Copy link
Collaborator

lextm commented Sep 15, 2021

Workarounds like await Task.WhenAny(send, cancellation).ConfigureAwait(false) have side effects, because the actual UDP send call is not actually cancelled due to Microsoft's old API design. True cancellation support is only added in .NET 6 #160 . Therefore, I don't plan to accept this pull request.

But you can publish it as your own NuGet package (such as a package named SharpSnmpLib.WithCancellation, which depends on Lextm.SharpSnmpLib), so that anyone wants to try them out can install the package along.

@zivillian
Copy link
Contributor Author

According to the documentation the outstanding async requests are cancelled if the socket is closed.

To cancel a pending BeginReceiveMessageFrom, call the Close method.

Whenever the socket is created by the library it is used inside an using statement, so when the cancellationToken is cancelled the socket is closed and the SendAsync/ReceiveAsync is aborted.

I've also verified this behaviour with the following unit test:

public class SocketTestFixture
{
    [Fact]
    public async Task TestReceiveIsAborted()
    {
        var receiveWasAborted = false;
        var receiver = new IPEndPoint(IPAddress.Loopback, 11337);
        using (new UdpClient(receiver))//listen to prevent ICMP unreachable
        using (var socket = receiver.GetSocket())
        {
            using (var cts = new CancellationTokenSource())
            {
                try
                {
                    await socket.SendToAsync(new ArraySegment<byte>(Array.Empty<byte>()), SocketFlags.None, receiver);
                    var receive = ReceiveAsync(socket, cts.Token, ReceiveMessageFromAsync);
                    cts.Cancel();
                    await receive;
                }
                catch(TaskCanceledException)
                {
                    //that's expected, we just cancelled the token
                }
            }
        }
        
        await Task.Delay(100);//yield is not enough to execute the catch
        Assert.True(receiveWasAborted);

        async Task ReceiveMessageFromAsync(Socket socket)
        {
            try
            {
                var buffer = new ArraySegment<byte>(new byte[10]);
                EndPoint remote = new IPEndPoint(IPAddress.Any, 0);
                await socket.ReceiveMessageFromAsync(buffer, SocketFlags.None, remote);
            }
            catch
            {
                receiveWasAborted = true;
                throw;
            }
        }
    }

    private async Task ReceiveAsync(Socket socket, CancellationToken cancellationToken, Func<Socket, Task> receiverWrapper)
    {
        var cancellation = Task.Delay(Timeout.Infinite, cancellationToken);
        var receive = receiverWrapper(socket);
        var result = await Task.WhenAny(receive, cancellation).ConfigureAwait(false);
        await result;
        await receive.ConfigureAwait(false);
    }
}

If the socket is passed in via the caller, we cannot guarantee that the socket is diposed properly, so I'm thinking about removing the cancellationToken from all public methods, which have a socket parameter. Then it would be the responsibility of the caller to abort the request by closing the socket (instead of passing a cancellationToken).

@lextm
Copy link
Collaborator

lextm commented Sep 16, 2021

We actually allow users of the library to reuse their Socket instances via overloading versions of GetResponse such as https://github.com/lextudio/sharpsnmplib/blob/12.4.0/SharpSnmpLib/Messaging/SnmpMessageExtension.cs#L271

As far as I can see, the workaround in this pull request will not work well for those overloading methods and might require extra steps. I rather have the true cancellation support for all cases (sadly .NET 6 only) than accepting a workaround that only supports a few cases (but like I explained above you can publish a separate NuGet package for whoever wants that).

@lextm
Copy link
Collaborator

lextm commented Dec 1, 2021

No plan to accept this pull request as discussed above. Close it now to avoid confusion.

Active work is in https://github.com/lextudio/sharpsnmplib/tree/net6 to bring native cancellation support to this library.

@lextm lextm closed this Dec 1, 2021
@lextm lextm added the idea Something out of your mind label Aug 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
idea Something out of your mind
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Please add timeout parameter for Messenger.GetAsync() method.
3 participants