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

Remove StreamObj and soft-deprecate FutureObj #1352

Closed
Nemo157 opened this issue Nov 27, 2018 · 10 comments · Fixed by #1494
Closed

Remove StreamObj and soft-deprecate FutureObj #1352

Nemo157 opened this issue Nov 27, 2018 · 10 comments · Fixed by #1494

Comments

@Nemo157
Copy link
Member

Nemo157 commented Nov 27, 2018

For some prior context see #1348

Now that futures and streams are object safe there should be no reason to use FutureObj/StreamObj directly. There are currently two reasons I know of to use these:

  1. Passing around owned dynamically typed futures/streams. This can be replaced with use of the aliases below.
  2. Spawning futures via Spawn::spawn_obj. Rather than using this directly most users can use SpawnExt::spawn instead to automatically convert the future into a FutureObj (the only exception I know of is no_std use).

(am I missing any other use cases? I admit I've personally never used FutureObj)

Proposed additions to futures:

type BoxFuture<'a, T> = Pin<Box<dyn Future<Output = T> + Send + 'a>>;
type BoxStream<'a, T> = Pin<Box<dyn Stream<Item = T> + Send + 'a>>;

Using SpawnExt::spawn for a BoxFuture<'static, ()> will cause a double indirection, this can be mitigated by either specialization (maybe, I don't know whether this is specializable) or an additional SpawnExt::spawn_box method that directly converts the boxed future into a FutureObj.

trait SpawnExt {
    fn spawn_box(&mut self, future: BoxFuture<'static, ()>) -> Result<(), SpawnError> { ... }
}
@Nemo157
Copy link
Member Author

Nemo157 commented Nov 27, 2018

For comparison, here's a branch adding the aliases to futures, and here's a branch using that to remove all use of *Obj from tide.

@Nemo157
Copy link
Member Author

Nemo157 commented Nov 27, 2018

(cc @aturon, @cramertj)

@cramertj
Copy link
Member

it'd be nice if we could somehow default to 'static like trait objects do naturally. Ah well, not too big of an issue I suppose.

@Nemo157
Copy link
Member Author

Nemo157 commented Nov 27, 2018

Yeah, I did try out type BoxFuture<'a = 'static, T> but it appears default lifetimes aren't a thing 🙁

@cramertj
Copy link
Member

sounds like someone should write an RFC ;)

@ebkalderon
Copy link
Contributor

I'm running into a minor problem with futures-preview 0.3.0-alpha.11 which prevents me from adopting Box<dyn Future<Output = _> + Send + 'a> over FutureObj<'a, _>. I can't seem to call FutureExt::shared() on the trait object:

error: the `shared` method cannot be invoked on a trait object
  --> src/main.rs:98:28
   |
98 |         future.shared()
   |                ^^^^^^
help: another candidate was found in the following trait, perhaps add a `use` for it:
   |
66 | use futures_util::future::FutureExt;
   |

I already have futures::future::FutureExt imported, and I'm able to use it elsewhere in the current module, but this one occurrence is keeping me from switching away from FutureObj. Any idea what might be going wrong?

@Nemo157
Copy link
Member Author

Nemo157 commented Jan 17, 2019

@ebkalderon is this code public somewhere, I can try and track down what's going wrong (my understanding is it should be calling shared on the Box<..> since that implements Future, but it looks like it's instead Derefing the Box and trying to call shared on the dyn Future).

@ebkalderon
Copy link
Contributor

ebkalderon commented Jan 17, 2019

@Nemo157 Unfortunately, no. But I have successfully reproduced the error in a minimal verifiable example and pushed it up to a new repository here, if you would like to take a look: ebkalderon/shared-box-future-example

Edit: I have pushed a second commit showing the FutureObj equivalent which does successfully compile, so you can compare and contrast.

@Nemo157
Copy link
Member Author

Nemo157 commented Jan 17, 2019

@ebkalderon thanks, that helped a lot.

The issue is that Box<F> only implements Future where F: Future + Unpin, so Rust skips <Box<_> as FutureExt>::shared as a candidate method and instead attempts to use a deref coercion to call shared directly on the dyn Future (because shared doesn't require an Unpin future).

The best fix for this would be to use Pin<Box<F>> everywhere, that should be the canonical replacement for FutureObj. The only difference is that it removes the ability for you to take the value out of the box in the future, but without the unstable unsized locals feature that's impossible for trait objects anyway.

I don't know if there's any way to improve the error message here, somehow tell Rust that the impl Future for Box<F> where F: Future + Unpin constraint failure is a higher priority to show an error message for?

@yoshuawuyts
Copy link
Member

Relevant discussion on Runtime's use of FutureObj happening here: rustasync/runtime#6 (comment).

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 a pull request may close this issue.

4 participants