From df26b43e38e397fe88ebf72c4f146d95014a5e91 Mon Sep 17 00:00:00 2001 From: Jamie Read Date: Mon, 15 Jan 2018 23:37:53 +0000 Subject: [PATCH] Remove excessive locks causing deadlock --- Hazel/Udp/UdpClientConnection.cs | 188 ++++++++++++++--------------- Hazel/Udp/UdpConnectionListener.cs | 41 +++---- Hazel/Udp/UdpServerConnection.cs | 17 +-- 3 files changed, 119 insertions(+), 127 deletions(-) diff --git a/Hazel/Udp/UdpClientConnection.cs b/Hazel/Udp/UdpClientConnection.cs index 4ffe424..0291c02 100644 --- a/Hazel/Udp/UdpClientConnection.cs +++ b/Hazel/Udp/UdpClientConnection.cs @@ -21,9 +21,9 @@ namespace Hazel.Udp Socket socket; /// - /// The lock for the socket. + /// Object for locking the state. /// - Object socketLock = new Object(); + Object stateLock = new Object(); /// /// 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 } } /// 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; } } /// 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 /// void StartListeningForData() { - lock (socketLock) - socket.BeginReceive(dataBuffer, 0, dataBuffer.Length, SocketFlags.None, ReadCallback, dataBuffer); + socket.BeginReceive(dataBuffer, 0, dataBuffer.Length, SocketFlags.None, ReadCallback, dataBuffer); } /// @@ -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 fc68085..cb17c70 100644 --- 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 566bf1e..71bd8f6 100644 --- 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); } /// @@ -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); -- 2.39.5