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

RFC: Let's deprecate wait #571

Closed
rushmorem opened this issue Sep 5, 2017 · 16 comments
Closed

RFC: Let's deprecate wait #571

rushmorem opened this issue Sep 5, 2017 · 16 comments

Comments

@rushmorem
Copy link

It's too easy to use incorrectly

I have seen a lot of people, myself included, use it incorrectly either out of ignorance or just because it's so convenient to use (sometimes both) ending up with code that block the entire thread unnecessarily. IMHO, the situation has just gotten worse with async/await. People who don't understand the difference between wait() and await!() might use wait() in the async macro (probably because it makes code easier to read) which will work. They will wonder why their programs are slow (it blocks the entire event loop), worse still they might not even realise this.

It makes task execution confusing

According to the documentation we have 3 methods of executing futures; wait(), the thread pool and the event loop. I have seen some people get confused about when to use which. The documentation tries to explain this but the fact that the thread pool returns a future and that all futures have a wait method doesn't help. If we deprecate wait() we remain with only 2, the thread pool and the event loop. However, considering that the thread pool simply returns a future, we will be forced to run that future on the event loop which brings us down to only one way to drive futures to completion.

Questions that have also come up include

  • when to use the thread pool
  • when to use the event loop
  • how to use both

In my experience there is only one ideal way to drive futures to completion. That is, using an event loop. If you have tasks that are CPU bound, throw them on the thread pool and grab the CPU futures you get back. If you have tasks that are IO bound construct them as needed and grab the IO futures you get back. Multiplex your CPU futures with your IO futures as needed (you can join, select them etc, all that good stuff) and grab the resulting future. Throw that resulting future on the event loop and you are done. In my experience, this is the most efficient way to use futures and deprecating wait() helps guide users towards this.

It's easy to implement

Those who really need it can easily implement it on top of await().

@tomwhoiscontrary
Copy link

It's sometimes useful to be able to wait for a future. I often do it during an initialisation process that can afford to take some time, and where the complexity involved in making the process properly asynchronous just isn't worth it.

If the method to do this has a confusing name, let's rename it. .join()? .blockUntIlComplete()?

Getting rid of this method entirely, however, is a bad move. Saying it's not needed because it's easy to reimplement (thousands of times, in every codebase that needs it) seems silly.

I worry i might have misunderstood this issue; apologies if so.

@alexcrichton
Copy link
Member

To me the main thrust of this issue is less so about the functionality and moreso about the placement of the API, in that Future::wait, a method on the trait with a "sweet name" is perhaps too prominent a location for this functionality. I think that we'll always want this functionality in the futures crate, just perhaps not as a method on the trait.

(I'm not 100% sure where else it would go though)

@tomwhoiscontrary
Copy link

@alexcrichton In that case, i'd re-raise the idea of renaming the method to something really unattractive, rather than removing it.

Failing that, how about an extension trait, so the method is not usually in scope?

@abonander
Copy link

Throwing it out there, what about blocking_wait()? That says exactly what the method does.

@rushmorem
Copy link
Author

@tomwhoiscontrary

It's sometimes useful to be able to wait for a future. I often do it during an initialisation process that can afford to take some time, and where the complexity involved in making the process properly asynchronous just isn't worth it.

I know. However, as pointed out in my second reason, wait() is not the only way to wait for a future. You can still use Core::run() for that.

Getting rid of this method entirely, however, is a bad move. Saying it's not needed because it's easy to reimplement (thousands of times, in every codebase that needs it) seems silly.

I specifically put that point last because it's the least important.

  • The first point is saying that it's very easy to shoot yourself in the leg with that method.
  • The second point then says you don't really need that method because there is a better method you can use (that is Core::run())
  • That last point is then saying, if you really insist on using this method it's very easy to implement on top of await()

The point is to try and protect users from shooting themselves in the leg using the API, much like Rust tries to protect users.

@alexcrichton in alexcrichton/futures-await#22 (comment) you said:-

I believe there's a whole slew of reasons to deprecate the Future::wait method at this point, so it's likely to happen

Mind sharing some of those reasons?

@rushmorem
Copy link
Author

While renaming the method will help to differentiate it from await!() it still doesn't address all the issues above. We already have Core::run() do we really need 2 ways to wait for a future? If you have a future that you want to wait on but don't really care about async what's stopping you from just doing core.run(future)?

@est31
Copy link
Member

est31 commented Sep 5, 2017

@rushmorem Core::run is a function of the tokio_core crate, while Futures::wait is directly inside the futures crate. tokio_core brings in a bunch of dependencies like cpupool which you might not want nor need. E.g. I am developing a library to provide async parsing of a format, and I only rely on futures so that nobody has to pull in tons of dependencies. So if its going to be deprecated, there should definitely a renamed replacement inside the futures crate, e.g. what @abonander suggested.

@carllerche
Copy link
Member

IMO the function should be removed from the Future trait to add some "pepper" against using it. The advantage of removing it from Future is that it will help control the the function is learned (it isn't randomly found poking on Future) as well as it would require a step to "think" about using a function that isn't on the trait itself. Perhaps by requiring an import.

@est31
Copy link
Member

est31 commented Sep 5, 2017

@carllerche btw could you have a look: rust-lang/rfcs#2076

Its offtopic sorry but I couldn't reach you otherwise :)

@tafia
Copy link

tafia commented Sep 6, 2017

Not an expert but why not requiring a core in the future wait fn: wait<R: Reactor>(core: &mut R) ? This is easy to find and use? We could then perhaps have a default Reactor (not sure if there is already a trait for that) that blocks the thread to reproduce current behavior?

@SimonSapin
Copy link
Contributor

what @SimonSapin suggested.

(Perhaps you meant someone else? I have no idea what this is about.)

@est31
Copy link
Member

est31 commented Sep 6, 2017

@SimonSapin 🤣 sorry I've meant @abonander ... edited

@rushmorem
Copy link
Author

Closing this in favour of tokio-rs/tokio-rfcs#2...

@casey
Copy link
Contributor

casey commented Oct 16, 2017

I would like to see this resurrected. tokio-rs/tokio-rfcs#2 and tokio-rs/tokio-rfcs#3 seem to leave wait in place, unless I'm misunderstanding. I think it should be removed. For convenience, core::wait_until(future) could be provided, making clear that it is run on a core.

@alexcrichton
Copy link
Member

@casey
Copy link
Contributor

casey commented Oct 17, 2017

Oh sweet, I searched but must have missed that. Thanks!

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

9 participants