Remove excessive locks causing deadlock

diff --git a/Hazel/Udp/UdpClientConnection.cs b/Hazel/Udp/UdpClientConnection.cs
index 4ffe4242d5d6f5c563a9c6648e6de58cad77e33d..0291c025f394a2c578985357d830e15bb16847e3 100644 (file)
--- a/Hazel/Udp/UdpClientConnection.cs
+++ b/Hazel/Udp/UdpClientConnection.cs
@@ -21,9 +21,9 @@ namespace Hazel.Udp
         Socket socket;
 
         /// <summary>
-        ///     The lock for the socket.
+        ///     Object for locking the state.
         /// </summary>
-        Object socketLock = new Object();
+        Object stateLock = new Object();
 
         /// <summary>
         ///     The buffer to store incomming data in.
@@ -37,119 +37,117 @@ namespace Hazel.Udp
         public UdpClientConnection(NetworkEndPoint remoteEndPoint)
             : base()
         {
-            lock (socketLock)
-            {
-                this.EndPoint = remoteEndPoint;
-                this.RemoteEndPoint = remoteEndPoint.EndPoint;
-                this.IPMode = remoteEndPoint.IPMode;
+            this.EndPoint = remoteEndPoint;
+            this.RemoteEndPoint = remoteEndPoint.EndPoint;
+            this.IPMode = remoteEndPoint.IPMode;
 
-                if (remoteEndPoint.IPMode == IPMode.IPv4)
-                    socket = new Socket(AddressFamily.InterNetwork, SocketType.Dgram, ProtocolType.Udp);
-                else
-                {
-                    if (!Socket.OSSupportsIPv6)
-                        throw new HazelException("IPV6 not supported!");
+            if (remoteEndPoint.IPMode == IPMode.IPv4)
+                socket = new Socket(AddressFamily.InterNetwork, SocketType.Dgram, ProtocolType.Udp);
+            else
+            {
+                if (!Socket.OSSupportsIPv6)
+                    throw new HazelException("IPV6 not supported!");
 
-                    socket = new Socket(AddressFamily.InterNetworkV6, SocketType.Dgram, ProtocolType.Udp);
-                    socket.SetSocketOption(SocketOptionLevel.IPv6, (SocketOptionName)27, false);    //TODO these lines shouldn't be needed anymore
-                }
+                socket = new Socket(AddressFamily.InterNetworkV6, SocketType.Dgram, ProtocolType.Udp);
+                socket.SetSocketOption(SocketOptionLevel.IPv6, (SocketOptionName)27, false);    //TODO these lines shouldn't be needed anymore
             }
         }
 
         /// <inheritdoc />
         protected override void WriteBytesToConnection(byte[] bytes)
         {
-            lock (socketLock)
+            lock (stateLock)
             {
                 if (State != ConnectionState.Connected && State != ConnectionState.Connecting)
                     throw new InvalidOperationException("Could not send data as this Connection is not connected and is not connecting. Did you disconnect?");
+            }
 
-                try
-                {
-                    socket.BeginSendTo(
-                        bytes, 
-                        0, 
-                        bytes.Length, 
-                        SocketFlags.None, 
-                        RemoteEndPoint,
-                        delegate (IAsyncResult result)
+            try
+            {
+                socket.BeginSendTo(
+                    bytes, 
+                    0, 
+                    bytes.Length, 
+                    SocketFlags.None, 
+                    RemoteEndPoint,
+                    delegate (IAsyncResult result)
+                    {
+                        try
                         {
-                            try
-                            {
-                                lock (socket)
-                                    socket.EndSendTo(result);
-                            }
-                            catch (ObjectDisposedException e)
-                            {
-                                HandleDisconnect(new HazelException("Could not send as the socket was disposed of.", e));
-                            }
-                            catch (SocketException e)
-                            {
-                                HandleDisconnect(new HazelException("Could not send data as a SocketException occured.", e));
-                            }
-                        }, 
-                        null
-                    );
-                }
-                catch (ObjectDisposedException)
-                {
-                    //User probably called Disconnect in between this method starting and here so report the issue
-                    throw new InvalidOperationException("Could not send data as this Connection is not connected. Did you disconnect?");
-                }
-                catch (SocketException e)
-                {
-                    HazelException he = new HazelException("Could not send data as a SocketException occured.", e);
-                    HandleDisconnect(he);
-                    throw he;
-                }
+                            lock (socket)
+                                socket.EndSendTo(result);
+                        }
+                        catch (ObjectDisposedException e)
+                        {
+                            HandleDisconnect(new HazelException("Could not send as the socket was disposed of.", e));
+                        }
+                        catch (SocketException e)
+                        {
+                            HandleDisconnect(new HazelException("Could not send data as a SocketException occured.", e));
+                        }
+                    }, 
+                    null
+                );
+            }
+            catch (ObjectDisposedException)
+            {
+                //User probably called Disconnect in between this method starting and here so report the issue
+                throw new InvalidOperationException("Could not send data as this Connection is not connected. Did you disconnect?");
+            }
+            catch (SocketException e)
+            {
+                HazelException he = new HazelException("Could not send data as a SocketException occured.", e);
+                HandleDisconnect(he);
+                throw he;
             }
         }
 
         /// <inheritdoc />
         public override void Connect(byte[] bytes = null, int timeout = 5000)
         {
-            lock(socketLock)
+            lock (stateLock)
             {
                 if (State != ConnectionState.NotConnected)
                     throw new InvalidOperationException("Cannot connect as the Connection is already connected.");
 
                 State = ConnectionState.Connecting;
+            }
+                
+            //Begin listening
+            try
+            {
+                if (IPMode == IPMode.IPv4)
+                    socket.Bind(new IPEndPoint(IPAddress.Any, 0));
+                else
+                    socket.Bind(new IPEndPoint(IPAddress.IPv6Any, 0));
+            }
+            catch (SocketException e)
+            {
+                State = ConnectionState.NotConnected;
+                throw new HazelException("A socket exception occured while binding to the port.", e);
+            }
 
-                //Begin listening
-                try
-                {
-                    if (IPMode == IPMode.IPv4)
-                        socket.Bind(new IPEndPoint(IPAddress.Any, 0));
-                    else
-                        socket.Bind(new IPEndPoint(IPAddress.IPv6Any, 0));
-                }
-                catch (SocketException e)
-                {
-                    State = ConnectionState.NotConnected;
-                    throw new HazelException("A socket exception occured while binding to the port.", e);
-                }
-
-                try
-                {
-                    StartListeningForData();
-                }
-                catch (ObjectDisposedException)
-                {
-                    //If the socket's been disposed then we can just end there but make sure we're in NotConnected state.
-                    //If we end up here I'm really lost...
+            try
+            {
+                StartListeningForData();
+            }
+            catch (ObjectDisposedException)
+            {
+                //If the socket's been disposed then we can just end there but make sure we're in NotConnected state.
+                //If we end up here I'm really lost...
+                lock (stateLock)
                     State = ConnectionState.NotConnected;
-                    return;
-                }
-                catch (SocketException e)
-                {
-                    Dispose();
-                    throw new HazelException("A Socket exception occured while initiating a receive operation.", e);
-                }
+                return;
+            }
+            catch (SocketException e)
+            {
+                Dispose();
+                throw new HazelException("A Socket exception occured while initiating a receive operation.", e);
             }
 
             //Write bytes to the server to tell it hi (and to punch a hole in our NAT, if present)
             //When acknowledged set the state to connected
-            SendHello(bytes, () => { lock (socketLock) State = ConnectionState.Connected; });
+            SendHello(bytes, () => { lock (stateLock) State = ConnectionState.Connected; });
 
             //Wait till hello packet is acknowledged and the state is set to Connected
             bool timedOut = !WaitOnConnect(timeout);
@@ -167,8 +165,7 @@ namespace Hazel.Udp
         /// </summary>
         void StartListeningForData()
         {
-            lock (socketLock)
-                socket.BeginReceive(dataBuffer, 0, dataBuffer.Length, SocketFlags.None, ReadCallback, dataBuffer);
+            socket.BeginReceive(dataBuffer, 0, dataBuffer.Length, SocketFlags.None, ReadCallback, dataBuffer);
         }
 
         /// <summary>
@@ -182,8 +179,7 @@ namespace Hazel.Udp
             //End the receive operation
             try
             {
-                lock (socketLock)
-                    bytesReceived = socket.EndReceive(result);
+                bytesReceived = socket.EndReceive(result);
             }
             catch (ObjectDisposedException)
             {
@@ -230,7 +226,7 @@ namespace Hazel.Udp
         {
             bool invoke = false;
 
-            lock (socketLock)
+            lock (stateLock)
             {
                 //Only invoke the disconnected event if we're not already disconnecting
                 if (State == ConnectionState.Connected)
@@ -255,16 +251,18 @@ namespace Hazel.Udp
             if (disposing)
             {
                 //Send disconnect message if we're not already disconnecting
-                if (State == ConnectionState.Connected)
+                bool connected;
+                lock (stateLock)
+                    connected = State == ConnectionState.Connected;
+
+                if (connected)
                     SendDisconnect();
 
                 //Dispose of the socket
-                lock (socketLock)
-                {
+                lock (stateLock)
                     State = ConnectionState.NotConnected;
 
-                    socket.Close();
-                }
+                socket.Close();
             }
 
             base.Dispose(disposing);

diff --git a/Hazel/Udp/UdpConnectionListener.cs b/Hazel/Udp/UdpConnectionListener.cs
index fc680858c2a1f151169d5d1355f2c814ff6cb8c0..cb17c70e16c3cbc834b3ab74d93364e064a41e5a 100644 (file)
--- a/Hazel/Udp/UdpConnectionListener.cs
+++ b/Hazel/Udp/UdpConnectionListener.cs
@@ -68,8 +68,7 @@ namespace Hazel.Udp
         {
             try
             {
-                lock (listener)
-                    listener.Bind(EndPoint);
+                listener.Bind(EndPoint);
             }
             catch (SocketException e)
             {
@@ -88,8 +87,7 @@ namespace Hazel.Udp
             
             try
             {
-                lock (listener)
-                    listener.BeginReceiveFrom(dataBuffer, 0, dataBuffer.Length, SocketFlags.None, ref remoteEP, ReadCallback, dataBuffer);
+                listener.BeginReceiveFrom(dataBuffer, 0, dataBuffer.Length, SocketFlags.None, ref remoteEP, ReadCallback, dataBuffer);
             }
             catch (ObjectDisposedException)
             {
@@ -116,8 +114,7 @@ namespace Hazel.Udp
             //End the receive operation
             try
             {
-                lock (listener)
-                    bytesReceived = listener.EndReceiveFrom(result, ref remoteEndPoint);
+                bytesReceived = listener.EndReceiveFrom(result, ref remoteEndPoint);
             }
             catch (ObjectDisposedException)
             {
@@ -190,21 +187,18 @@ namespace Hazel.Udp
         {
             try
             {
-                lock (listener)
-                {
-                    listener.BeginSendTo(
-                        bytes,
-                        0,
-                        bytes.Length,
-                        SocketFlags.None,
-                        endPoint,
-                        delegate (IAsyncResult result)
-                        {
-                            listener.EndSendTo(result);
-                        },
-                        null
-                    );
-                }
+                listener.BeginSendTo(
+                    bytes,
+                    0,
+                    bytes.Length,
+                    SocketFlags.None,
+                    endPoint,
+                    delegate (IAsyncResult result)
+                    {
+                        listener.EndSendTo(result);
+                    },
+                    null
+                );
             }
             catch (SocketException e)
             {
@@ -231,10 +225,7 @@ namespace Hazel.Udp
         protected override void Dispose(bool disposing)
         {
             if (disposing)
-            {
-                lock (listener)
-                    listener.Close();
-            }
+                listener.Close();
 
             base.Dispose(disposing);
         }

diff --git a/Hazel/Udp/UdpServerConnection.cs b/Hazel/Udp/UdpServerConnection.cs
index 566bf1ec9c744d8d7deb837ebea1e80bb7964884..71bd8f6a8fd3d8b261fd9a4d45d5a97ed28d23f6 100644 (file)
--- a/Hazel/Udp/UdpServerConnection.cs
+++ b/Hazel/Udp/UdpServerConnection.cs
@@ -51,9 +51,9 @@ namespace Hazel.Udp
             {
                 if (State != ConnectionState.Connected)
                     throw new InvalidOperationException("Could not send data as this Connection is not connected. Did you disconnect?");
-
-                Listener.SendData(bytes, RemoteEndPoint);
             }
+
+            Listener.SendData(bytes, RemoteEndPoint);
         }
 
         /// <inheritdoc />
@@ -96,15 +96,18 @@ namespace Hazel.Udp
             if (disposing)
             {
                 //Send disconnect message if we're not already disconnecting
-                if (State == ConnectionState.Connected)
-                    SendDisconnect();
+                bool connected;
 
                 lock (stateLock)
-                {
-                    Listener.RemoveConnectionTo(RemoteEndPoint);
+                    connected = State == ConnectionState.Connected;
 
+                if (connected)
+                    SendDisconnect();
+                
+                Listener.RemoveConnectionTo(RemoteEndPoint);
+
+                lock (stateLock)
                     State = ConnectionState.NotConnected;
-                }
             }
 
             base.Dispose(disposing);