Help with TCP Server.
in [Framework]
|
From: Michael Ober on 18 Dec 2009 18:48 Pete and all, Here's the final code. I actually got this working yesterday but ran into another problem with cross thread errors inside the TCPServer class itself. After googling for the specific error message and finding multiple references as well as a bug report to MS that was closed because MS couldn't duplicate the bug, I realized that all the bug reports were from VB developers and none from C# developers and that the bug first appeared in VB 2005 RTM. VB 2005 and 2008 have a "My" class that provides several shortcuts into the framework, which are really useful to someone just starting in VB simply because of the sheer size of the framework. Reading the My class documentation, almost all these classes eventually inherit from WinForms when used in a WinForms application. Cross thread operations against WinForms has long been documented, even in the Win32 API, as a major don't do. Now when I start maintainence on a program, I search the entire solution for "My." and recode them to use standard framework classes. This fixed the problem and also probably explains why MS couldn't recreate it - they were testing using the framework only. Once again, thanks to everyone, especially Pete, who spent the time to provide feedback. Mike. '============================================ Option Compare Text Option Strict On Option Explicit On Option Infer Off Imports System.Net.Sockets Imports System.Net Imports System.Threading Imports System.Text.ASCIIEncoding Imports System.IO Public MustInherit Class IPServer Implements IDisposable Public Event ConnectionList(ByVal sConnection As List(Of String)) Public MustOverride Function ProcessMessage(ByVal message As String) As String ' My list of clients Private Clients As New ClientList Private _tcpServer As Socket = Nothing Private ReadOnly _CommandTermination As String = BEL ' Timeout support; Asynchronous sockets don't directly support timers. Protected ReadOnly _SocketTimeout As TimeSpan Protected ReadOnly _timer As Timer = Nothing Public Sub New(ByVal Port As Integer) Me.New(Port, BEL, Nothing) End Sub Public Sub New(ByVal Port As Integer, ByVal SocketTimeout As TimeSpan) Me.New(Port, BEL, SocketTimeout) End Sub Public Sub New(ByVal Port As Integer, ByVal CommandTermination As String) Me.New(Port, CommandTermination, Nothing) End Sub Public Sub New(ByVal Port As Integer, ByVal CommandTermination As String, ByVal SocketTimeout As TimeSpan) ' save the termination and TTL before doing anything else to avoid threading issues _CommandTermination = CommandTermination _SocketTimeout = SocketTimeout Try Dim myAddr As IPAddress = Dns.GetHostEntry(Environment.MachineName).AddressList(0) Dim sEndPoint As String = Environment.MachineName & "(" & myAddr.ToString() & "):" & Port.ToString() WriteLog("Configure Listener on " & sEndPoint) _tcpServer = New Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp) Dim LocalEP As IPEndPoint = New IPEndPoint(myAddr, Port) _tcpServer.Bind(LocalEP) _tcpServer.Listen(5) ' 5 is the standard backlog value _tcpServer.BeginAccept(AddressOf OnAccept, Nothing) WriteLog("Listening for connections on " & sEndPoint) If _SocketTimeout <> Nothing Then Dim Interval As New TimeSpan(0, 1, 0) _timer = New Threading.Timer(AddressOf TimerTick, Nothing, Interval, Interval) WriteLog("Socket Timer Started") End If Catch ex As Exception WriteLog(ex) SMTPMail.SendMessage("mis", AppName() & ": Unable to create Listener", ex.Message()) Throw ex End Try End Sub 'Handle connection requests Private Sub OnAccept(ByVal ar As System.IAsyncResult) Dim ci As New ConnectionInformation(Me, _SocketTimeout) Try ci.sock = _tcpServer.EndAccept(ar) ' Start listening for the next connection _tcpServer.BeginAccept(AddressOf OnAccept, Nothing) ' Wait for data ci.BeginReceive(AddressOf OnReceive) Catch ex As Exception WriteLog(ci.ToString(), ex) Finally WriteLog(Clients.ToString(), True) RaiseEvent ConnectionList(Clients.ToList()) End Try End Sub Private Sub OnReceive(ByVal ar As IAsyncResult) Dim ci As ConnectionInformation = CType(ar.AsyncState, ConnectionInformation) Try Dim bytesReceived As Integer = ci.EndReceive(ar) Select Case bytesReceived Case 0 ci.Close("Client Closed Connection") Case Else Dim i As Integer = InStr(ci.MessageIn, _CommandTermination) Do While i > 0 ' Process msg Dim msg As String = Left$(ci.MessageIn, i - 1) WriteLog(ci.ToString() & " => " & msg) Dim msgOut As String = ProcessMessage(msg) If msgOut <> "" Then ci.Send(msgOut & _CommandTermination) WriteLog(ci.ToString() & " <= " & msgOut) End If ci.MessageIn = Mid$(ci.MessageIn, i + _CommandTermination.Length) i = InStr(ci.MessageIn, _CommandTermination) Loop ' Finally, read the next chunk of data ci.BeginReceive(AddressOf OnReceive) End Select Catch ex As Exception WriteLog(ex) Finally WriteLog(Clients.ToString()) RaiseEvent ConnectionList(Clients.ToList()) End Try End Sub Private Sub TimerTick(ByVal o As Object) ' loop by descending index to ensure no connections are missed ' Cannot use a for each loop as ConnectionInformation.Close() removes the connection from the clients list For i As Integer = Clients.Count - 1 To 0 Step -1 Dim ci As ConnectionInformation = Clients(i) Threading.Interlocked.Decrement(ci.Activity) If ci.Activity <= 0 Then ci.Close("Socket Inactivity Timeout") Next RaiseEvent ConnectionList(Clients.ToList()) End Sub Public Sub Close() WriteLog("Server Shutdown Starting") ' Close the listener WriteLog("Listener being closed") If _tcpServer IsNot Nothing Then _tcpServer.Close() _tcpServer = Nothing End If ' Stop the socket timer If _timer IsNot Nothing Then _timer.Dispose() ' Stop the clients; Don't use "for each" as closing a connection removes it from clients ' Closing a client connection has the side effect of removing the connection from the clients collection WriteLog("Closing Clients: " & Clients.ToString()) Do While Clients.Count > 0 Clients(0).Close("Server Shutting Down") Loop End Sub #Region " IDisposable Support " ' This code added by Visual Basic to correctly implement the disposable pattern. Protected Overridable Sub Dispose(ByVal disposing As Boolean) Close() End Sub Public Sub Dispose() Implements IDisposable.Dispose ' Do not change this code. Put cleanup code in Dispose(ByVal disposing As Boolean) above. Dispose(True) GC.SuppressFinalize(Me) End Sub #End Region Public Class ConnectionInformation Implements IDisposable Public MessageIn As String = "" ' Who's talking to me? Private _ClientName As String = "" Private _ClientPort As Integer = 0 ' Who am I Private _server As IPServer = Nothing ' Private structures and storage for sending and receiving data Private _sock As Socket = Nothing Private Const _bufSize As Integer = 1500 Private _bufIn(_bufSize) As Byte ' These two variables could be the source of a possible, but never seen, race condition Private _SendResults As IAsyncResult = Nothing Private _bufOut() As Byte ' TimeOut support; Async sockets don't directly support timeouts Public Activity As Integer = 0 Private _Activity As Integer = 0 Public Sub New(ByVal server As IPServer, ByVal sock As Socket, ByVal SocketTimeout As TimeSpan) Me.New(server, SocketTimeout) Me.sock = sock End Sub Public Sub New(ByVal server As IPServer, ByVal SocketTimeout As TimeSpan) _server = server If SocketTimeout <> Nothing Then _Activity = SocketTimeout.Minutes Activity = _Activity _server.Clients.Add(Me) End Sub Public Sub BeginReceive(ByVal callback As AsyncCallback) _sock.BeginReceive(_bufIn, 0, _bufSize, SocketFlags.None, callback, Me) Activity = _Activity End Sub Public Function EndReceive(ByVal asyncResult As IAsyncResult) As Integer Dim ci As ConnectionInformation = CType(asyncResult.AsyncState, ConnectionInformation) Try If ci._sock Is Nothing Then Return 0 ci.Activity = ci._Activity Dim bytesReceived As Integer = ci._sock.EndReceive(asyncResult) ci.MessageIn &= ASCII.GetString(ci._bufIn, 0, bytesReceived) Return bytesReceived Catch exSock As SocketException Dim sockErr As SocketError = exSock.SocketErrorCode Dim msg As String = [Enum].GetName(GetType(SocketError), sockErr) WriteLog("Socket Exception: " & msg, exSock) Return 0 Catch ex As Exception WriteLog(ex) Return 0 Finally ci.Activity = ci._Activity End Try Return 0 ' Forces the socket to close elsewhere in code End Function Public Sub Send(ByVal MessageOut As String) Try Activity = _Activity Do Until _SendResults Is Nothing OrElse _bufOut.Length = 0 _SendResults.AsyncWaitHandle.WaitOne() Loop _bufOut = System.Text.Encoding.ASCII.GetBytes(MessageOut) _SendResults = _sock.BeginSend(_bufOut, 0, _bufOut.Length, SocketFlags.None, AddressOf OnSend, Me) Catch ex As Exception WriteLog(ex) Finally Activity = _Activity End Try End Sub Private Sub OnSend(ByVal ar As IAsyncResult) Dim ci As ConnectionInformation = CType(ar.AsyncState, ConnectionInformation) ci.Activity = ci._Activity Try Dim bytesSent As Integer = ci._sock.EndSend(ar) ' Did we send the entire buffer? If bytesSent >= _bufOut.Length Then Array.Resize(_bufOut, 0) Else ' No; reset the buffer to contain only the bytes needed to be sent Dim tBuf(_bufOut.Length - bytesSent - 1) As Byte Array.Copy(_bufOut, bytesSent, tBuf, 0, tBuf.Length) _bufOut = tBuf ' Send them _SendResults = ci._sock.BeginSend(_bufOut, 0, _bufOut.Length, SocketFlags.None, AddressOf OnSend, ci) End If ci.Activity = ci._Activity Catch ex As Exception WriteLog(ci.ToString() & vbNewLine & ex.Message, True) Finally ci.Activity = ci._Activity End Try End Sub Public WriteOnly Property sock() As Socket Set(ByVal value As Socket) _sock = value If _sock Is Nothing Then _ClientName = "" _ClientPort = 0 Activity = 0 Else Dim ClientEndPoint As IPEndPoint = CType(_sock.RemoteEndPoint, IPEndPoint) _ClientName = Dns.GetHostEntry(ClientEndPoint.Address.ToString()).HostName _ClientPort = ClientEndPoint.Port Activity = _Activity End If End Set End Property Public ReadOnly Property ClientName() As String Get Return _ClientName End Get End Property Public ReadOnly Property ClientPort() As Integer Get Return _ClientPort End Get End Property Public Shadows Function ToString() As String Return _ClientName & ":" & _ClientPort.ToString() End Function Public ReadOnly Property ClientCount() As Integer Get Return _server.Clients.Count End Get End Property Public Function Close(ByVal Reason As String) As Integer WriteLog(RemoveSpaces(Reason & ": " & Me.ToString())) If _sock IsNot Nothing Then _sock.Shutdown(SocketShutdown.Both) _sock.Close() _sock = Nothing End If If _server.Clients.Contains(Me) Then _server.Clients.Remove(Me) Return _server.Clients.Count End Function #Region " IDisposable Support " ' This code added by Visual Basic to correctly implement the disposable pattern. Protected Overridable Sub Dispose(ByVal disposing As Boolean) Close("Server Disposing Connection") End Sub Public Sub Dispose() Implements IDisposable.Dispose ' Do not change this code. Put cleanup code in Dispose(ByVal disposing As Boolean) above. Dispose(True) GC.SuppressFinalize(Me) End Sub #End Region End Class Private Class ClientList Inherits SynchronizedCollection(Of ConnectionInformation) Public Function ToList() As List(Of String) Dim sa As New List(Of String) For Each ci As ConnectionInformation In MyBase.Items.ToArray() sa.Add(ci.ToString() & "/" & ci.Activity.ToString("#,##0")) Next Return sa End Function Public Overloads Function ToString() As String Dim sa As List(Of String) = Me.ToList() Dim s As String = "Connections: " & sa.Count.ToString("#,##0") For Each li As String In sa s &= vbNewLine & li Next Return s End Function End Class End Class
From: Peter Duniho on 18 Dec 2009 19:45 Michael Ober wrote: > [...] > Once again, thanks to everyone, especially Pete, who spent the time to > provide feedback. I have more. :) Here are some things that I noticed looking through the code you posted: -- There's a fair amount of redundant code, particularly with respect to the "Activity = _Activity" statement that appears a lot. Not that big of a deal, but you'll wish you'd made the "Activity" member a property when you see the next item. -- You have a threading bug in the handling of the "Activity" field. In particular, while you do use the Interlocked class to decrement the value, none of the assignments are protected in any way. So you have no guarantee that when you decrement the value, it's actually the correct, most recent value. Instead, any line that reads "Activity = _Activity" should read "Thread.VolatileWrite(Activity, _Activity)". This will ensure volatile semantics for the assignment. (Had you made "Activity" a property, you could've fixed it in one place, rather than having to go visit each and every statement in the code where "Activity" is used). -- It also appears to me that you have a numerical bug in the initialization of the "_Activity" field. In particular, you are using TimeSpan.Minutes, while I believe you really want TimeSpan.TotalMinutes (truncated to an integer, of course). As the code is now, if someone specifies a 90-minute timeout, you'll only wait 30 minutes before expiring the connection. -- You also have managed to incorrectly implement the ToString() method, in two completely different ways. :) You should make the method an "Overrides", but you have one place where you've specified "Shadows", and another place where you've specified "Overloads". -- In your ConnectionInformation.Close() method, there is no point in calling Contains(). It is not an error to call Remove() with an element not actually in the collection, and since half of the work of the Remove() method is scanning the list to find the element, which is the same work the Contains() method does, you effectively do that twice for no benefit to the code. -- You have left in the behavior I commented on before, with respect to having the per-connection "_SendResults" member, and a per-connection buffer. If done correctly, this would at worst be a potential performance issue, but because there's no synchronization around either the _SendResults member, nor the _bufOut member, you have a potential data corruption bug. You should either fix the design so that you have a per-i/o-operation data structure that isolates each operation from any other operation, or synchronize the code where you use the shared member variables. On that last point, note that just adding synchronization is probably fine. Generalizing the data structures so you have a per-i/o structure would make the code more flexible, but then you'd have to worry about keeping overlapped sends in the correct order, which is just one more complication you'll then need to code for. :) That's all I see for now. :) Pete
From: Michael Ober on 18 Dec 2009 21:58 "Peter Duniho" <no.peted.spam(a)no.nwlink.spam.com> wrote in message news:OaFSgSEgKHA.4528(a)TK2MSFTNGP06.phx.gbl... > Michael Ober wrote: >> [...] >> Once again, thanks to everyone, especially Pete, who spent the time to >> provide feedback. > > I have more. :) > > Here are some things that I noticed looking through the code you posted: > > -- There's a fair amount of redundant code, particularly with respect to > the "Activity = _Activity" statement that appears a lot. Not that big of > a deal, but you'll wish you'd made the "Activity" member a property when > you see the next item. > > -- You have a threading bug in the handling of the "Activity" field. In > particular, while you do use the Interlocked class to decrement the value, > none of the assignments are protected in any way. So you have no > guarantee that when you decrement the value, it's actually the correct, > most recent value. Instead, any line that reads "Activity = _Activity" > should read "Thread.VolatileWrite(Activity, _Activity)". This will ensure > volatile semantics for the assignment. (Had you made "Activity" a > property, you could've fixed it in one place, rather than having to go > visit each and every statement in the code where "Activity" is used). > The _Activity field is part of the timeout system. If it gets an inconsistent value, I don't think it's an an issue. It will either be decremented early, which will cause an early timeout, or reset to the initial value late, which will keep an inactive client connected longer. I had actually used Interlocked.Decrement originally in the TimerTick method, but couldn't find an equivalent for resetting this value. I didn't know about the VolatileWrite method. That said - here's the new code for setting and resetting Activity. I'm using Private _TimeRemaining As Integer = 0 Public Property Activity() As Integer Get Thread.VolitileRead(_TimeRemaining) End Get Set(ByVal value As Integer) Thread.VolitileWrite(_TimeRemainaing, value) End Set End Property C#'s volitile keyword would have made this easier. One question - how do the VolitileRead and VolitileWrite methods interact with the Interlocked.Decrement method used in the IPServer.TimerTick callback? > -- It also appears to me that you have a numerical bug in the > initialization of the "_Activity" field. In particular, you are using > TimeSpan.Minutes, while I believe you really want TimeSpan.TotalMinutes > (truncated to an integer, of course). As the code is now, if someone > specifies a 90-minute timeout, you'll only wait 30 minutes before expiring > the connection. > I hadn't even thought of this scenerio. The longest timeout I had used was 15 minutes. Fixed. Thanks. > -- You also have managed to incorrectly implement the ToString() method, > in two completely different ways. :) You should make the method an > "Overrides", but you have one place where you've specified "Shadows", and > another place where you've specified "Overloads". > This is unfortunately one of the things that VB allows. There is definitely a weaknesses here in the compiler itself. Another one that I find troublesome is that properties and methods can be referenced with or without the (), unlike C# that strictly enforces this. Fixed. > -- In your ConnectionInformation.Close() method, there is no point in > calling Contains(). It is not an error to call Remove() with an element > not actually in the collection, and since half of the work of the Remove() > method is scanning the list to find the element, which is the same work > the Contains() method does, you effectively do that twice for no benefit > to the code. > Updated. I was expecting an exception if the object wasn't already in the collection. RTM (Read The Manual). > -- You have left in the behavior I commented on before, with respect to > having the per-connection "_SendResults" member, and a per-connection > buffer. If done correctly, this would at worst be a potential performance > issue, but because there's no synchronization around either the > _SendResults member, nor the _bufOut member, you have a potential data > corruption bug. You should either fix the design so that you have a > per-i/o-operation data structure that isolates each operation from any > other operation, or synchronize the code where you use the shared member > variables. > > On that last point, note that just adding synchronization is probably > fine. Generalizing the data structures so you have a per-i/o structure > would make the code more flexible, but then you'd have to worry about > keeping overlapped sends in the correct order, which is just one more > complication you'll then need to code for. :) > SyncLock _outBuf ' All operations on _SendResults and _outBuf in the method. End SyncLock Since I can't send the next message until the previous one completes, any performance hit would already have been experienced simply by the wait loops that are in the code. > That's all I see for now. :) > > Pete
From: Peter Duniho on 19 Dec 2009 03:28 Michael Ober wrote: > The _Activity field is part of the timeout system. If it gets an > inconsistent value, I don't think it's an an issue. Without volatile semantics, in theory writes to the field might _never_ be visible to other threads. On x86, I believe this would happen only due to compiler optimizations, and given that it's a member field of a class, I'd guess that wouldn't happen. But I prefer code that's provably correct, rather than "probably won't break" in a specific environment. :) > [...] > C#'s volitile keyword would have made this easier. One question - how > do the VolitileRead and VolitileWrite methods interact with the > Interlocked.Decrement method used in the IPServer.TimerTick callback? I think it should be fine. You'll notice that Volatile...() methods only support data types that can be handled atomically. The Interlocked methods all do atomic operations as well. If you'd rather stick only with the Interlocked class, there is the Interlocked.Exchange() method which you can use to write to a variable, and the Interlocked.Read() method. But my recollection is that you have an atomicity guarantee for 32-bit values in .NET, so other than the race conditions that, as you say, aren't of concern, I don't see a problem mixing the APIs. By the way, as I scan through the code again, I see at least one more problem: you haven't synchronized the ClientInformation.Close() method. You could get an ObjectDisposedException if two different threads try to close the same object at the same time, as one reaches the _sock.Close() before the other reaches the _sock.Shutdown(). (I don't recall off the top of my head, but it's possible you can't even call Shutdown() twice with the SocketShutdown.Both value...but for sure, trying to call Shutdown() on a disposed/closed socket is not good). You should definitely put a lock around the Socket shutdown/closure code, so that only one caller to Close() ever finds the _sock field non-null. Related to this is the fact that the Close() method returns the new client count. I didn't bother to look at how this return value is used, but you should review that to make sure that having two different calls to the Close() method returning the same value is okay, and that having any given call to Close() return a value that is more than one less than what the client count was before the Close() call is also okay (the latter could happen if one client is removed in one thread, and another is removed by a different thread at the same time). Pete
From: Michael Ober on 19 Dec 2009 09:24
"Peter Duniho" <no.peted.spam(a)no.nwlink.spam.com> wrote in message news:emxP7UIgKHA.2160(a)TK2MSFTNGP02.phx.gbl... > Michael Ober wrote: >> The _Activity field is part of the timeout system. If it gets an >> inconsistent value, I don't think it's an an issue. > > Without volatile semantics, in theory writes to the field might _never_ be > visible to other threads. > > On x86, I believe this would happen only due to compiler optimizations, > and given that it's a member field of a class, I'd guess that wouldn't > happen. But I prefer code that's provably correct, rather than "probably > won't break" in a specific environment. :) > >> [...] >> C#'s volitile keyword would have made this easier. One question - how do >> the VolitileRead and VolitileWrite methods interact with the >> Interlocked.Decrement method used in the IPServer.TimerTick callback? > > I think it should be fine. > > You'll notice that Volatile...() methods only support data types that can > be handled atomically. The Interlocked methods all do atomic operations > as well. > > If you'd rather stick only with the Interlocked class, there is the > Interlocked.Exchange() method which you can use to write to a variable, > and the Interlocked.Read() method. But my recollection is that you have > an atomicity guarantee for 32-bit values in .NET, so other than the race > conditions that, as you say, aren't of concern, I don't see a problem > mixing the APIs. > > By the way, as I scan through the code again, I see at least one more > problem: you haven't synchronized the ClientInformation.Close() method. > You could get an ObjectDisposedException if two different threads try to > close the same object at the same time, as one reaches the _sock.Close() > before the other reaches the _sock.Shutdown(). (I don't recall off the > top of my head, but it's possible you can't even call Shutdown() twice > with the SocketShutdown.Both value...but for sure, trying to call > Shutdown() on a disposed/closed socket is not good). > In my logs, I actually see the ObjectDisposedException a few times a day. Hadn't had time to track it down, so thanks. I'll put a SyncLock Me before the test If _sock IsNot Nothing. Locking the whole object here is safer than locking just the _sock object since the _sock object may be nothing, which I don't think will support a lock. > You should definitely put a lock around the Socket shutdown/closure code, > so that only one caller to Close() ever finds the _sock field non-null. > > Related to this is the fact that the Close() method returns the new client > count. I didn't bother to look at how this return value is used, but you > should review that to make sure that having two different calls to the > Close() method returning the same value is okay, and that having any given > call to Close() return a value that is more than one less than what the > client count was before the Close() call is also okay (the latter could > happen if one client is removed in one thread, and another is removed by a > different thread at the same time). > The return value from Close isn't used anymore. If you still have the earlier code, it was used as the second argument to the NewConnection and ClosedConnection events. Removing it isn't an issue. > Pete Mike. |