Deprecate Future::wait. #634

carllerche · 2017-11-08T06:47:52Z

This commit deprecates Future::wait. Instead of having a trait function, a wrapper is provided at future::Blocking. Blocking wraps a Future value and provides thread-blocking operation on it.

The deprecation strategy differs from that proposed in tokio-rs/tokio-rfcs#3. The intent is that there still will be current-thread executor functions described in the PR but those will come after.

This PR proposes adding future::Blocking which wraps a T: Future and provides thread-blocking operations. Currently, only wait and poll are provided, but wait_timeout could be added easily later.

I opted for this strategy because in @alex's branch, he provided both BlockingStream and BlockingSink, yet BlockingFuture was omitted. If BlockingSink and BlockingStream are worth having (and I think they are), then BlockingFuture should also be provided. This proves to be a natural place to add any functions for using a future outside the scope of an executor.

I believe that this solves the problems being encountered with wait by not being a trait function (you won't find this function by looking at what the Future trait provides). Also, in the future, thread-blocking operations will panic when invoked from the context of an executor.

This PR opts to name the type future::Blocking.

If this PR is accepted, I will apply a similar treatment to Sink and Stream.

This PR is based against the tokio-reform branch, and not master.

Remaining

Deprecate Wait types
Deprecate functions on Spawn.
Check docs
Add test for new functionality.

This commit deprecates `Future::wait`. Instead of having a trait function, a wrapper is provided at `future::Blocking`. `Blocking` wraps a `Future` value and provides thread-blocking operation on it. Related: tokio-rs/tokio-rfcs#3

carllerche · 2017-11-08T06:56:44Z

Also worth noting, calling future:::Blocking::wait will only drive the current future and not the current thread executor. This avoids the problem of accidentally dropping tasks that never completed.

seanmonstar · 2017-11-08T09:00:51Z

src/future/blocking.rs

+    /// the inner future is not in a ready state.
+    ///
+    /// This function will return immediately if the inner future is not ready.
+    pub fn poll(&mut self) -> Option<Result<T::Item, T::Error>> {


The name poll returning something that isn't Poll seems confusing, when every other future or stream poll will. Maybe something like peek or tap?

I'm open to change the name if a good alternative is proposed.

peek implies the value remains in the future (and there is a peek combinator that does this on Future).

tap I don't understand how tap applies to the behavior (I am not familiar w/ tap as a name here).

Re confusion, the return type is different, so at least it won't compile if you are expecting Poll.

I want to second @seanmonstar's point about poll; we've used that pretty universally to designate that interest will be registered.

It could match cases with blocking data structures in std and be called try_wait, but that's not awesome.

Also, it's a tad confusing for this to be part of the Blocking wrapper, since the point here is precisely that it doesn't block :-)

It could be named something besides Blocking. The real goal of the type is "Use this future from the edge outside of the future task system", but I went with Blocking to make that behavior very explicit.

I considered try_wait, but that doesn't really make sense as it isn't trying to wait...

The only other name that I thought of would be try_take.

Oh I'm not complaining about the name Blocking (which I like), I'm saying that Blocking::poll is a bit confusing because it's not in fact blocking. If you're wanting to poll the future outside of an executor, why would you think to reach for Blocking to do so?

But anyway, I'm more concerned that we find a name other than poll.

re: try_take, I was thinking along similar lines. Seems good?

Does anyone hate try_take instead of poll?

I'm confused as to how this avoids registering interest. My understanding of futures is that they register with the current task before returning NotReady. With poll_future_notify below, isn't the interest is still registered?

Thomasdezeeuw · 2017-11-08T13:31:30Z

src/future/blocking.rs

+
+/// Provides thread-blocking operations on a future.
+///
+/// See [`blocking`] documentation for more details.


This links to nothing, missing "(fn.blocking.html)"?

carllerche · 2017-11-08T17:07:35Z

I would also want to add some tests for Blocking::poll (naming TBD?) before merging. I am waiting to hear if this is PR represents a good strategy.

There are also more tests to update to move away from the deprecated wait.

aturon · 2017-11-08T17:16:00Z

@carllerche It'd be helpful to see the rationale for this API surface as an addition beyond what's in the RFC (maybe I missed the motivation somewhere?) How should one think about having both this and the current-thread executor?

Also worth noting, calling future:::Blocking::wait will only drive the current future and not the current thread executor. This avoids the problem of accidentally dropping tasks that never completed.

I'm also curious to understand how this fits into the RFC design, which was also attempting to solve this problem.

carllerche · 2017-11-08T17:38:21Z

@aturon A few things:

The RFC doesn't touch on what to do with Stream::wait and Sink::wait.
The latest version of the RFC doesn't actually provide a way to block the thread waiting until a single future completes.

@alexcrichton's PR deals with Stream::wait and Sink:wait by providing BlockingStream and BlockingSink, but there is no way in the PR to block waiting for a single future to complete. This fact is apparent when trying to update the tests.

It doesn't make sense to include BlockingStream and BlockingSink without including BlockingFuture. So, either those two types should go or a BlockingFuture equivalent should be added.

I'm also curious to understand how this fits into the RFC design, which was also attempting to solve this problem.

The goal of the RFC was to prevent being able to spawn current-thread tasks and having them be implicitly dropped. This PR maintains this goal.

Blocking::wait does not "enter" the current-thread executor, as such a current-thread spawn will panic.

aturon · 2017-11-08T17:59:08Z

@carllerche Got it, thanks! And yeah, an earlier version of the RFC had something like this (though it was less elaborate; it wasn't a wrapper), which was lost in the shuffle.

I'm good with this approach!

twmb · 2017-11-09T06:59:46Z

Is there a reason for try_take to exist at the moment? I realize that a no-op notifier is the only way to internally implement it, but w.r.t. the inability to avoid actually registering interest, it seems like fitting a square peg into a round hole. Also, I'm not sure why I would wrap a future in Blocking and then have the desire to check it in a non-blocking manner. This also extends down into the get_ref, get_mut, and into_inner functions - if the only purpose of Blocking is truly to block until the future returns, there is no need to access a Blocking's internals.

Additionally, I feel as if blocking, and its corresponding type type Blocking, feel ergonomically awkward to read and I think that feeling would translate to use as well. I recognize that a pattern in the futures crate is to name wrapper types after the functions that produce them (and_then -> AndThen, etc.), but those function names are all verbs that extend off a Future, and the types are really only used in in return types (something that should go away once impl Trait is stable).

I'd prefer something closer to blocking::wrap(f) or block::wrap(f) - this reads to me as something that provides blocking is wrapping a future, which is exactly what blocking(f) does. Assuming that .wait() will be chained on that 99% of the time, blocking::wrap(f).wait() still gives me a bit more context as to what is happening as opposed to blocking(f).wait(). Alternatively, block_on(f), which could directly replace wait as a purely blocking function call.

In terms of the type Blocking itself - this feels the weirdest, because the -ing suffix makes me read it as a verb, but the word itself is a verbal noun. The lack of a sentence as context makes the isolation weird. This is nitpicky, and feel free to ignore this and the prior two paragraphs. I bring this up (and will add more in other tickets / prs / rfc#3) because I had a difficult time reading and understanding the futures api and source due to small semantic things such as this.

carllerche · 2017-11-09T07:09:43Z

@twmb

try_take is very useful in tests.

if the only purpose of Blocking is truly to block until the future returns, there is no need to access a Blocking's internals.

I don't understand why you believe this to be true. A future is very stateful, there is plenty of reason to want to access the inner T before or after blocking. Also, at some point there will be a wait_timeout.

Re naming, the only thing that I care about is that it is succinct, and clearly communicates that the type can block the thread.

twmb · 2017-11-09T07:32:35Z

I don't understand why you believe this to be true.

I meant that only if try_take were to be removed and the only option were to be wait - wait_timeout is more compelling. I'm still a bit skeptical on providing a public function that is mostly useful for tests, but that's not really a showstopper for me.

re: Re naming, if anybody else has input, that would be great as well. Blocking and blocking are likely perfectly fine, but I figured I would throw that out there.

aturon

Just some nits, otherwise LGTM.

aturon · 2017-11-09T17:08:06Z

src/future/blocking.rs

+    /// transitions to a ready state. Once the future is complete, the result of
+    /// this future is returned.
+    ///
+    /// > **Note:** This method is not appropriate to call on event loops or


We should make the function panic in those cases, similarly to what was proposed in the RFC. Basically, just need to assert that there is not currently an executor. But maybe you're planning on doing that once we get the Enter stuff in?

Yes, this step will happen in future PRs.

aturon · 2017-11-09T17:12:20Z

src/sink/blocking.rs

+/// **These operations will block the current thread**. This means that they
+/// should not be called while in the context of a task executor as this will
+/// block the task executor's progress. Also, note that **this value will block
+/// the current thread on drop**. Droping `Blocking` will ensure that the inner


Might be worth mentioning that this is analogous to dropping a BufWriter in std.

aturon · 2017-11-09T17:15:30Z

src/sink/blocking.rs

+#[derive(Debug)]
+#[must_use = "sinks do nothing unless used"]
+pub struct Blocking<T: Sink> {
+    inner: Option<executor::Spawn<T>>,


I can't figure out why this is an Option; AFAICT it's always Some (and everything except the dtor asserts that).

It is an Option to support into_inner (which has to be able to extract the inner type without triggering the dtor).

Ah yep, missed that. 👍

aturon

Alright, good to go!

carllerche · 2017-11-09T20:33:00Z

@alexcrichton could you enable "squash & merge" and "rebase and merge" for this repo?

stbuehler · 2017-11-28T07:22:13Z

I'm curious why blocking is not a trait method; using blocking Futures and Streams in the same module feels unnecessary complicated.

Also there are a lot of doc tests (and some normal unit tests) using the old wait functions; they should be converted to blocking.

Deprecate Future::wait.

feb99e7

This commit deprecates `Future::wait`. Instead of having a trait function, a wrapper is provided at `future::Blocking`. `Blocking` wraps a `Future` value and provides thread-blocking operation on it. Related: tokio-rs/tokio-rfcs#3

carllerche requested review from alexcrichton and aturon November 8, 2017 06:48

seanmonstar reviewed Nov 8, 2017

View reviewed changes

Thomasdezeeuw reviewed Nov 8, 2017

View reviewed changes

Fix for older Rust + update more tests

d9b7c77

carllerche added 6 commits November 8, 2017 17:26

Actually discard notifications

0143fdd

Update tests to avoid deprecated fns

1713e8c

Deprecate Stream::wait for stream::Blocking

a28c0ee

Fix stream docs + remove empty impl block.

fa5514c

Deprecate Sink::wait for sink::Blocking.

7b48a0b

Deprecate more things

d54070b

Add future::Blocking::try_take test

485e3dc

aturon reviewed Nov 9, 2017

View reviewed changes

Mention std::BufWriter in the sink::Blocking docs.

ce4fc94

aturon approved these changes Nov 9, 2017

View reviewed changes

carllerche merged commit 458887c into tokio-reform Nov 10, 2017

carllerche mentioned this pull request Nov 16, 2017

Tokio reform changes #645

Closed

3 tasks

carllerche deleted the deprecate-wait branch November 29, 2017 21:28

cuviper mentioned this pull request Dec 15, 2017

Deadlock with futures mpsc and rayon_wait rayon-rs/rayon#396

Closed

yoshuawuyts mentioned this pull request Mar 14, 2022

Add blocking wrapper smol-rs/async-task#16

Closed

Deprecate Future::wait. #634

Deprecate Future::wait. #634

Uh oh!

Conversation

carllerche commented Nov 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Remaining

Uh oh!

carllerche commented Nov 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carllerche commented Nov 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aturon commented Nov 8, 2017

Uh oh!

carllerche commented Nov 8, 2017

Uh oh!

aturon commented Nov 8, 2017

Uh oh!

twmb commented Nov 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carllerche commented Nov 9, 2017

Uh oh!

twmb commented Nov 9, 2017

Uh oh!

aturon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aturon left a comment

Choose a reason for hiding this comment

Uh oh!

carllerche commented Nov 9, 2017

Uh oh!

stbuehler commented Nov 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

carllerche commented Nov 8, 2017 •

edited

Loading

carllerche commented Nov 8, 2017 •

edited

Loading

twmb commented Nov 9, 2017 •

edited

Loading