-
Notifications
You must be signed in to change notification settings - Fork 343
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
Conversation
marcusklaas
commented
Aug 26, 2019
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
commented
Aug 27, 2019
Maybe CC @matklad? I know I've seen you chatting about fused things in the past.
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
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.
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.
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.
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.
Thanks for the explanation.
The select! in client_writer() uses fuse(), but the final change to the broker currently doesn't and won't compile.