feat(client): introduce lower-level Connection API #1454
Conversation
| /// Polls to determine whether this sender can be used yet for a request. | ||
| /// | ||
| /// If the associated connection is closed, this returns an Error. | ||
| pub fn poll_ready(&mut self) -> Poll<(), ::Error> { |
There was a problem hiding this comment.
Is the intent for this that you'd call it when pulling a connection out of a pool?
There was a problem hiding this comment.
That is a fine time to do so, or even if you're not using a pool, and just have a single connection, it lets you know if the connection could process it right now. I think I need to alter this PR slightly so that it returns NotReady for h1 connections that aren't pipelining. It would also return NotReady if this were an h2 connection and you'd reached SETTINGS_MAX_CONCURRENT_STREAMS .
There was a problem hiding this comment.
If you did try to send a request, would that just be queued up on the connection becoming ready, or would it be an error?
There was a problem hiding this comment.
If the connection weren't ready, you'd get an error. We start using this poll_ready -> send pattern in the h2 crate, and it felt very natural, and it's what Sink is changing to in futures 0.2.
There was a problem hiding this comment.
Ah cool, makes sense. I was thinking it wouldn't be an error, so I was a bit confused :)
| /// upgrade. Once the upgrade is completed, the connection would be "done", | ||
| /// but it is not desired to actally shutdown the IO object. Instead you | ||
| /// would take it back using `into_parts`. | ||
| pub fn poll_without_shutdown(&mut self) -> Poll<(), ::Error> { |
There was a problem hiding this comment.
Couldn't poll know that the connection's upgraded and not shut it down?
There was a problem hiding this comment.
It could, but that's probably a trap, since you may not choose to do anything special with a Connection, and just call exe.spawn(conn.map_err(|e| debug!("conn error: {}", e)). If you did that, and poll did what poll_with_shutdown does in this PR, then the transport would never have shutdown called.
If Connection::Item were Parts, then that could probably be easier, since then you'd need to handle the Parts in some way (even if just dropping) when spawning the future. But, in order to do that, Connection needs to hold the internal state in an Option, so it can remove it, and thus every poll requires a self.inner.as_mut().unwrap(). I figured by providing poll_without_shutdown, Connection::poll isn't bothered, and anyone could wrap poll_with_shutdown and an Option if they wanted that kind of API.
Does that reasoning seem convincing?
There was a problem hiding this comment.
FWIW storing an Option and .as_mut().unwrap()'ing it is super common in futures/stream combinators so it wouldn't be that crazy. Seems fine to have two methods though.
src/client/conn.rs
Outdated
| /// | ||
| /// Default is true. | ||
| #[inline] | ||
| pub fn keep_alive(&mut self, enabled: bool) -> &mut Builder { |
There was a problem hiding this comment.
Does this inject a Connection: close or just cause poll to return ready after the first response comes in?
There was a problem hiding this comment.
Currently it tells the internal h1::Conn that after the first response, it cannot be used for a new request, and sets its state as Closed. This is the same as disabling keep_alive on the Client as well.
I've sometimes thought that it should probably set Connection: close, to be nice, but then, even if the header isn't set, once a response has been read, it's up to the client to use it again, or close the connection.
Interestingly, I think this option would do nothing if the connection were HTTP2. Or maybe it could send a GOAWAY after the request...
There was a problem hiding this comment.
Given that it wouldn't do anything for h2, would it make more sense to not have this method at all and have users set Connection: close to manage keep alive? Hyper does pay attention if that header is set on outbound requests I think, right?
seanmonstar
force-pushed
the
client-connection
branch
12 times, most recently
from
55c1d17 to
030ddd8
Compare
seanmonstar
force-pushed
the
client-connection
branch
from
030ddd8 to
1207c2b
Compare
Adds the API described in #1449.
Closes #1449