Reorganize the futures_core::task module
#762
Conversation
alexcrichton
force-pushed
the
rewrite-task
branch
from
76202fc to
b4e4944
Compare
| //! thought of as one unit, as the entire result is essentially just moving | ||
| //! through one large state machine. | ||
| //! | ||
| //! A "task" is the unit of abstraction for what is driving this state machine |
There was a problem hiding this comment.
@aturon Is this still how we want to talk about "tasks"? My intuition always lead me to think that a "task" was the top-level future driven by an executor -- we'd even talked about trait Task = Future<Item = (), Error = ()>;.
Edit: from reading further, it's clear these aren't new docs-- github just thinks they are :)
There was a problem hiding this comment.
Oh right yeah so I've just copied/pasted doc stuff around, I'm thinking we'll want docs to be a future PR. Right now it's all likely horribly outdated.
There was a problem hiding this comment.
Yeah I will do a complete pass on the docs once the dust settles.
futures-core/src/task/mod.rs
Outdated
| pub struct Context<'a> { | ||
| handle_factory: &'a (Fn() -> NotifyHandle + 'a), | ||
| id: usize, | ||
| map: &'a LocalMap, |
There was a problem hiding this comment.
Why is this an immutable reference, rather than a mutable one? We are mutating the LocalMap, and it shouldn't be shared with anyone else at the time, should it?
There was a problem hiding this comment.
Similarly, is there a reason the handle_factory is an &'a Fn() instead of an &'a mut FnMut?
There was a problem hiding this comment.
An excellent point! Just a carryover from before, I'll change it to a mut everywhere.
futures-core/src/task/mod.rs
Outdated
| /// This function will panic if a task is not currently being executed. | ||
| /// That is, this method can be dangerous to call outside of an | ||
| /// implementation of `poll`. | ||
| pub fn waker(&mut self) -> Task { |
There was a problem hiding this comment.
TODO: make this return a Waker rather than a Task.
There was a problem hiding this comment.
Sure yeah, I can do that in this PR as well.
alexcrichton
force-pushed
the
rewrite-task
branch
3 times, most recently
from
cc18ba0 to
b7af114
Compare
This commit is a large reorganization of the `task` module in accordance with RFC 2 which added an explicit `Context` argument to all futures-related functions. This argument is now explicitly constructed when necessary and otherwise passed around in implementations of `poll`.
alexcrichton
force-pushed
the
rewrite-task
branch
from
b7af114 to
fff7e7b
Compare
| loop { | ||
| match future.poll_future_notify(&::IntoNotifyHandle(self.handle), 0) { | ||
| match future.poll(&mut task::Context::new(&mut map, 0, self.handle)) { |
There was a problem hiding this comment.
IMO this method is a bit confusing-- if i call task::Context::new(&mut map, 0, handle).block_on(fut) manually, it seems like i could reasonably expect it to use the LocalMap I provided, rather than generating a new one for this specific task. I guess this function will probably go away (or at least look different) in the new executor design, so maybe it's not worth worrying about now. What do you think?
There was a problem hiding this comment.
Yeah I think let's revisit then.
|
Gonna go ahead and merge this and start hacking on the executor stuff and the |
This commit is a large reorganization of the
taskmodule in accordance withRFC 2 which added an explicit
Contextargument to all futures-relatedfunctions. This argument is now explicitly constructed when necessary and
otherwise passed around in implementations of
poll.