Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Book: Select wants a FusedFuture for broker() in tutorial #105

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

Open
clemensw wants to merge 1 commit into async-rs:main
base: main
Choose a base branch
Loading
from clemensw:patch-1

Conversation

Copy link

@clemensw clemensw commented Aug 23, 2019

The select! in client_writer() uses fuse(), but the final change to the broker currently doesn't and won't compile.

marcusklaas reacted with thumbs up emoji
Copy link

Ran into this as well. It would be great if the tutorial briefly went into what these fuses are doing and why they are required too!

Copy link

Maybe CC @matklad? I know I've seen you chatting about fused things in the past.

Copy link
Member

matklad commented Aug 27, 2019

cc #13 and #40 :)

Fuse is definitely a pain point. #13 and #40 apply surgical fixes to it, but the underlying problem that one must call .fuse manually remains. It would be ideal if, instead of explaining the current quirks, we could just design a better API that doesn't require .fuse.

Suggested course of action:

  • merge impl fused future for Next #13 as it's a simple fix
  • write an RFC/issue for general stream integration and fuseing (cc @stjepang @yoshuawuyts ). My tentative proposal: just make all streams&futeres exposed by async-std fused. It seems adding fuse for futures is much cheaper than for iterators, so it's ok to always do this?

Copy link

ghost commented Aug 27, 2019

It would be ideal if, instead of explaining the current quirks, we could just design a better API that doesn't require .fuse.

I think the "proper" solution would be to port crossbeam-channel to futures and provide a select macro that works with async channels.

Copy link
Author

While improving the story around .fused() is desireable, merging this PR would make the example in the book compile (and be consistent with previous examples), which would help lower confusion for newbies like me.

Copy link
Member

matklad commented Aug 28, 2019

Unfortunately, the fix in this PR is not correct: as per select’s doc, fused should be called outside of the select loop, and this is a pretty bad footgun. Really, the underlying future is fused, we should just make sure it is reflected in the type system as in #13.

Copy link
Member

matklad commented Aug 28, 2019

To be more precise, the patch in this PR definitely makes the code work, but it doesn’t use select/fuse at they are supposed to be used, and I’d rather not do that in the docs.

Copy link
Author

Thanks for the explanation.

Copy link
Contributor

#208 and #187 add macro support for non-fused streams and futures respectively. I think this will allow us to bypass a lot of the problems with futures-rs's select blocks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
documentation Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /