rust-lang / futures-rs

Zero-cost asynchronous programming in Rust
https://rust-lang.github.io/futures-rs/
Apache License 2.0
5.34k stars 616 forks source link

futures-util: Update Future combinators to preserve Clone #2742

Open zjijz opened 1 year ago

zjijz commented 1 year ago

This updates most Future combinators to preserve Clone when available on the base Future.

For motivation, imagine you have some complicated Future that is not Clone and requires .shared() to properly share it. Then imagine you have a library function that is meant to bundle together a bunch of combinators to fulfill some semantic purpose. That library function will have to call .shared() if it wants to try to guarantee the return Future is Clone, but this might be suboptimal if the input Future was already Clone, plus it obfuscates the .shared() allocation. With this change, you can instead require Future + Clone on the input Future and have a guarantee the output will be Clone as well.

The hold-out Future implementations are:

For the hold-outs, the existing pattern of using .shared() allows for Clone, and follows the intended semantics of those combinators.

Some combinators that might not make the most sense to preserve Clone:

If these changes make sense, I think it would also make sense to apply them to Stream combinators as well (although I don't see myself utilizing this property as much with them). If that is the case, these Future -> Stream combinators make sense to preserve Clone.

Tested:

zjijz commented 1 year ago

2743 is a separate fix for the missing #[derive(Debug)] warning.

zjijz commented 1 year ago

@taiki-e Any thoughts on this?

This doesn't seem to impose any oddities for me when using the await path to consume a Future (possible wrapped in combinators) since all cloning would have to happen before the Future was ever polled once.

However, with the ability to easily poll with things like FutureExt::poll_unpin or Box::pin + Future::poll, I'm worried that some of the Future implementations weren't written with the possibility of cloning in mind and might clone a Future's intermediate state instead of fully recreating the initial version (which might be what people expect, but seems to be an edge case when I think about it). Lazy is one example of this that if cloned after a call to poll_unpin would return a Lazy that always panics on the next poll.

taiki-e commented 1 year ago

Thanks for the PR. Which future combinator is actually needed for your use case?

zjijz commented 1 year ago

Map is the big one for my use case. I'm not sure the current implementation lends itself to Clone being thrown on it to where it can potentially panic if there was a clone thrown in between or after manual polls. I can also write my own Map that is less panicky about polling after completion if you don't want to expose the potential footgun here.