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

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

zjijz
Copy link

@zjijz zjijz commented May 8, 2023

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:

  • Remote / RemoteHandle due to their use of futures_channel::oneshot::{Sender, Receiver}. This seems like it is by design that these cannot be Clone.
  • JoinAll / TryJoinAll due to their use of Stream combinators, but also that it seems unlikely that people would expect them to offer Clone since they are used to perform a potentially costly sync barrier that would probably be desired to happen only once.

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:

  • IntoStream
  • TryFlattenStream

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:

  • cargo doc.
  • cargo fmt.
  • cargo test --all-features.

@zjijz zjijz force-pushed the zjijz/future-combinators-preserve-clone branch from 5e2f791 to 9f35f08 Compare May 8, 2023 06:17
@zjijz zjijz changed the title Update Future combinators to preserve Clone futures-util: Update Future combinators to preserve Clone May 9, 2023
@zjijz zjijz force-pushed the zjijz/future-combinators-preserve-clone branch from 9f35f08 to 9ecc6b8 Compare May 9, 2023 00:03
@zjijz
Copy link
Author

zjijz commented May 9, 2023

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

zjijz added 2 commits May 8, 2023 23:01
Fixes this error on ToT:
```
error: type does not implement `Debug`; consider adding `#[derive(Debug)]` or a manual implementation
   --> futures-util/src/compat/compat01as03.rs:325:1
    |
325 | struct WakerToHandle<'a>(&'a task03::Waker);
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `-D missing-debug-implementations` implied by `-D warnings`
```

Tested:
- `cargo fmt`.
- `cargo doc`.
- `cargo test`.
This updates most `Future` combinators to preserve `Clone` when available on the input `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 funciton 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 has the ability to obfuscate and hide 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:
- `Remote` / `RemoteHandle` due to their use of `futures_channel::oneshot::{Sender, Receiver}`. This seems like it is by design that these cannot be `Clone`.
- `JoinAll` / `TryJoinAll` due to their use of `Stream` combinators, but also that it seems unlikely that people would expect them to offer `Clone` since they are used to performa a  potentially costly sync barrier that would probably be desired to happen only once.

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`:
- `IntoStream`
- `TryFlattenStream`

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:
- `cargo doc`.
- `cargo fmt`.
- `cargo test --all-features`.
@zjijz zjijz force-pushed the zjijz/future-combinators-preserve-clone branch from 9ecc6b8 to ceee10f Compare May 9, 2023 06:24
@zjijz
Copy link
Author

zjijz commented May 13, 2023

@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
Copy link
Member

taiki-e commented Jun 9, 2023

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

@taiki-e taiki-e added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author label Jul 19, 2023
@zjijz
Copy link
Author

zjijz commented Jul 25, 2023

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.

@taiki-e taiki-e added S-needs-decision Status: A decision on whether or not to do this is needed. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author labels Jul 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-needs-decision Status: A decision on whether or not to do this is needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants