Skip to content
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

what's the expected way to detect a timeout error #626

Closed
softprops opened this issue Aug 8, 2015 · 26 comments
Closed

what's the expected way to detect a timeout error #626

softprops opened this issue Aug 8, 2015 · 26 comments

Comments

@softprops
Copy link
Contributor

I noticed recently timeouts were recently added which is fantastic. However, when I took this feature for a spin I noticed something that was confusing. Error handling wasn't documented or consistent between https and http requests. The pull for std lib rfc impl referenced ErrorKind.TimedOut and ErrorKind.WouldBlock in its tests for timeout failures. When I took hypers support for a spin

#![feature(duration)]
extern crate hyper;
use hyper::Client;
use std::io;
use std::time::Duration;
fn main() {
  let mut client = Client::new();
  client.set_read_timeout(Some(Duration::from_millis(2)));
  let mut res = client.get("https://google.com").send().unwrap();
  let _ = io::copy(&mut res, &mut io::stdout()).unwrap();
}

errors with

thread '<main>' panicked at 'unexpected error ErrorWantRead with ret -1', ~/.cargo/registry/src/gh.neting.cc-0a35038f75765ae4/openssl-0.6.4/src/ssl/mod.rs:996
#![feature(duration)]
extern crate hyper;
use hyper::Client;
use std::io;
use std::time::Duration;
fn main() {
  let mut client = Client::new();
  client.set_read_timeout(Some(Duration::from_millis(2)));
  let mut res = client.get("http://google.com").send().unwrap();
  let _ = io::copy(&mut res, &mut io::stdout()).unwrap();
}

errors with

thread '<main>' panicked at 'called `Result::unwrap()` on an `Err` value: Io(Error { repr: Os { code: 35, message: "Resource temporarily unavailable" } })', ../src/libcore/result.rs:732

would it be possible for hyper to document a consistent error thrown on read/write timeouts?

Again, having support for timeouts is great, not having a clear path for handling them, not so much.

@softprops softprops changed the title what's the expected away to detect a timeout error what's the expected way to detect a timeout error Aug 8, 2015
@seanmonstar
Copy link
Member

Interesting. I haven't looked to see how timeouts are implemented on TcpStreams. Seems something conflicts with the optimizations openssl is doing? @sfackler

@sfackler
Copy link
Contributor

sfackler commented Aug 8, 2015

It's a bug in SslStream's error handling. I will push a fix.

@sfackler
Copy link
Contributor

sfackler commented Aug 8, 2015

This should fix it: sfackler/rust-openssl@69cbd14

Can you run the test against that and see if it resolves the issue?

@softprops
Copy link
Contributor Author

I'm not sure I can run that down stream. I hit this build error

native library `openssl` is being linked to by more than one package, and can only be linked to by one package

  openssl-sys v0.6.4 (https://github.com/sfackler/rust-openssl#69cbd145)
  openssl-sys v0.6.4

this may be related to that?

@softprops
Copy link
Contributor Author

scratch that. I just needed to turn off hypers default-features flag to force my local dep to take priority

Now when I run the same code for an https url I get a different error

#![feature(duration)]

extern crate hyper;

use hyper::Client;
use std::io;

use std::time::Duration;

fn main() {
  println!("test");
  let mut client = Client::new();
  client.set_read_timeout(Some(Duration::from_millis(10)));
  let mut res = client.get("https://google.com").send().unwrap();
  let _ = io::copy(&mut res, &mut io::stdout()).unwrap();
}
thread '<main>' panicked at 'called `Result::unwrap()` on an `Err` value: Io(Error { repr: Custom(Custom { kind: InvalidInput, error: StringError("Invalid scheme for Http") }) })', ../src/libcore/result.rs:732

I suspect because I just opted out of hyper's default features in my build

@seanmonstar
Copy link
Member

@softprops yep, if you turn off the ssl feature, then the DefaultConnector is HttpConnector. I believe you can override the path of the openssl using .cargo/config.

@softprops
Copy link
Contributor Author

I can't seem to the right combination of cargo config options make this happen. Can you provide an example?

@seanmonstar
Copy link
Member

According to http://doc.crates.io/config.html

paths = ["../openssl"] in a .cargo file next to your Cargo.toml.

You can put it anywhere, and adjust the path.

On Sat, Aug 8, 2015, 7:33 PM doug tangren notifications@github.com wrote:

I can't seem to the right combination of cargo config options make this
happen. Can you provide an example?


Reply to this email directly or view it on GitHub
#626 (comment).

@softprops
Copy link
Contributor Author

okay. worked it out by cloning rust-openssl next to my project then creating a .cargo/config file with

paths = ["/abs/path/to/rust-openssl"]

now the errors are without https

thread '<main>' panicked at 'called `Result::unwrap()` on an `Err` value: Io(Error { repr: Os { code: 35, message: "Resource temporarily unavailable" } })', ../src/libcore/result.rs:732

and with https

thread '<main>' panicked at 'called `Result::unwrap()` on an `Err` value: Io(Error { repr: Custom(Custom { kind: TimedOut, error: StringError("socket read timed out") }) })', ../src/libcore/result.rs:732

👍 @sfackler

@seanmonstar thoughts aligning the non ssl timeout request err with a similar err kind? Ideally something like io::ErrorKind::TimedOut would make it easier to handle these errors consistently

@softprops
Copy link
Contributor Author

For what it's worth

35 seems to be mapped to 2 errors in libc. In rusts io module these seem to map to a ErrorKind::WouldBlock so I'm not sure if this mapping is actually a bug in rust itself. TimedOut errors are mapped here to the ETIMEDOUT const in libc. os code 60 ( shrugs ).

suggestions?

@seanmonstar
Copy link
Member

@softprops according to http://linux.die.net/man/7/socket a timed out read will report the error EAGAIN/EWOULDBLOCK (they're the same constant, as you've noticed).

@seanmonstar
Copy link
Member

I don't yet know what the best solution is. It may be worth discussion in internals about whether TcpStream should map WouldBlock to TimedOut iff the socket has a timeout set. Socket timeouts are still unstable, so these details may not have had enough testing.

@seanmonstar
Copy link
Member

I tried asking in the original RFC: rust-lang/rfcs#1047 (comment)

@softprops
Copy link
Contributor Author

Cool thanks

@sfackler
Copy link
Contributor

I picked TimedOut basically arbitrarily. OpenSSL doesn't formally expose the underlying error so I had to just pick one. It might be the case that errno is preserved, in which case I can just return io::Error::last_os_error(), but that doesn't appear to be guaranteed anywhere in the docs.

@seanmonstar
Copy link
Member

@sfackler certainly not an expert in openssl's source, but my browsing found this: https://github.com/openssl/openssl/blob/master/ssl/ssl_lib.c#L2391

I couldn't find a check for the os error, so it could be completely ignored? Either way, it'd be nice if the returned error was the same as that from a TcpStream. I supposed I can add a check in Request and Response to translate WouldBlock into TimedOut, since I know (for now) that the socket is in blocking mode.

@sfackler
Copy link
Contributor

Ok, try now.

@softprops
Copy link
Contributor Author

@sfackler I tried my little test app with your latest commit and now get a consistent error for both https and http requests in hyper.

thread '<main>' panicked at 'called `Result::unwrap()` on an `Err` value: Io(Error { repr: Os { code: 35, message: "Resource temporarily unavailable" } })', ../src/libcore/result.rs:732

I would prefer a TimedOut error to an opaque Os error if possible

@sfackler
Copy link
Contributor

The Os error isn't visible in the API, just the Debug output. io::Error has a kind() method which will return an io::ErrorKind variant that you can look at, though on Linux it'll be WouldBlock rather than TimedOut.

@seanmonstar
Copy link
Member

To make it consistent, I'd be fine with hyper matching WouldBlock and
returning TimedOut inside Request and Response (unless that'd somehow seem
weird to Linux users).

On Mon, Aug 10, 2015, 8:08 PM Steven Fackler notifications@github.com
wrote:

The Os error isn't visible in the API. io::Error has a kind() method
which will return an io::ErrorKind variant that you can look at, though
on Linux it'll be WouldBlock rather than TimedOut.


Reply to this email directly or view it on GitHub
#626 (comment).

@sfackler
Copy link
Contributor

That would probably seem weird to linux users.

@softprops
Copy link
Contributor Author

Is the something that could be stablized in the feature flagged socket timeouts in rusts std lib?

@seanmonstar
Copy link
Member

@softprops is what something that could be stabilized?

@softprops
Copy link
Contributor Author

The error to expect when setting read write timeouts. I'm really excited to see this implemented but I'm finding little in the way of docs for how to handle timeout errors

@seanmonstar
Copy link
Member

@softprops after lots of reading man pages and stack overflow questions, it seems the standard is this:

  • On Unixy systems, read() with a timeout will return EAGAIN, which in Rust is ErrorKind::WouldBlock.
  • On Windows, read() with a timeout will return WSAETIMEDOUT, which in Rust is ErrorKind::TimedOut.

Pick what you need. It may be worth filing an issue on the rust repo to include that in the documentation of timeouts, but it might not.

@softprops
Copy link
Contributor Author

Ok. I know its common in some rustdocs to have a panic or errors section to document how to handle errors. I'll see if I can't find someway to get these documented somewhere outside of gh issues comments and pull diffs :)

mateusmedeiros added a commit to mateusmedeiros/lgster that referenced this issue Apr 10, 2021

Verified

This commit was signed with the committer’s verified signature.
mateusmedeiros Mateus Medeiros
It seems we can't universally trust ErrorKind::WouldBlock to be returned
when our short-timeout-workaround ends in the tcp response because only
unix-like systems will return that.

On Windows the io::Error raised will actually be ErrorKind::TimedOut,
which actually makes sense, so `¯\_(ツ)_/¯`

Props to @seanmonstar for all his "lots of reading man pages and stack
overflow questions" that he mentioned in this comment
hyperium/hyper#626 (comment)

You don't know me, man, but may all of your wishes come true during your
tenure upon this earth, because I could've pulled out a hell of a lot
more hairs than I did if it wasn't for that comment 😅
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants