smol-rs / futures-lite

Futures, streams, and async I/O combinators.
Apache License 2.0
451 stars 26 forks source link

Implement combinators needed to migrate `prodash` to `futures-lite` #2

Closed Byron closed 3 years ago

Byron commented 4 years ago

As I am working on a venerable dual core MacBook Air, I will always be mindful of compile times, trying my best to reduce them in all my crates.

Futures-lite seems like it could make a difference in prodash, so I tried to migrate. Right now, I would be missing the following capabilities to finish the transition:

Even though I could add boxed* myself (or could have worked around it), I feel way less confident about contributing implementations for the above without having a conversation first and most certainly guidance.

Do you consider adding those at some point and/or would you be open to contributions for these?

Thanks a bunch!

Edit: I will get started on implementing these unless @stjepang gets to them more quickly.

ghost commented 4 years ago

Yes, I intend to add those methods :) It's just that adding them is kind of boring manual work so in the first version I only implemented the combinators I personally needed.

Byron commented 4 years ago

I am glad to hear that, as it means I can delve into contributing these soonish. It would be my first 'manual' implementation of combinators, and I am a little bit afraid of the unknown. But on the other hand, this is certainly the safest place to get started with it, so I will open a WIP PR for these once I get started on these to avoid duplicate work and allow early feedback.

Wish me luck 😅

ghost commented 4 years ago

So the way I add these combinators is I look at what futures-rs and async-std have and then take whichever implementation/docs look better :)

Byron commented 4 years ago

That sounds easy enough! The devil lurks in the detail, but I am sure it will work. Thanks for the hint :), always appreciated!

Byron commented 4 years ago

Since futures-lite 1.4, I managed to get rid of all futures_util usages, except for a single use of StreamExt::select_all(…). The latter could be emulated with something like StreamExt::race(…) I presume. Maybe race for streams could be implemented similarly.

ghost commented 4 years ago

@Byron I'm curious -- do you perhaps have an example use case for stream::select_all()?

I wonder if race() and race!() should be enough. Do you think we'd also need race_all()?

Byron commented 4 years ago

I used select_all to merge three streams into one, which in my case is fine with stream::or (it’s ok that it is biased).

Personally I prefer stream::all over select_all even, but some might be interested in a fair race_all of some sort.

Sent from my iPhone

On 16. Sep 2020, at 22:04, Stjepan Glavina notifications@github.com wrote:

 @Byron I'm curious -- do you perhaps have an example use case for stream::select_all()?

I wonder if race() and race!() should be enough. Do you think we'd also need race_all()?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

ghost commented 4 years ago

I have now also added stream::race() and StreamExt::race() so you can use that if you need fairness. It's just published in v1.5.0.

Byron commented 4 years ago

Thanks for going the extra mile and making the APIs more symmetric! Now the only feature I seem to be missing something akin to futures::join_all(). Despite me trying really hard, I was unable to emulate this with Task::spawn and awaiting all tasks in order. Which is odd, as join_all(…) probably is not much more than a balanced version of that.

Byron commented 4 years ago

For me it's ok to close this issue, the join_all part is just used in the dashboard example of prodash, thus won't cause extra dependencies for prodash consumers.

ghost commented 4 years ago

Hmm you can actually do join_all() using streams:

let results: Vec<_> = stream::iter(my_futures).then(|f| f).collect().await;

I should add this to the documentation.

Byron commented 4 years ago

Right, this is what streams are doing after all, it's quite a heureka moment for actually, so that I never saw join_all is actually a somewhat simple stream.

What I really like about futures-lite is that it doesn't just blindly copy an API that people are used to, but actually improves the naming for making the tradeoffs more obvious (i.e. or and race).

It would certainly be great to have that documented, and I wish there was a way to make this show if people search the crate docs for join_all or similar.

Byron commented 3 years ago

I am closing this issue as there really is nothing more to do here - spring cleaning :D.