Set timeouts immediately after accepting a connection #1006
Conversation
`HttpListener` will be required to manage timeouts soon, which requires additional fields.
| // This default implementation is only here to prevent the addition of | ||
| // these methods from being a breaking change. They should be removed | ||
| // when the next breaking release is made. | ||
| unimplemented!(); |
There was a problem hiding this comment.
Since these methods are called unconditionally later, anyone just upgrading hyper will find that suddenly, their custom NetworkConnector panics, which would be a breaking change. These should probably just do nothing, instead of calling unimplemented!.
If we want, we could put a warn! or even error! in here instead, so people looking at logs can see they should implement these. Or we could just do nothing.
There was a problem hiding this comment.
You're right. I didn't think this through.
I think a warn! would be appropriate. Doing nothing would just mean that timeouts would be silently ignored, which could be very surprising to both users and implementers of custom NetworkConnectors. An error! seems too strong to me, as nothing is actually going wrong (yet) at this point.
Currently, timeouts are set in `Worker::handle_connection`, _after_ the stream has been returned by `Listener::accept`. This is not quite right, as a `Listener` implementation that wraps `HttpListener` might do I/O in its own `accept` implementation. For that I/O, no timeouts have been set yet. This is not a purely theoretical concern either, as `HttpsListener` can do this, depending on the implementation of `SslServer` it uses. I suspect, but could not yet confirm, that this behavior is responsible for issue #950. As of this commit, the timeouts are set by `HttpListener`, directly after the connection has been accepted. Please note that `Worker` also still sets the timeouts. This redundancy will be cleaned up in the following commits.
As of the previous commit, timeouts are set directly after accepting the connection in `HttpListener::accept`. This commit removes the code that still redundantly set it in `Worker::handle_connection`. `Worker` still does some read timeout setting related to keep-alive. That code remains untouched.
|
Pushed the fixes suggested by @seanmonstar. Also adjusted a misleading commit title (replaced "fix(net)" with "fix(server)"). |
|
Oh right, PS, if its at all possible to test that this does help #950 using a git dependency, that'd be great, so we can know if releasing |
This is a possible solution for #950. I'm not running this in production yet, and thus could not confirm whether it really solves the problem. I thought I'd submit it anyway, so it can get some review. Whether or not this really is a solution for #950, I believe it fixes a real problem and should be merged in any case.
I've struggled and failed to come up with an automated test to verify that this does what it intends to do. Since there doesn't seem to be a test for timeout behavior currently, and this is a relatively straight-forward change, I decided to submit it without an automated test.
Please note that this only affects the server-side code. I haven't checked whether the same problem is present client-side.
Another note: This pull request sets the default read and write timeouts to
None, which I believe preserves the current behavior. This might not be a good default for a server-side application, so some thought should be put into whether to change this.A reminder to me, or in case I forgot, anyone else who might read this: This pull request adds default implementations of trait methods that should be removed in the next release which includes breaking changes (see comments in code). It would probably be a good idea to open an issue as a reminder, in case this pull request is merged.