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

feat(client): add connect, read, and write timeouts to client #1255

Closed
wants to merge 1 commit into from

Conversation

hjr3
Copy link
Contributor

@hjr3 hjr3 commented Jul 12, 2017

  • Add a TimeoutConnector implememtation that can be used to wrap any
    implemention of Connect.
  • Add read and write timeout support to Client and client Config.
    The internal client implementation now wraps I/O with the
    tokio-io-timeout crate. Due to the way tokio-proto works, the user
    will see a "broken pipe" error instead of a "timed out" error when
    a read or write timeout occurs.

Closes #1234

- Add a `TimeoutConnector` implememtation that can be used to wrap any
  implemention of `Connect`.
- Add read and write timeout support to `Client` and client `Config`.
  The internal client implementation now wraps I/O with the
  tokio-io-timeout crate. Due to the way tokio-proto works, the user
  will see a "broken pipe" error instead of a "timed out" error when
  a read or write timeout occurs.

Closes hyperium#1234
@seanmonstar
Copy link
Member

It looks like this can work entirely out-of-crate? I kind of lean towards keeping it that way. Is there something that having this in crate allows that wouldn't be possible otherwise?

As a side note, looking at the tokio-io-timeout crate, the implementations of AsyncRead and AsyncWrite don't pass forward the read_buf and write_buf methods, which would mean readv and writev are no longer used for TcpStream.

@hjr3
Copy link
Contributor Author

hjr3 commented Jul 13, 2017

@seanmonstar TimeoutConnector can be out of crate. I don't see how the read and write timeouts would be out of crate. That being said, I was looking at other issues where you were talking about the ClientProto onion. Maybe that is a better way to introduce some of this functionality.

@seanmonstar
Copy link
Member

Can't the read and write timeouts be set in any connector by returning a TimeoutStream?

@hjr3
Copy link
Contributor Author

hjr3 commented Jul 13, 2017

Of course! Not sure why I did not think of that earlier. Ok, I will make a separate crate and add all of the logic there.

@hjr3 hjr3 closed this Jul 13, 2017
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

Successfully merging this pull request may close these issues.

2 participants