Prev: VB.NET http web request with client certificate
Next: Is anyone able to make My.Settings work? I can't!
From: Peter Duniho on 14 Dec 2009 21:48
Michael Ober wrote:
> Peter,
>
> I think I found it this morning. It actually wasn't in the IPServer
> namespace at all.

Just as I suggested might be the case.

I'm glad you were able to find the problem. But I hope you can see,
based on this experience, why a concise-but-complete code example is so
important in terms of obtaining help. Without one, all anyone can do is
make random guesses and suggestions, none of which may have anything at
all to do with the problem.

While comments you received to your question might have been helpful in
a general sense, I doubt any of them were instrumental in leading you to
a solution to the specific problem you were asking about.

> [...]
> I think I corrected all but the _SendResults and _bufOut issues that you
> pointed out and I'm simply not sure how to fix that one, short of using
> SyncLock.

Nothing wrong with a SyncLock, IMHO. It's always most important to make
sure the code works right, then worry about whether there's a better,
more efficient solution. And .NET's Monitor class is reasonably efficient.

> [...]
> I also realized that creating a disconnect timer for each connection was
> extremely inefficient so I recoded this feature to use a single timer in
> the IPServer class and a decrement counter in each ClientInformation
> object.

If I were to code the timeout/disconnect logic from scratch, I might use
a sorted event queue, sorted by the event time and with a single thread,
and a call to Thread.Sleep() to delay until the next event. Add the
various logic to deal with resetting timers, adding new ones with a
future time earlier than the earliest current one in the queue, etc. and
you're done.

Fortunately, I believe that's basically what the System.Timers.Timer
timer does. It's specifically designed for server-related timers (or at
least, so the documentation says :) ), and I'm pretty sure one of its
main features is the ability to track a large number of timed events as
timers, without putting a significant load on the system.

Note that as compared to this approach:

> Since timeouts are always measured in minutes and the exact
> timeout period isn't critical, this change too hard to do. The
> decrement counter is set to the max inactivity time passed to the
> IPServer.New method and decremented once a minute by a timertick handler
> in the IPServer class itself. The Send and Receive methods of the
> ClientInformation class reset this counter to the time out value.

Advantage to your approach: reset of a timer is simply a new counter
value. Disadvantage to your approach: on every tick of the timer, you
have to inspect every connected client's data structure.

One downside to the sorted list of future events is the cost of the
insertion of a new element (either O(n) or O(log n), depending on the
implementation, which I don't know), borne every time you add a timer or
reset one. And of course, for large numbers of clients, this can be
large too.

Of course, if all the connections use the same timeout (which seems
likely), than any reset timer always winds up at the very end of the
sorted list, and the cost of doing that can be almost as cheap as
resetting a counter, because you already know where the new position is
in the list rather than having to search for it.

Obviously it depends somewhat on the specifics. But based on what I
think is likely about your implementation (specifically, every
connection uses the same timeout), you may find it preferable to
actually just create your own single-threaded event queue for the
timing, taking advantage of the fact that you know every new event added
(whether a new connection or one for which i/o operation happened and
thus needs a reset timer) goes to the end of the list.

I don't know for sure, but it's possible System.Timers.Timer includes an
optimization to check for end-of-list insertions, and so could in fact
perform basically as well as a custom implementation would. (I took a
quick look in Reflector, but unfortunately, the meat of the
implementation seems to be in native code, which doesn't show up in
Reflector; I can, however, verify that System.Timers.Timer just uses
System.Threading.Timer underneath, so whatever characteristics one has,
the other is pretty much the same :) ).

> Yes,
> there can be a race condition here, but if it occurs, one of two things
> will happen. Either the server will break down the connection, which
> still complies with the server's contract with its clients, or the
> connection timeout will be reset, which will simply keep the connection
> alive longer. In either case, if the client disconnects, the EndRecieve
> method of the socket will return 0 bytes and the connection will be
> shutdown by the server. [...]

Right. Timeouts always involve race conditions. The best you can do is
just make sure you have a way of knowing who won, and then get that part
right without corrupting your data. :)

Pete
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
First  |  Prev  |  Next  |  Last
Pages: 1 2 3 4 5
Prev: VB.NET http web request with client certificate
Next: Is anyone able to make My.Settings work? I can't!