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

Refactor to reduce the amount of unsafe and duplicated code. #2128

Merged
merged 4 commits into from
May 6, 2020

Conversation

Diggsey
Copy link
Contributor

@Diggsey Diggsey commented Apr 12, 2020

  • Replace uses of pin_utils with the safer pin_project crate
  • Implement macros to allow building new combinators from existing ones more
    easily
  • Add an internal fns module as a workaround until impl Trait is usable in more places.
  • Get rid of Chain as the building block for future combinators, and instead
    build from Map and Flatten primitives
  • Delete a lot of code which is no longer necessary.

There should not be any breaking changes as I have endeavoured to keep all type signatures exactly the same.

@Diggsey Diggsey force-pushed the snip branch 4 times, most recently from 62d6656 to 81e2baf Compare April 12, 2020 22:02
@cramertj
Copy link
Member

Wow, at a first glance this looks like a great improvement! Sadly though, this change is pretty large and exceeds my review capacity at the moment. I'll try to give it a read-through in the next week or so, but if either @Nemo157 or @taiki-e are available to take a look I'd greatly appreciate it :)

@Diggsey
Copy link
Contributor Author

Diggsey commented Apr 22, 2020

Rebased to catch up with master.

Copy link
Member

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really great!

I've reviewed changes in most combinator implementation and public methods.
The following has not been reviewed yet:

  • Whether the behavior of the delegated combinator has changed (i.e., removed files + details of fns.rs)
  • Changes in trait bounds of the public struct definition and implementations

futures-util/src/sink/with.rs Show resolved Hide resolved
futures-util/src/stream/stream/filter.rs Outdated Show resolved Hide resolved
futures-util/src/stream/try_stream/try_filter.rs Outdated Show resolved Hide resolved
futures-util/src/future/future/flatten.rs Outdated Show resolved Hide resolved
futures-util/src/future/try_future/try_flatten.rs Outdated Show resolved Hide resolved
futures-util/src/future/try_future/try_flatten_err.rs Outdated Show resolved Hide resolved
futures-util/src/future/try_future/mod.rs Outdated Show resolved Hide resolved
futures-util/src/stream/stream/mod.rs Outdated Show resolved Hide resolved
- Replace uses of `pin_utils` with the safer `pin_project` crate
- Implement macros to allow building new combinators from existing ones more
  easily
- Get rid of Chain as the building block for future combinators, and instead
  build from `Map` and `Flatten` primitives
- Delete a lot of code which is no longer necessary.
@Diggsey
Copy link
Contributor Author

Diggsey commented Apr 24, 2020

Fixed an issue with the rebase and created a new commit with the changes as per review.

Comment on lines 83 to 84
if ready!(fut.poll(cx)) {
pending_fut.set(None);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if ready!(fut.poll(cx)) {
pending_fut.set(None);
let yield_item = ready!(fut.poll(cx));
pending_fut.set(None);
if yield_item {

I mean "if fut.poll returns Ready", not "if ready!(fut.poll) returns true" (I'm sorry if the comment was hard to understand)

Comment on lines 81 to 82
if ready!(fut.poll(cx)) {
pending_fut.set(None);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if ready!(fut.poll(cx)) {
pending_fut.set(None);
let yield_item = ready!(fut.poll(cx));
pending_fut.set(None);
if yield_item {

Copy link
Member

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that some trivial trait implementations were added by delegate_all macro. I'd prefer to remove these implementations as they may inadvertently become valid when the internal type implementation changes.
Also, similarly, I'd prefer to remove stream/sink implementations on future combinators other than flatten-family and to remove future implementations to stream combinators. These are not intended uses and may prevent change for optimizations in the future.

futures-util/src/stream/stream/mod.rs Outdated Show resolved Hide resolved
futures-util/src/stream/stream/mod.rs Outdated Show resolved Hide resolved
futures-util/src/stream/try_stream/mod.rs Outdated Show resolved Hide resolved
futures-util/src/stream/try_stream/mod.rs Outdated Show resolved Hide resolved
futures-util/src/stream/try_stream/mod.rs Outdated Show resolved Hide resolved
futures-util/src/future/try_future/mod.rs Outdated Show resolved Hide resolved
futures-util/src/future/try_future/mod.rs Outdated Show resolved Hide resolved
futures-util/src/future/try_future/mod.rs Outdated Show resolved Hide resolved
futures-util/src/future/try_future/mod.rs Outdated Show resolved Hide resolved
futures-util/src/future/try_future/mod.rs Outdated Show resolved Hide resolved
@taiki-e
Copy link
Member

taiki-e commented Apr 25, 2020

Hmm... looks like CI says that the behavior of TryFutureExt::{and_then, or_else} has changed: https://travis-ci.com/github/rust-lang/futures-rs/jobs/323173386#L340-L345

@taiki-e
Copy link
Member

taiki-e commented Apr 25, 2020

Ok, the error is due to the old future is no longer dropped before the closure is run.
This is what TryChain did (#1069 (comment)), but TryFlatten does not handle closure, so this needs to be done in the Map.

Specifically, it needs to change the future field of Map to Option<Fut> and replace it with None before running the closure:

    #[project]
    fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<T> {
        #[project]
        let Map { mut future, f } = self.project();
        let fut = future.as_mut().as_pin_mut().expect("Map polled after completion");
        let output = ready!(fut.poll(cx));
        future.set(None); // Drop the old futures before the closure runs.

        let f = f.take().unwrap();
        Poll::Ready(f.call_once(output))
    }

@Diggsey
Copy link
Contributor Author

Diggsey commented Apr 25, 2020

@taiki-e Yeah, I spotted that issue on the last CI run. I would like to avoid increasing the size of Map, so unfortunately it looks like I'd have to reintroduce a Chain-style abstraction for these combinators. However, implementing Chain would be easier if pin_project were to support the project_replace method we discussed previously, that way I could avoid an extra Option.

I had a look at what it would take to implement such a method in pin_project and it's surprisingly difficult - there are some awkward edge-cases around what happens if the destructor of a pinned field panics, so short term I will go with the Option approach, and open an issue in the unsafe-code-guidelines repo to see what the contract should be for panicking pinned destructors...

@Diggsey
Copy link
Contributor Author

Diggsey commented Apr 25, 2020

Eh, I'm being stupid - Map already has an Option for the FnOnce field, so I can just get rid of that and turn the whole struct into an enum without increasing the size at all, and then everything will compose nicely with no special casing 😄. The only problem is that this again relies on project_replace, but luckily I think there's an alternative I can use for this specific case.

@Diggsey Diggsey force-pushed the snip branch 3 times, most recently from d7dc71b to 208c281 Compare April 26, 2020 15:15
@Diggsey
Copy link
Contributor Author

Diggsey commented Apr 26, 2020

@taiki-e I believe I have addressed all the issues and the tests are now passing.

I introduced some unsafe code in Map: this would be removable if project_replace functionality is added to pin_project, but is required for the moment. However, I have tested it against MIRI and it passes.

Co-Authored-By: Taiki Endo <te316e89@gmail.com>
@taiki-e
Copy link
Member

taiki-e commented May 3, 2020

Also, this PR adds the following APIs:

  • FutureExt::map_into
  • future::try_maybe_done
  • TryFutureExt::ok_into
  • TryFutureExt::try_flatten

I feel map_into and ok_into may not be needed, but I don't have a strong opinion on this, so I leave it to other reviewers.

@cramertj
Copy link
Member

cramertj commented May 6, 2020

This is outstanding work! I took a quick read through and I'm really excited to get this in. Thanks for all your hard work, @Diggsey, and for the detailed review, @taiki-e! There is quite a bit of code change here, so I'll do a release later this week to start getting this out to folks and make sure that there isn't any accidental breakage lurking.

@cramertj cramertj merged commit 3bf5ac9 into rust-lang:master May 6, 2020
@Diggsey Diggsey deleted the snip branch May 12, 2020 21:37
@taiki-e taiki-e mentioned this pull request Oct 14, 2020
6 tasks
exrook pushed a commit to exrook/futures-rs that referenced this pull request Apr 5, 2021
Refactor to reduce the amount of unsafe and duplicated code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants