-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
feat(server): Make sleep_on_error duration configurable and enable it per default #1457
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
src/server/mod.rs
Outdated
@@ -683,14 +685,14 @@ impl Stream for AddrIncoming { | |||
return Ok(Async::Ready(Some(AddrStream::new(socket, addr)))); | |||
}, | |||
Err(ref e) if e.kind() == io::ErrorKind::WouldBlock => return Ok(Async::NotReady), | |||
Err(ref e) if self.sleep_on_errors => { | |||
Err(ref e) if self.sleep_on_errors.is_some() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this can be rewritten as:
Err(ref e) if let Some(delay) = self.sleep_on_errors => {
so you don't need to use .unwrap()
later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried that, but the compiler does not allow let
in a match arm, as far as I can see.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be done like this, right?
match self.listener.accept() {
// Ok ...
Err(e) => {
if let Some(delay) = self.sleep_on_errors {
// continue or delay
}
return Err(e);
}
}
The test fail on AppVeyor does not seem related to this change. |
/// This can be disabled by setting the duration to None. | ||
/// | ||
/// Default is 10ms. | ||
pub fn sleep_on_errors(&mut self, duration: Option<Duration>) -> &mut Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We couldn't make this change for 0.11.x, but could be in 0.12.
src/server/mod.rs
Outdated
@@ -683,14 +685,14 @@ impl Stream for AddrIncoming { | |||
return Ok(Async::Ready(Some(AddrStream::new(socket, addr)))); | |||
}, | |||
Err(ref e) if e.kind() == io::ErrorKind::WouldBlock => return Ok(Async::NotReady), | |||
Err(ref e) if self.sleep_on_errors => { | |||
Err(ref e) if self.sleep_on_errors.is_some() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be done like this, right?
match self.listener.accept() {
// Ok ...
Err(e) => {
if let Some(delay) = self.sleep_on_errors {
// continue or delay
}
return Err(e);
}
}
a815d74
to
a420ebd
Compare
@seanmonstar good idea with the |
05fee87
to
48fc125
Compare
So, the changes look right! Thanks! However, as mentioned in a line comment, this is a breaking change, so couldn't be done in 0.11. I'd say this could be a PR against the 0.12.x branch, but with that switching to the new tokio, I'm unsure where timeouts will come from at the moment... |
I think we can put this on hold until the new Tokio is integrated and then rewrite this wherever it fits in. |
With #1488 merged, a variant of this is now in place. Thanks! |
For issue #1455 .
This is a minor API break because I'm changing the type of the sleep_on_error() method. This method exists only for a very short amount of time and Hyper is pre-1.0 so changing this might be ok?