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.

Sign up for GitHub

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

Jump to bottom

Remove StreamObj, caution against using FutureObj #1494

Merged
merged 1 commit into from
Apr 24, 2019
Merged

Remove StreamObj, caution against using FutureObj #1494

merged 1 commit into from
Apr 24, 2019
+47 −293

Conversation

Nemo157
Copy link
Member

@Nemo157 Nemo157 commented Mar 24, 2019

Fixes #1352

cramertj
cramertj reviewed Apr 15, 2019
View reviewed changes
futures-core/src/future/mod.rs
#[cfg(feature = "alloc")]
/// An owned dynamically typed [`Future`] for use in cases where you can't
/// statically type your result or need to add some indirection.
pub type BoxFuture<'a, T> = Pin<alloc::boxed::Box<dyn Future<Output = T> + Send + 'a>>;
Copy link
Member

Choose a reason for hiding this comment

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

I'd also like to see non-Send versions of these-- maybe BoxFutureLocal? unsure-- we can sort that out in a separate change :)

Copy link
Member

Choose a reason for hiding this comment

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

(similarly, adding a boxed_local would be great)

@cramertj
Copy link
Member

Why not remove FutureObj as well?

cramertj
cramertj reviewed Apr 15, 2019
View reviewed changes
futures/tests/futures_unordered.rs Outdated
@@ -68,8 +67,8 @@ fn finished_future() {
let (c_tx, c_rx) = oneshot::channel::<i32>();

let mut stream = futures_unordered(vec![
FutureObj::new(Box::new(a_rx)),
//FutureObj::new(Box::new(b_rx.select(c_rx))),
a_rx.boxed() as BoxFuture<_>,
Copy link
Member

Choose a reason for hiding this comment

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

hmm... I kind of think we should change the default return of boxed to the trait-object version to make this cast unnecessary. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good to me, if users want a non-trait-object they can call Box::new themselves.

Copy link
Member Author

Choose a reason for hiding this comment

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

One issue with this is that BoxFuture can't actually be directly converted to FutureObj because of #1535, which is why https://github.com/rust-lang-nursery/futures-rs/pull/1494/files#diff-57ad78f21e9043ebe29659ff5d335774R9 was necessary. I've got a fix for #1535 ready to open once this is merged (it requires an enhancement to the Pin API so will be blocked on a rust PR).

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, can you clarify-- is #1535 blocked on this change, or is this change now blocked on #1535 ? What's the change to Pin? Do you have a PR open / issue open already for that?

Copy link
Member Author

Choose a reason for hiding this comment

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

This change exacerbates the issue from #1535 since FutureExt::boxed now returns BoxFuture and FutureObj::new can’t take a BoxFuture since it doesn’t support boxed unsized values.

Nemo157@b436178 is the change I’m referring to which will allow converting a BoxFuture to a FutureObj, that’s the one using the Pin api changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, the API changes I was referring to are rust-lang/rust#60165, they can be worked around similar to the old code if we want to get a stable release out for 1.36.

Copy link
Member

Choose a reason for hiding this comment

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

In that case, would you like to go ahead and land this change, or would you prefer to hold off?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think landing this as is is fine, I'll open a PR for that other change straight away so it can be merged once rust-lang/rust#60165 is (or updated to work around it), I just didn't do it as a parallel PR as it would definitely have merge conflicts with this one.

Copy link
Member

Choose a reason for hiding this comment

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

sg, merged this one.

@cramertj
Copy link
Member

Apologies for the delay in reviewing this!

@cramertj
Copy link
Member

(I need to keep a better eye on the repo-- or perhaps we should set up something bors-like so that I can get notifications-- I guess pinging me on the thread would also accomplish this goal)

@Nemo157
Copy link
Member Author

Nemo157 commented Apr 15, 2019

FutureObj is still needed for no_std support with Spawn.

--

Having bors here would also be useful for merging multiple PRs (I assume you're using the admin override to merge without CI passing, I can only merge a PR if it's up to date with master because of CI limitations).

@Nemo157 Nemo157 force-pushed the remove-obj branch 2 times, most recently from 2bf5e98 to 69cf4ab Compare April 15, 2019 18:12
This was referenced Apr 18, 2019
Closed
Closed
@cramertj cramertj merged commit 3b67140 into rust-lang:master Apr 24, 2019
@Nemo157 Nemo157 deleted the remove-obj branch April 24, 2019 21:41
@chmln chmln mentioned this pull request Apr 27, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cramertj cramertj cramertj left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove StreamObj and soft-deprecate FutureObj
2 participants
@Nemo157 @cramertj