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

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

clemensw
Copy link

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

@marcusklaas
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!

@jamesmunns
Copy link

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

@matklad
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?

@ghost
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.

@clemensw
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.

@matklad
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.

@matklad
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.

@clemensw
Copy link
Author

Thanks for the explanation.

@yoshuawuyts
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.

@yoshuawuyts yoshuawuyts added the documentation Improvements or additions to documentation label Sep 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants