Read can hang because socket has infinite read timeout

[tekhedd]

Socket defaults to an infinite receive/send timeout. On a flaky internet connection, a half-open connection (ESTABLISHED but never closed presumably because of hard failures or edge router not sending RST) will leave the session open, and the connection will hang forever on any Read() of the socket data.

(This results in a lost listener thread in my server app... at this point I only have one flaky remote connection, but eventually I will run out of threads!)

Unfortunately, the conditions are difficult to reproduce manually, as network stacks are designed to aggressively avoid this sort of situation. What I do to reproduce it.

• Set up a listener on a port
• Open a connection to that listener, and send data so that a reader is waiting for a Read() call
• use iptables to block SYN,ACK in and RST out on the client, so that the session is effectively hard disconnected
• Wait a long time.
• Stop the listener, and watch the stack trace

Stack trace will be something along the lines of:

System.IO.IOException: Unable to read data from the transport connection: A blocking operation was interrupted by a call to WSACancelBlockingCall. ---> System.Net.Sockets.SocketException: A blocking operation was interrupted by a call to WSACancelBlockingCall
   at System.Net.Sockets.Socket.Receive(Byte[] buffer, Int32 offset, Int32 size, SocketFlags socketFlags)
   at System.Net.Sockets.NetworkStream.Read(Byte[] buffer, Int32 offset, Int32 size)
   --- End of inner exception stack trace ---
   at System.Net.Sockets.NetworkStream.Read(Byte[] buffer, Int32 offset, Int32 size)
   at SocketHttpListener.Net.RequestStream.Read(Byte[] buffer, Int32 offset, Int32 count)
   at System.IO.Stream.InternalCopyTo(Stream destination, Int32 bufferSize)
   at System.IO.Stream.CopyTo(Stream destination)
   at (My HandlePost handler)(HttpListenerContext ctx)

I believe this could be averted by setting the socket's ReceiveTimeout and SendTimeout properties (I will experiment with this next). Maybe this could be set to the same as the other HttpConnection timeouts (5 minutes? I guess that makes sense for keepalive sessions.) The read timeout should probably be the same then, although maybe not the write timeout.

[tekhedd]

On further consideration, setting the read timeout shorter would be a bad thing for HTTP keepalive sessions, so it makes more sense for the client of the listener to manage body-read timeouts using async operations with cancellation (or some other mechanisms). However, the infinite timeout is a definite issue -- TCP connections never close unless at least one end of the connection explicitly terminates them.

Adding socket settings to the HttpConnection constructor lets abandoned connections eventually close.

 sock.SendTimeout = s_timeout;
 sock.ReceiveTimeout = s_timeout;

This should probably be set in the Endpoint manager, but the timeout variable is convenienty located here in the HttpConnection already.

[tekhedd]

Sorry to spam you with analysis. Looks like the .NET framework's ReadWriteTimeout defaults to 300s, so maybe I'll see if I can duplicate that behavior, as that would be consistent and unsurprising, I guess. Would certainly make my life easier. :)

[tekhedd]

Two possible changes come to mind:

1. set socket send/receive timeouts
2. Implement CanTimeout/ReadTimeout/WriteTimeout in Request/ResponseStream (optional)

And possibly a third:

3. Isn't 5-minutes a bit long for an HTTP keepalive session?

1: According to the docs, NetworkStream read/write timeouts default to 300000ms, and CanTimeout returns true. This default is wrong, possibly based on obsolute or platform-dependent Socket defaults. In any case, Infinite is a problem in real server applications. (Certainly it has ruined my week. :) )

2: (Edited) timeouts only apply to synchronous operations, so the values can be the same

2: may not work if NetworkConnection does not implement the Timeout functionality

There is code in HttpConnection to manage the read timeout, which simply closes the socket on timeout. This only protects the "waiting for the next HTTP message" loop, and is not complete protection against half-open connections even if clients of the listener never read or write. Outside of this loop, there is no protection against abandoned sockets, and it should be posible to hang the listener itself if the socket is abandoned during a SendResponse type operation.

3: 1 or 2 minute timeout is more common these days, but 5 used to be the thing and I don't see a problem with that. The code does not appear to use the keepalive timeout supplied with the HTTP headers, so a 5 minute keepalive is hard coded. I suppose I could add just the bits of TimeoutManager required to support a configurable timeout, but it's Friday!

Final note: Do I really need to go rewrite all my client code to use async I/O? I guess I do. Darn it!

[tekhedd]

#16 The smallest PR I've ever made.

After all that analysis, I think a small tweak to cover the worst case scenario is the best approach.