1
\$\begingroup\$

I'm trying to implement a thread-safe Connection Manager. I've split my class in two files, one with TraceMessages and another with the actual implementation. I'm just going to post the file with the implementation, ok?

Can you guys please give me some feedback?

using System;
using System.Collections.Concurrent;
using System.Diagnostics;
using System.Net.Sockets;
using System.Threading;
using System.Threading.Tasks;

namespace Overlook.Dispatcher
{
    public sealed partial class ClientConnectionManager : IDisposable
    {
        // Used to uniquely identify instances and improve Trace messages
        private static object _StaticLock = new object();
        private static int _NumberOfInstancesCreated = 0;
        private readonly int _InstanceId;

        // Used to prevent multithreading-dispose issues
        private ReaderWriterLockSlim _DisposeLock = new ReaderWriterLockSlim(LockRecursionPolicy.NoRecursion);

        // The object used to accept connections
        private TcpListener _ClientTcpListener;

        // Used to prevent multiple calls to StartListening()
        private object _ListeningLock = new object();
        private bool _CurrentlyListening = false;

        /// <summary>
        // Client connections are 'stored' here after the connection is established and "validated".
        // The decision of 'accepting its request', 'disconnecting' or whatever is taken somewhere else.
        /// </summary>
        private BlockingCollection<TcpClient> _Clients = new BlockingCollection<TcpClient>();

        /// <summary>
        /// Creates a new instance of <see cref="ClientConnectionManager"/>.
        /// </summary>
        public ClientConnectionManager(TcpListener clientTcpListener)
        {
            if (clientTcpListener == null)
                throw new ArgumentNullException(nameof(clientTcpListener));

            lock (_StaticLock)
            {
                _InstanceId = _NumberOfInstancesCreated;
                _NumberOfInstancesCreated += 1;
            }

            _ClientTcpListener = clientTcpListener;
            Trace.WriteLine(TraceMessages.InstanceCreated(this));
        }

        /// <summary>
        /// Starts listening for client connections.
        /// Whenever a connection is accepted, a new thread is created to handle it and add it to this managers's
        /// collection of clients.
        /// </summary>
        /// <remarks>
        /// If nothing goes wrong, this method never returns, so it shouldn't be run in the "main thread".
        /// </remarks>
        public void StartListening()
        {
            _DisposeLock.EnterReadLock();
            if (_DisposedValue)
                throw new ObjectDisposedException($"This {nameof(ClientConnectionManager)} has been disposed already.");

            // Prevents multiple calls to StartListening
            lock (_ListeningLock)
            {
                if (_CurrentlyListening)
                    throw new InvalidOperationException($"This {nameof(ClientConnectionManager)} is already listening.");

                _ClientTcpListener.Start();
                _CurrentlyListening = true;
            }

            _DisposeLock.ExitReadLock();

            Trace.WriteLine(TraceMessages.StartedListening(this));

            while (_CurrentlyListening)
            {
                var clientConnection = _ClientTcpListener.AcceptTcpClient();
                Trace.WriteLine(TraceMessages.AcceptedConnection(this, clientConnection));

                Task.Run(() => HandleNewClientConnection(clientConnection));
            }
        }

        /// <summary>
        /// Gets a client connection from this manager.
        /// If there are no connection, this method blocks until there is one avaliable.
        /// </summary>
        public TcpClient GetClientConnection()
        {
            _DisposeLock.EnterReadLock();
            if (_DisposedValue)
                throw new ObjectDisposedException($"This {nameof(ClientConnectionManager)} has been disposed.");
            var client = _Clients.Take();

            _DisposeLock.ExitReadLock();

            return client;
        }

        /// <summary>
        /// Try to get a connection from this manager.
        /// If there are no connections, this method  sets <paramref name="client"/> to null and
        /// returns false immediately.
        /// If there is at least one connection stored, this method sets <paramref name="client"/> appropriately
        /// and returns true immediately.
        /// </summary>
        public bool TryGetClientConnection(out TcpClient client)
        {

            _DisposeLock.EnterReadLock();
            if (_DisposedValue)
                throw new ObjectDisposedException($"This {nameof(ClientConnectionManager)} has been disposed.");

            var gotClient = _Clients.TryTake(out client);

            _DisposeLock.ExitReadLock();

            if (!gotClient)
                client = null;

            return gotClient;
        }

        /// <summary>
        /// This method should do some basic 'connection initialization', like 
        /// negotiating the protocol version the client and the dispatchar are going to use and whatnot.
        /// </summary>
        private void HandleNewClientConnection(TcpClient clientConnection)
        {
            _DisposeLock.EnterReadLock();
            if (_DisposedValue)
                throw new ObjectDisposedException($"This {nameof(ClientConnectionManager)} has been disposed.");

#warning Implement Procol-Version-Handshake

            _Clients.Add(clientConnection);

            _DisposeLock.ExitReadLock();
        }

        /// <summary>
        /// Returns a basic description of this ClientConnectionManager.
        /// </summary>
        public override string ToString()
        {
            return $"{nameof(ClientConnectionManager)} instance id {_InstanceId}";
        }

        #region IDisposable Support
        // bool operations are atomic, but we need to mark it as
        // volatile to make sure we are reading the most up-to-date value
        private volatile bool _DisposedValue = false;

        void Dispose(bool disposing)
        {
            _DisposeLock.EnterWriteLock();

            if (!_DisposedValue)
            {
                if (disposing)
                {
                    // TODO: dispose managed state (managed objects).
                    while (_Clients.Count > 0)
                    {
                        // Maybe I should create a "CloseClientConnection" method?
                        var connection = _Clients.Take();
                        connection.GetStream().Close();
                        connection.Close();
                        connection.Dispose();
                    }
                    _Clients.Dispose();
                }
                _ClientTcpListener.Stop();

                // TODO: free unmanaged resources (unmanaged objects) and override a finalizer below.
                // TODO: set large fields to null.
                _ClientTcpListener = null;
                _Clients = null;
                _DisposedValue = true;
                _CurrentlyListening = false;
            }

            _DisposeLock.ExitWriteLock();
        }

        // TODO: override a finalizer only if Dispose(bool disposing) above has code to free unmanaged resources.
        // ~ClientConnectionMannager() {
        //   // Do not change this code. Put cleanup code in Dispose(bool disposing) above.
        //   Dispose(false);
        // }

        // This code added to correctly implement the disposable pattern.
        public void Dispose()
        {
            // Do not change this code. Put cleanup code in Dispose(bool disposing) above.
            Dispose(true);
            // TODO: uncomment the following line if the finalizer is overridden above.
            // GC.SuppressFinalize(this);
        }
        #endregion
    }
}
\$\endgroup\$

Your Answer

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

Browse other questions tagged or ask your own question.