Migrate to latest Tokio and Hyper #5
Conversation
| pub use hyper::client::HttpConnector; | ||
| #[cfg(feature = "rustls")] | ||
| pub type HttpsConnector = RustlsConnector<HttpConnector>; |
There was a problem hiding this comment.
These are re-exported so people can name them when they use the Client<C>
| reconnect_opts: ReconnectOptions, | ||
| } | ||
|
|
||
| /// Client that connects to a server using the Server-Sent Events protocol | ||
| /// and consumes the event stream indefinitely. | ||
| pub struct Client { | ||
| pub struct Client<C> { | ||
| http: hyper::Client<C>, |
There was a problem hiding this comment.
this is very cheap to clone and all clones will share a single connection pool
| } | ||
|
|
||
| fn backoff(&mut self) -> Duration { | ||
| fn backoff(mut self: Pin<&mut Self>) -> Duration { |
There was a problem hiding this comment.
made the receiver Pin because these only get called from poll_next and this makes some things easier
mraerino
force-pushed
the
refactor/tokio-hyper
branch
from
05c1e38 to
9424071
Compare
There was a problem hiding this comment.
Hi, @mraerino! I'm super excited you took the time to bring our code into the 21st century - this must have taken a fair amount of work.
Sorry for taking a really long time to respond to this PR. I wanted to take the time before responding to port our not-yet-public SDK over to the new APIs, and run our internal compatibility test suite against it. I'm pleased to report the SDK works fine with your branch, although it does appear to expose an existing bug.
At the high level I'm 100% on board with the goal and approach. I'd love to support the latest tokio/futures APIs, and be async/await-compatible.
This is complicated slightly by a bugfix (#6) I've been working on that conflicts with this change. I do want to land the bugfix first since it's a correctness issue, but I'm happy to help with resolving the merge conflict once it's in. I had a go at resolving the conflict myself but it's fairly involved, since both PRs make nontrivial changes to Decoded::poll.
Again, thank you for spending the time to do this and for contributing your work back - I'm excited to get this merged in.
src/client.rs
Outdated
| } | ||
| }; | ||
| self.as_mut().project().state.set(new_state); | ||
| self.poll_next(cx) |
There was a problem hiding this comment.
This is replacing a loop {} with a recursive call, correct? Is that the idiomatic way to write stream combinators in modern async Rust?
This is intuitively a little surprising to me, and according to a (very) brief Google search, tail call optimisation is possible but not guaranteed. We would obviously not want a possibility of stack overflow, given that these streams could be very long-lived (I believe we regularly see stream connections open for days).
Very willing to be convinced that I'm behind the times and this is a normal and reasonable thing to do :)
Similar question about Decoded::poll_next.
There was a problem hiding this comment.
I did some more research and found this article which also seems to suggest that TCO is not guaranteed, so I took the liberty of changing this back to a loop: eebd97b
|
Hi @mraerino, wanted to let you know that I ported my fix from #6 to this branch and resolved the merge conflicts (Github allows me to push to your branch because you opened a PR for it), so I'm happy to merge this in now! Thanks again for taking the time to make these improvements and for contributing your work back so other folks can benefit. |
Refactoring the Stream types to:
reqwestwithhyper&hyper-rustlsNotes: