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

Add a loom aware block_on for running futures to completion #148

Closed
wants to merge 1 commit into from

Conversation

faern
Copy link
Contributor

@faern faern commented May 17, 2020

This solves tokio-rs/tokio#2463

Adding a way to run a Future to completion inside a loom model. Allows using loom to test the correctness of Future::poll implementations.

Do you want to have this in loom? If not, I might publish it as loom-executor or something along those lines. But the further upstream it can be the better IMO.

Feedback on the implementation greatly appreciated!

@faern
Copy link
Contributor Author

faern commented May 17, 2020

Since I first wrote this as a standalone crate, I had to rely only on the public API of loom. But I bet it can be implemented more efficiently/better with some internals if it were to be inside loom like this PR suggests. Probably something like park/unpark. But I'm going to wait for some feedback before putting any effort into that.

@LucioFranco
Copy link
Member

I think it would make sense to include a loom executor in here, maybe even behind a futures feature flag.

@faern
Copy link
Contributor Author

faern commented Jun 16, 2020

Hmm.... Wait a minute. There already is a future module containing a block_on 😮 Doesn't this already do exactly what this PR implements?

I have completely missed it, because it's not in the docs.rs documentation. We should probably make it build the documentation with all features enabled. Otherwise the extra features become very hard to discover.

@LucioFranco
Copy link
Member

@faern yeah, that is probably also a good idea too :)

@hawkw
Copy link
Member

hawkw commented Jun 16, 2020

Should probably also add the doc_cfg attributes to show the required feature flags in the docs. :)

@faern
Copy link
Contributor Author

faern commented Jun 19, 2020

I can do that. Do you want me to pull in the doc_cfg dependency to make it work on stable? Or go with the solution tokio has where it passes --cfg docsrc only on docs.rs and relies on nightly for just that?

Personally I would not be a huge fan of pulling in quote and proc_macro2 into the dependency tree just for such a simple thing. But since you mentioned that crate name I thought you might prefer it. On the other hand tokio probably have the other solution for a reason 🤷

@hawkw
Copy link
Member

hawkw commented Jun 19, 2020

I can do that. Do you want me to pull in the doc_cfg dependency to make it work on stable? Or go with the solution tokio has where it passes --cfg docsrc only on docs.rs and relies on nightly for just that?

Personally I would not be a huge fan of pulling in quote and proc_macro2 into the dependency tree just for such a simple thing. But since you mentioned that crate name I thought you might prefer it. On the other hand tokio probably have the other solution for a reason shrug

I think it's better to do what Tokio does and just enable the feature on nightly only --- I wasn't actually aware of the doc_cfg crate until you mentioned it, I was just referring to the RustDoc feature.

@faern
Copy link
Contributor Author

faern commented Jun 20, 2020

Replaced by #151... Kind of

@faern faern closed this Jun 20, 2020
hawkw added a commit that referenced this pull request May 19, 2022
The fact that there is a `futures` feature flag and a `block_on` method
was a bit hard to discover. At least for me. That lead me to create
#148. Turns out it was not needed.

To make it more discoverable, this PR makes docs.rs build the
documentation with all features active, so the extra functionality they
enable are visible. It also uses the unstable `doc_cfg` attribute to
make it more obvious in the docs that those items are feature gated.

This is taken more or less directly from how the tokio crate does the
same thing.

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
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.

None yet

3 participants