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

Port into the std::future ecosystem. #73

Merged
merged 2 commits into from
Jan 9, 2020

Conversation

sapphire-arches
Copy link
Contributor

Sorry that this is a bit of a patch bomb, but it's mostly a straightfoward find-and-replace job to convert from futures-0.1 to std::future. I'm not sure it should get merged until the upstream libraries (hyper, futures, etc.) make real point releases rather than alphas, but given the size of the patch I figured getting it out here sooner rather than latter was probably a good idea =). As mentioned in the commit message, I'm also converting all the examples (both binaries and docs) from block_on_all to .await.

This looks like a terrifying amount of changes, but it's almost entirely
mechanical rewritting from futures-0.1 to std::future APIs. The major semantic
change is using #[tokio::main] for all of the examples and tests, instead of
the (no longer cleanly exposed) block_on_all. While we could have continued to
use block_on_all by linking directly against tokio-executor, .await syntax is
likely more representative of how future APIs will be consumed going forward.
@sapphire-arches
Copy link
Contributor Author

Oh, I forgot to mention it above but this is certainly a breaking change since the futures crate is part of the public API.

@sapphire-arches
Copy link
Contributor Author

sapphire-arches commented Sep 9, 2019

Looks like all the compilers in Appveyor are too old, and this change definitely violates the (implicit?) MSRV policy since it requires 1.39.

@adwhit
Copy link
Collaborator

adwhit commented Sep 24, 2019

Thanks for the PR! I definitely want to make this change but obviously not until the ecosystem is ready. I need to catch up a bit with tokio/hyper but I assume they will formally migrate to std::future soon after rustc 1.39 is released. Still we can probably make these changes before then and just hold off on the release. I will aim to review this week.

@sapphire-arches
Copy link
Contributor Author

No rush, I hacked this together over a weekend to shave some personal yaks.

Just to capture some of the thoughts I had during the process before they entirely leak out of my head, a few of the hand-implemented features behave a little bit oddly in the context of .await. .await syntax sort of assumes that once a future returns Async::Ready, polling it more won't do any more useful work. That's not really true of the UploadFuture, so maybe we should turn that into a stream. This blocks rewriting the manual state machines for the low-level TwitterFuture and RawFuture since async blocks and functions return unnameable types -- so we would need to either accept some boxing in UploadFuture or retain those manual implementations. I think this also affects most of the Streams too, since IIRC they also store futures directly. I'm not sure how much any of this matters since essentially none of this is exposed to users of the library and tolerating some uglyness internally for efficiency + better user ergonomics is basically what libraries exist for.

mre added a commit to hello-rust/hello that referenced this pull request Nov 11, 2019
Copy link
Collaborator

@adwhit adwhit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for doing this, and sorry it has taken me so long to take a look at it. The good news is that the ecosystem has settled down somewhat now so I think we can get this release out quite soon.

@adwhit adwhit merged commit 61fbb6c into egg-mode-rs:master Jan 9, 2020
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