Prev: VB.NET http web request with client certificate
Next: Is anyone able to make My.Settings work? I can't!
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.

First  |  Prev  | 
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!