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

Consider removing .boxed() combinators and the Box aliases #228

Closed
carllerche opened this issue Nov 4, 2016 · 8 comments
Closed

Consider removing .boxed() combinators and the Box aliases #228

carllerche opened this issue Nov 4, 2016 · 8 comments

Comments

@carllerche
Copy link
Member

.boxed() has to be hard coded to either be Send + 'static or not. Where as calling Box::new is able to infer which it should be. Given this, when a user tries to call boxed() but the future is not Send, a compilation error happens. I've noticed that this often causes confusion and it seems to be one of the more common questions that is asked in the gitter channel.

I propose to remove boxed in favor of explicitly calling Box::new.

@alexcrichton alexcrichton added this to the 0.2 release milestone Nov 4, 2016
@alexcrichton
Copy link
Member

Agreed we should consider, but I don't think this should block the 0.2 release, we can deprecate and message at any time.

@zimond
Copy link

zimond commented Nov 29, 2016

I think the problem becomes a problem when a function needs to return some futures. Say I have a function which could do async job A, B or C, and return one of them. Using a Box to wrap the future is the only way I could see as a function must return only ONE implementation of trait in Rust.

As the tuturial suggests using .boxed(), it is very easy for a newbie (like me) to box futures here and there. Encountering Send problem later.

And it's not so obvious as a fact that tokio-core is running on a single thread. The idea of "Oh I don't need Send, maybe there is a way to remove that bound" is not a thought that could easily come to my head.

@jimmycuadra
Copy link
Contributor

Here's an example of someone getting tripped up by this: hyperium/hyper#1082

@stbuehler
Copy link
Contributor

I once had an issue when the combined future type was too big and rustc refused to compile it with some strange "the size of ... cannot be statically determined" message. Boxing in the middle fixed it, and I really liked being able to put .to_box(); putting Box::new(..) around it would have made it much more unreadable. (to_box() is a local trait extension which doesn't require Send).

So I'd like to point out that if you don't provide boxing methods others will probably extend it locally anyway.

Maybe some of the current work on impl (default, specializiation?) would allow to provide a trait extending future based on whether the future was Send, Sync and automatically using an appropriate box type?

As an alternative I'd really like to see a boxed_unsync() method :)

@izolyomi
Copy link

I've also hit this issue and as a Rust newbie it took me a really lot of time to understand and solve the problem. The errors about the Send trait are absolutely misleading when it's otherwise not required for your use case and the solution is to simply ignore the builtin boxed() function and use Box::new().

@jimmycuadra
Copy link
Contributor

I noticed MaidSafe uses an extension trait with an into_box method that works as a non-Send version of boxed. Maybe instead of removing boxed, it could be split into two methods, into_box and into_sendable_box (names to be bikeshed, of course) to provide both versions and make the distinction more clear.

@alexcrichton
Copy link
Member

I think to me it's relatively clear at this point that we'll be removing this method. If there's pretty solid agreement on that point we could go ahead and add a #[deprecated] tag to it, and maybe recommend, if it's used, to have a local extension trait and/or local method.

alexcrichton added a commit that referenced this issue Aug 15, 2017
These methods have caused far more confusion than they were ever intended to
cause, and they're use rarely enough in practice "to success" that they're
likely to be removed from any 0.2 release. As a result this commit starts out
the removal process by deprecating them and removing usage internally.

Closes #228
alexcrichton added a commit that referenced this issue Aug 15, 2017
These methods have caused far more confusion than they were ever intended to
cause, and they're use rarely enough in practice "to success" that they're
likely to be removed from any 0.2 release. As a result this commit starts out
the removal process by deprecating them and removing usage internally.

Closes #228
@alexcrichton
Copy link
Member

I've opened a PR for their removal in #556

rillian added a commit to rillian/sccache that referenced this issue Oct 10, 2017
In futures 0.1.15, the boxed() method on Future structs was
deprecated. This returned a BoxFuture type which had to be
declared either Send or 'static while wrapping a Future
in Box::new() allows inference of the appropriate trait bounds.

We already don't use the BoxFuture trait, so just call Box::new()
directly instead to avoid the deprecation warning and make it
easier to port to future releases.

See also rust-lang/futures-rs#228
luser pushed a commit to mozilla/sccache that referenced this issue Oct 11, 2017
In futures 0.1.15, the boxed() method on Future structs was
deprecated. This returned a BoxFuture type which had to be
declared either Send or 'static while wrapping a Future
in Box::new() allows inference of the appropriate trait bounds.

We already don't use the BoxFuture trait, so just call Box::new()
directly instead to avoid the deprecation warning and make it
easier to port to future releases.

See also rust-lang/futures-rs#228
linkmauve added a commit to linkmauve/tokio-socks5 that referenced this issue May 13, 2018
The former is deprecated since futures 0.1.15, and has been removed in
futures 0.2.

See rust-lang/futures-rs#228
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

No branches or pull requests

6 participants