search

MediaBrowser / SocketHttpListener

A standalone HttpListener with support for SSL, WebSockets and Mono
MIT License
42 stars 14 forks source link

Set socket timeout in HttpConnection for blocking I/O. #16

Closed tekhedd closed 7 years ago

tekhedd commented 7 years ago

Sets the timeout for blocking Read()/Write() to the same value as the default read timeout for connections.

LukePulverenti commented 7 years ago

Hi, thanks for this! Is there some kind of unit test that can be used to simulate it? Would this have an impact on long running requests?

tekhedd commented 7 years ago

Yes, I realize I need to create tests for this. (Sorry! Of course I'm not supposed to be working on it at all, and I'm on Linux so the tests don't run (or even load in Monodevelop, and there's no xcopy, etc). Last week was a bit of a crunch with the code hanging forever on a live installation. :) ) The ability to set the timeout is necessary for testing--you're certainly not going to wait for a 5 minute timeout when running unit tests. But it should be possible to test with a short timeout and a simple socket client. I'll have free time to look into it in about... what month is it? Somewhere around late December. :( OK fine, I wonder if I have the right version of Dev Studio installed... )

A read/write timeout should be OK for long-running HTTP sessions, as it only affects the amount of time the socket will wait for each Read or Write request. ISS limits keepalive to 2 minutes by default, whereas Apache defaults to 15s, so 5m is certainly long enough for most normal applications. If you have a "long poll" HTTP client (to simulate push) that expects an infinite timeout, that could be a problem. But, to handle long poll applications it might be good to be able to set the timeout even longer or even to infinity (ack don't do this), but for most server applications 5m would be considered a "very long" time. Whereas for my app 1-5s would be a better keepalive timeout, and 30-60s would be a good read/write timeout, as I have too many idle TCP connections and some flaky internet connections.

So, for both testing and flexiblity, making both the socket and keepalive timeouts configurable would be good. The timeouts should be separate because socket timeout is a failsafe while keepalive is not. IMO hard coding 5m for the socket fallback is acceptable, but the hard coded keepalive is pretty limiting.

Ideally, the connection would consider the client's requested HTTP KeepAlive header as a timeout on a per-connection basis. Web servers are supposed to use the lesser of the builtin keepalive or the client's requsted value, but I think for self-hosting REST servers the rules can be safely ignored. And to be honest, it's simply easier to code a timeout in the server: no per-connection value to remember. I would call this a wishlist feature.

tekhedd commented 7 years ago

I do have MSVS2012 installed and have written a test. Unfortunately it takes 5 minutes to run (and we need more than 1 test), so I'll have to implement at least some simple timeout setters. I'm on it but things are a bit hectic at the moment. :)

tekhedd commented 7 years ago

Because you asked for unit tests, I'm creating a bunch of tests and a partial implementation of HttpListenerTimeoutManager. I hope you're proud of yourself. :)