3
\$\begingroup\$

There is no facility in TcpListener (or for that matter Socket) to close a listening socket that is waiting for a connection without throwing an exception. After running up a test bench with 8 threads opening/closing/connecting thousands of times and reviewing the .Net source I found three different exceptions out of a listening socket when closing it. ObjectDisposedException was b yfar the most common. This inherited class aims to resolve the inabilty to close a TcpListener cleanly when the caller has called AcceptTcpClientAsync()

Please help me by reviewing this:

using System;
using System.Net;
using System.Net.Sockets;
using System.Threading;
using System.Threading.Tasks;

namespace TcpListenerExtended
{
    public class TcpListenerEx : TcpListener
    {
        #pragma warning disable 618
        [Obsolete("This method has been deprecated. Please use TcpListenerEx(IPAddress localaddr, int port) instead. https://go.microsoft.com/fwlink/?linkid=14202")]
        public TcpListenerEx(int port) : base(port) { }
        #pragma warning restore 618
        public TcpListenerEx(IPEndPoint localEP) : base(localEP) { }
        public TcpListenerEx(IPAddress localAddr, int port) : base(localAddr, port) { }

        /// <summary>
        /// The CancellationToken that will be monitored for cancellation requests.
        /// </summary>
        private CancellationToken _cancellationToken;

        /// <summary>
        /// Callback method which will be registered against the CancellationToken passed to AccpetTcpClientAsync()
        /// </summary>
        private void CancellationCallback()
        {
            // Stops the listener hard in its' tracks by closing the port
            // this will cause an await base:AcceptTcpClientAsync() to throw one of three execptions
            base.Stop();
        }

        /// <summary>
        /// Accepts a pending connection request as an asyncronous operation.
        /// </summary>
        /// <param name="cancellationToken">The token to be monitored for cancellation requests.</param>
        /// <returns>
        /// The task object representing the asynchronous operation. The Result property on the task object
        /// returns a TcpClient used to send and receive data.
        /// </returns>
        /// <exception cref="OperationCanceledException">
        /// The listener was closed  due to the <paramref name="cancellationToken"/> transitioning to a
        /// Cancelled state.  The inner exception will contain the actual exception which was thrown by the underlying
        /// .Net libraries.
        /// </exception>
        public async Task<TcpClient> AcceptTcpClientAsync(CancellationToken cancellationToken)
        {
            _cancellationToken = cancellationToken;

            // Register a Callback to be executed when the token is Cancelled.
            _cancellationToken.Register(CancellationCallback);

            try
            {
                return await AcceptTcpClientAsync();
            }
            catch (Exception ex) when (_cancellationToken.IsCancellationRequested)
            {
                // Depending on the exact state of the socket, ex could be one of three exceptions:
                //    * SocketException
                //    * ObjectDisposedException
                //    * InvalidOperationException
                //         or
                //    * An as yet unidentified Exception
                // Bubble up the exception as an InnerException of an OperationCancelledException
                throw new OperationCanceledException("AcceptTcpClientAsync() was cancelled.", ex, _cancellationToken);
            }
            catch
            {
                // An exception caught without a cancellation request is bubbled to the caller
                throw;
            }
        }
    }
}

Below find a test strap for the intended use of the TcpListener

Please do not review this: :-)

static async Task Main(string[] args)
{

    List<Task> taskList = new List<Task>();

    // Create an auto cancelling TokenSource
    CancellationTokenSource cts = new CancellationTokenSource(TimeSpan.FromSeconds(20));
    CancellationToken ct = cts.Token;

    TcpListenerEx tcpListenerEx = new TcpListenerExtended.TcpListenerEx(9999);
    tcpListenerEx.Start();

    taskList.Add(Task.Run(async () =>
    {
        while (true)
        {
            try
            {
                using var tcpClient = await tcpListenerEx.AcceptTcpClientAsync(ct);

                Console.WriteLine($"New connection from {tcpClient.Client.RemoteEndPoint.ToString()}.  Closing...");
                tcpClient.Close();
            }
            catch (OperationCanceledException)
            {
                Console.WriteLine("The TcpListener was closed.");
                break;
            }
            catch
            {
                throw;
            }
        }
    }));

    for(int i = 0;i<10;i++)
    {
        taskList.Add(Task.Run(() =>
        {
            // Initialise and connect
            using var tcpClient = new TcpClient("127.0.0.1", 9999);
            // Server will close connection, task will come to a natural end
        }));

        // Wait ~1 second before firing off the next connection
        await Task.Delay(1000);
    }

    await Task.WhenAll(taskList);
}
\$\endgroup\$
  • \$\begingroup\$ Why do you need to keep token in a private field? \$\endgroup\$ – Bohdan Yarema Jan 15 at 12:27
  • \$\begingroup\$ You're right, in this case I didn't, that's a hang over from other classes I have coded that needed it. Thanks for pointing that out. \$\endgroup\$ – Rowan Smith Jan 15 at 20:08
  • 1
    \$\begingroup\$ Please see What to do when someone answers. I have rolled back Rev 3 → 1 \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Jan 15 at 22:21
2
\$\begingroup\$

You shouldn't store the cancellationToken. If someone was to call AcceptTcpClientAsync twice it would overwrite it. Also it starts making your async call keep state and that's not something you want to do or even need to do in this case.

Also need to wrap the Register into a using statement such as

// Register a Callback to be executed when the token is Cancelled.
using (cancellationToken.Register(CancellationCallback))
{
    try
    {
       return await AcceptTcpClientAsync();
    }

From quick search it seems this is the "standard" way people are handing this.

\$\endgroup\$
  • \$\begingroup\$ Thanks. I had never noticed that Register returned a Disposable! I looked at some Microsoft .Net source examples and did it differently again, please review. As for someone calling it twice, if multiple threads call AcceptTcpClientAsync (or for that matter the underlying Socket.BeginAccept the last one always wins, previous ones will never get anything and just sit there for ever waiting. However I actually have no need for maintaining the state of that object in this case so I have removed it, thanks for making me think about it. \$\endgroup\$ – Rowan Smith Jan 15 at 20:04
  • 1
    \$\begingroup\$ Looks good. I don't know much about TcpListener so can't comment on multiple calls to it. If you did want to cancel the previous one if someone called into it twice that would be the situation to store some state, a linked CancellationTokenSource, and cancel it, if exist, but would need to be done in a memory barrier like Interlocked.Exchange or lock block. \$\endgroup\$ – CharlesNRice Jan 15 at 20:28
1
\$\begingroup\$

Final version of the class, incorporating changes suggested by CharlesNRice, and removing the unnecessary callback method.

public class TcpListenerEx : TcpListener
{
    #pragma warning disable 618
    [Obsolete("This method has been deprecated. Please use TcpListenerEx(IPAddress localaddr, int port) instead. https://go.microsoft.com/fwlink/?linkid=14202")]
    public TcpListenerEx(int port) : base(port) { }
    #pragma warning restore 618
    public TcpListenerEx(IPEndPoint localEP) : base(localEP) { }
    public TcpListenerEx(IPAddress localAddr, int port) : base(localAddr, port) { }

    /// <summary>
    /// Accepts a pending connection request as an asyncronous operation.
    /// </summary>
    /// <param name="cancellationToken">The token to be monitored for cancellation requests.</param>
    /// <returns>
    /// The task object representing the asynchronous operation. The Result property on the task object
    /// returns a TcpClient used to send and receive data.
    /// </returns>
    /// <exception cref="OperationCanceledException">
    /// The listener was closed  due to the <paramref name="cancellationToken"/> transitioning to a
    /// Cancelled state.  If an inner exception is present it will contain the actual exception which
    /// was thrown by the underlying .Net libraries.  Where there is no inner exception, then the
    /// <paramref name="cancellationToken"/> was cancelled before awaiting a pending connection.
    /// </exception>
    public async Task<TcpClient> AcceptTcpClientAsync(CancellationToken cancellationToken)
    {
        // Throw an OperationCancelledException if the supplied token is already cancelled
        // Will throw an ObjectDisposedException if the associated CancellationTokenSource is disposed
        cancellationToken.ThrowIfCancellationRequested();

        // Check that the CancellationToken can be cancelled and register a callback if it can
        // Will throw an ObjectDisposedException if the associated CancellationTokenSource is disposed
        CancellationTokenRegistration ctr = default;
        if (cancellationToken.CanBeCanceled)
            ctr = cancellationToken.Register(base.Stop);  // Base keyword included for clarity

        using (ctr)
        try
        {
            return await AcceptTcpClientAsync();
        }
        catch (Exception ex) when (cancellationToken.IsCancellationRequested)
        {
            // Depending on the exact state of the socket, ex could be one of three exceptions:
            //    * SocketException
            //    * ObjectDisposedException
            //    * InvalidOperationException
            //         or
            //    * An as yet unidentified Exception
            // Bubble up the exception as an InnerException of an OperationCancelledException
            throw new OperationCanceledException("AcceptTcpClientAsync() was cancelled.", ex, cancellationToken);
        }
        catch
        {
            // An exception caught without a cancellation request is bubbled to the caller
            throw;
        }
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.