Closing socket with less exception, less verbose

[GoogleCodeExporter]

What steps will reproduce the problem?
1. Create a RedisSubscriberConnection instance.
2. Open and Subscribe.
3. Dispose connection after a wlhile.

What is the expected output? What do you see instead?
No exception occurs in normal execution.
ObjectDisposedException or NullReferenceException occurs within 
RedisConnectionBase.OnDispose, one of those lines:

socket.Shutdown(SocketShutdown.Both);
socket.Close();
socket.Dispose();

Of cource, your code catches all exceptions but we don't expect any throw if 
possible.

What version of the product are you using? On what operating system?
1.3.38 and 1.3.39 in Windows 7/Windows Server 2008 R2

Please provide any additional information below.

This is obvious multithread problem.
Don't nullify field after check. Check after nullify and then proceed disposal 
invocations. Better code is:

var socket = Interlocked.Exchange(this.socket, null);
if (socket != null)
{
try
{
Trace("dispose", "closing sockrt...");
socket.Shutdown(SocketShutdown.Both);
socket.Close();
socket.Dispose();
Trace("dispose", "closed socket");
}
catch (Exception ex)
{
Trace("dispose", ex.Message);
}
}

Original issue reported on code.google.com by tcae...@gmail.com on 15 Oct 2013 at 3:35

[GoogleCodeExporter]

Interlocked.Exchange(ref this.socket, null);

Original comment by tcae...@gmail.com on 15 Oct 2013 at 3:47

[GoogleCodeExporter]

The Socket.Close() method calls Dispose internally. The above will correctly 
handle race conditions for the dispose method, however, it does not handle the 
general case.

Original comment by agcastl...@gmail.com on 30 Oct 2013 at 2:57

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Shutdown -> Close -> Dispose sequence is as same as 1.3.39 trunk.
The point of change is calling Interlocked.Exchange to prevent to access the 
socket by other thread.

194,203c194,204
<             try { if (socket != null) {                
<                 Trace("dispose", "closing socket...");
<                 socket.Shutdown(SocketShutdown.Both);
<                 socket.Close();
<                 socket.Dispose();
<                 Trace("dispose", "closed socket");                
<             } } catch (Exception ex){
<                 Trace("dispose", ex.Message);
<             }
<             socket = null;
---
>             var socket = Interlocked.Exchange(ref this.socket, null);
>             if (socket != null) {
>                 try {
>                     Trace("dispose", "closing socket...");
>                     socket.Shutdown(SocketShutdown.Both);
>                     socket.Close();
>                     Trace("dispose", "closed socket");                
>                 } catch (Exception ex){
>                     Trace("dispose", ex.Message);
>                 }
>             }

Original comment by tcae...@gmail.com on 20 Nov 2013 at 12:14

[GoogleCodeExporter]

The reason to modify this is about first chance exception. My colleagues and I 
usually enable "Break When an Exception is Thrown" for Common Language Runtime 
Exceptions because we don't want any exception if usual. In the 1.3.39 trunk, 
RedisConnectionBase.OnDispose can be invoked by multiple threads:

* RedisConnectionBase instance is disposed by user context. Then,
* AsyncReadCompleted spots end of stream and shutdown and dispose socket.

This simultaneous executions always occur whenever you call Dispose method of 
RedisConnectionBase which leads debugger to break by throwing exception. 
Interlocked.Exchange guarantees the only one thread executing OnDispose method 
can access "socket" field. Because an ObjectDisposedException is thrown by 
calling Dispose() on socket instance twice, this doesn't happen anymore; 
because a NullReferenceException is thrown by accessing socket after nullify 
field, this doesn't happen anymore, either.

Original comment by tcae...@gmail.com on 20 Nov 2013 at 3:03

[GoogleCodeExporter]

This is addressed in StackExchange.Redis, the successor to BookSleeve

Original comment by marc.gravell on 20 Mar 2014 at 12:09

• Changed state: WontFix