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

Clarify what a task is #52610

Merged
merged 1 commit into from
Jul 26, 2018
Merged

Conversation

MajorBreakfast
Copy link
Contributor

@MajorBreakfast MajorBreakfast commented Jul 22, 2018

Currently we call two distinct concepts "task":

  1. The top-level future that is polled until completion
  2. The lightweight "thread" that is responsible for polling the top-level future. What additional data beside the future is stored in this type varies between different Executor implementations.

I'd prefer to return to the old formulation by @alexcrichton:

/// A handle to a "task", which represents a single lightweight "thread" of
/// execution driving a future to completion.
pub struct Task {

Source: task_impl/mod.rs in futures-rs 0.1

I think that this change will make it much easier to explain everything.

r? @aturon
@cramertj

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 22, 2018
@MajorBreakfast MajorBreakfast force-pushed the task-terminology branch 3 times, most recently from 404a810 to 114eb16 Compare July 23, 2018 07:59
@MajorBreakfast
Copy link
Contributor Author

MajorBreakfast commented Jul 23, 2018

Here's a good alternative formulation for the Executor explanation:

Executors are used to spawn futures. An executor is responsible for polling its futures to completion on one or multiple threads.

Efficient scheduling is made possible through the "waking system": The executor creates wakers and associates them with its futures. It polls each future with a Context that contains such a waker. That way, the future can use the waker to signal whenever it can make further progress. Upon being notified, the executor enqueues the associated future to be polled again later. This system is very efficient because the executor never has to actively check its futures. Instead, its futures notify it when they can make further progress.

Note that this formulation completely avoids the term "task". What do you think? Do we really need the term "task"?

Copy link
Contributor

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

I kind of feel like that changed wording is confusing, as it conflates the future and task concept even more. I hadn't noticed the previous change, but before this used to be TaskObj, and so the documentation mentioning tasks was clearer. Part of the reason it was TaskObj was to eventually be able to just accept Task. Perhaps it'd be best to switch back from FutureObj to TaskObj...

Regardless, the actual field changes here will mean breakage for the 0.3-alpha, is that desired for such a minor change?

@MajorBreakfast
Copy link
Contributor Author

MajorBreakfast commented Jul 23, 2018

I kind of feel like that changed wording is confusing, as it conflates the future and task concept even more.

@seanmonstar Which one? The wording from the PR or from my comment above? What parts are unclear? Especially the one from my comment is phrased very carefully. I spent at least one hour writing it and I've been thinking about the "task thing" for the last few days now. I feel more and more convinced that the term "task" is truly useless terminology. I've nothing against keeping it if there's a strong reason for it. But, I want something stronger than "we always had a task, so let's keep it". After thinking so much about it, I'm really convinced now that the term is more confusing than it is helpful. Especially considering that it meant more/something different in futures 0.1.

I hadn't noticed the previous change, but before this used to be TaskObj, and so the documentation mentioning tasks was clearer. Part of the reason it was TaskObj was to eventually be able to just accept Task

@seanmonstar Such a Task trait was mentioned by @carllerche once already. I do not yet understand what its purpose could be. AFAIK our goal is spawn(dyn Future<Output = ()> + Send + Static), i.e. dyn by value. That solution requires object safe arbitrary self type traits and unsized rvalues. See this issue and this PR for why FutureObj was introduced.

Regardless, the actual field changes here will mean breakage for the 0.3-alpha, is that desired for such a minor change?

@seanmonstar I agree. We shouldn't break the API too often. Nevertheless it's an unstable API, so we should break it when we decide on improvements to it. We should discuss everything thoroughly, then break the API once and I can release a new alpha at the same time the new nightly becomes available.

@cramertj
Copy link
Member

I also find the new wording confusing, as @aturon and I had previously used Task to mean Future<Output = ()>. I use it to refer to futures that don't have a meaningful output.

@MajorBreakfast
Copy link
Contributor Author

@aturon and I had previously used Task to mean Future<Output = ()>. I use it to refer to futures that don't have a meaningful output.

@cramertj That's the new definition for it. It's also the one that's used in the async programming book draft.

  • It's completely different to the 0.1 def where the task was a struct called Task, not a future.
  • Output = () is not necessarily the distinguishing factor: There can be futures with Output = () that are not considered to be tasks but just infallible async operations. IMO what distinguishes a task has to be that it is scheduled independently. Like the futures in FuturesUnordered. But these don't have Output = () again. This lead me to conclude that the term "task" is meaningless.

@tinaun
Copy link
Contributor

tinaun commented Jul 24, 2018

i always saw a task as the "back end" of a future - user code deals with futures, then you hand it off to an executor where it becomes a task that the executor schedules.

@MajorBreakfast
Copy link
Contributor Author

@tinaun First, thanks for chiming in! I want everyone to participate in this discussion because I find it incredibly important. Now is the time to shape the API. We won't be able to change it after it is standardized.

i always saw a task as the "back end" of a future - user code deals with futures, then you hand it off to an executor where it becomes a task that the executor schedules.

But the line is sooo fuzzy: futures-util stuff uses the term "future" everywhere. This includes FuturesUnordered. But FuturesUnordered is also used by LocalPool and thus actually handles tasks in that context.

The distinction between future and task seems so wishy washy to me. In one location we refer to the future as "task", in the next we call it a "future" again, and the whole time in 0.1 a "task" is something different entirely.

@aturon
Copy link
Member

aturon commented Jul 24, 2018

@MajorBreakfast The terminology you're proposing here strongly matches how I think about things: a task is a lightweight thread, full stop.

In particular, a really important distinction to understand when working with futures is that a future, by itself, is inert; you always need to be polling it in the context of a task to actually drive it to completion.

So personally I'm 👍 on this PR.

@MajorBreakfast
Copy link
Contributor Author

We discussed this on Discord a bit last night. @seanmonstar said that it must be clear that tasks are not operating system threads. I've updated the wording a bit to make this clearer.

@cramertj
Copy link
Member

I like the new wording a lot more as well. @MajorBreakfast Thanks for working with folks to clear this up!

I'm still a bit hesitant about the breaking change, but I agree it's the right choice and it's better to do it now (early on) than to have to clean it up later, so

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jul 25, 2018

📌 Commit eacfd72 has been approved by cramertj

@bors
Copy link
Contributor

bors commented Jul 25, 2018

🌲 The tree is currently closed for pull requests below priority 99, this pull request will be tested once the tree is reopened

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 25, 2018
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jul 26, 2018
…cramertj

Clarify what a task is

Currently we call two distinct concepts "task":
1. The top-level future that is polled until completion
2. The lightweight "thread" that is responsible for polling the top-level future. What additional data beside the future is stored in this type varies between different `Executor` implementations.

I'd prefer to return to the old formulation by @alexcrichton:
```rust
/// A handle to a "task", which represents a single lightweight "thread" of
/// execution driving a future to completion.
pub struct Task {
```
Source: [`task_impl/mod.rs` in futures-rs 0.1](https://github.com/rust-lang-nursery/futures-rs/blob/1328fc9e8af5737183df477c7501e6ea24ff2053/src/task_impl/mod.rs#L49-L50)

I think that this change will make it much easier to explain everything.

r? @aturon
@cramertj
bors added a commit that referenced this pull request Jul 26, 2018
Rollup of 16 pull requests

Successful merges:

 - #52558 (Add tests for ICEs which no longer repro)
 - #52610 (Clarify what a task is)
 - #52617 (Don't match on region kinds when reporting NLL errors)
 - #52635 (Fix #[linkage] propagation though generic functions)
 - #52647 (Suggest to take and ignore args while closure args count mismatching)
 - #52649 (Point spans to inner elements of format strings)
 - #52654 (Format linker args in a way that works for gcc and ld)
 - #52667 (update the stdsimd submodule)
 - #52674 (Impl Executor for Box<E: Executor>)
 - #52690 (ARM: expose `rclass` and `dsp` target features)
 - #52692 (Improve readability in a few sorts)
 - #52695 (Hide some lints which are not quite right the way they are reported to the user)
 - #52718 (State default capacity for BufReader/BufWriter)
 - #52721 (std::ops::Try impl for std::task::Poll)
 - #52723 (rustc: Register crates under their real names)
 - #52734 (sparc ABI issue - structure returning from function is returned in 64bit registers (with tests))

Failed merges:

 - #52678 ([NLL] Use better spans in some errors)

r? @ghost
@bors bors merged commit eacfd72 into rust-lang:master Jul 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants