[WIP] Explicit task::Context argument to poll. #744

ahmedcharles · 2018-02-09T03:27:09Z

Only futures-core has been changed so far. Feedback welcome.

udoprog · 2018-02-09T10:34:30Z

Echoing this in the right place this time:

So I'm playing around converting the code base, and I have hit a roadblock with trying to implement catch_unwind (See comment). Does anyone know how to work around this?

Basically, this:

let (res, future) = catch_unwind(|| (future.poll(), future))?;

Becomes:

let (res, future) = catch_unwind(|| (future.poll(ctx), future))?;

And ctx can't be safely passed into the closure since it's not UnwindSafe. (My context is based on BorrowedTask).

ahmedcharles · 2018-02-09T10:36:41Z

I'm working on futures-util now. I'll update again after I get that to compile.

udoprog · 2018-02-09T14:32:28Z

After reading up this, if we can reason that task::Context is unwind safe, we can use AssertUnwindSafe to work around this:

let (res, future) = catch_unwind(panic::AssertUnwindSafe(|| (future.poll(ctx), future)))?;

As to whether they are unwind safe seems to depend on how a specific NotifyHandle is implemented, and how task-local storage works (is it still platform dependent in 0.2?). It would be nice to make sure this is documented for NotifyHandle implementors, and make sure that the task-local APIs we provide guarantee unwind safety.

ahmedcharles · 2018-02-09T14:45:08Z

My goal so far has been to make the 'interesting' commit as short as possible, so it's easier to discuss/review.

It currently compiles, though the tests don't pass (well, more accurately, the tests have regressed compared to the penultimate commit.

ahmedcharles · 2018-02-09T14:51:27Z

futures-executor/src/task_runner.rs

@@ -298,6 +298,7 @@ impl Future for SpawnedFuture {
    type Error = bool;

    fn poll(&mut self, ctx: &mut task::Context) -> Poll<bool, bool> {
-        self.inner.with_task_data(|f| f.poll(ctx))
+        //self.inner.with_task_data(|f| f.poll(ctx))
+        self.inner.get_mut().poll(ctx)


I'm not sure I understand why this was implemented this way to begin with. It seems like it would be a noop, but I have no idea where the current task gets set.

ahmedcharles · 2018-02-09T14:52:48Z

futures-util/src/stream/catch_unwind.rs

@@ -45,7 +45,6 @@ impl<S> Stream for CatchUnwind<S>
            }
            CatchUnwindState::Stream(stream) => stream,
        };
-        // FIXME: fix passing the actual context here.
        let res = catch_unwind(|| (stream.poll(&mut task::Context::panicking()), stream));


This needs more thought.

As mentioned in this comment, unwind safety seems to have been deferred to implementors of NotifyHandle and the platform-specific task-local storage implementation. This also was the case for TLS-based contexts. So it seem ignoring this using panic::AssertUnwindSafe is the way to go, and making sure panic safety is a documented requirement for NotifyHandle, UnsafeNotify, and task-local.

ahmedcharles · 2018-02-09T14:52:57Z

futures-util/src/future/catch_unwind.rs

@@ -30,7 +30,6 @@ impl<F> Future for CatchUnwind<F>

    fn poll(&mut self, _ctx: &mut task::Context) -> Poll<Self::Item, Self::Error> {
        let mut future = self.future.take().expect("cannot poll twice");
-        // FIXME: fix passing the actual context here.
        let (res, future) = catch_unwind(|| (future.poll(&mut task::Context::panicking()), future))?;


This needs more thought.

ahmedcharles · 2018-02-09T14:54:48Z

futures-core/src/task_impl/mod.rs

@@ -266,30 +199,16 @@ impl<T: ?Sized> Spawn<T> {
        self.enter(BorrowedUnpark::new(&mk, id), f)
    }

-    /// TODO: dox
-    pub fn with_task_data<F, R>(&mut self, f: F) -> R


The RFC doesn't actually state what the changes to all of the API's should be, but this one doesn't seem useful anymore, given that the context explicitly contains task data.

ahmedcharles · 2018-02-09T14:57:20Z

futures-core/src/task_impl/mod.rs

    /// Returns a `Context` which will panic when `waker` is called.
    /// This is useful when calling `poll` on a `Future` which doesn't
    /// access the context.
-    pub fn panicking() -> Context {
-        Context { waker: None }
+    pub fn panicking() -> Context<'a> {


This function seems questionable, but it did help migrate the tests for futures which don't access the context at all.

ahmedcharles · 2018-02-09T14:59:19Z

futures-core/src/task_impl/mod.rs

+/// the future as notifications arrive, until the future terminates.
+///
+/// The poll function accepts a context, which is usually provided by the executor.
+pub struct Context<'a>(Option<ContextInner<'a>>);


The interaction between Spawn and Context make having Context own the task-local data difficult, hence the lifetime.

udoprog · 2018-02-09T15:18:26Z

futures-core/src/task_impl/mod.rs

+/// the future as notifications arrive, until the future terminates.
+///
+/// The poll function accepts a context, which is usually provided by the executor.
+pub struct Context<'a>(Option<ContextInner<'a>>);


no-op contexts seems to be supported by providing an empty LocalMap plus a no-op NotifyHandle (or collecting, for testing).

The use for Context(None) seems to be to implement the panicking context. Could this be accomplished with an empty LocalMap and panicking NotifyHandle instead to avoid the Option?

Probably, though I don't think it's important to get it perfect in the first PR, since it's an implementation detail that can be changed locally at any time.

cramertj · 2018-02-12T18:22:27Z

futures-channel/tests/mpsc.rs

@@ -34,29 +34,29 @@ fn send_recv_no_buffer() {

    // Run on a task context
    let f = lazy(move || {
-        assert!(tx.flush().unwrap().is_ready());
-        assert!(tx.poll_ready().unwrap().is_ready());
+        assert!(tx.flush(&mut TaskContext).unwrap().is_ready());


TaskContext isn't the name of a variable in this scope-- is this just a WIP artifact?

This is using the prelude (I don't really like preludes, but I figured I'd use it... maybe it would confuse someone and convince them preludes were bad? j/k).

alexcrichton

Thanks for the PR @ahmedcharles! Everything looks mostly good to me but my main worry is about how we'd exactly like to implement the context argument here. I think after reading it over and thinking I'm leaning towards the "have a lifetime argument" solution but I'm not sure it's the best strategy, so I'm curious what @cramertj you think about this?

alexcrichton · 2018-02-12T19:20:55Z

futures-core/src/task_impl/mod.rs

 ///
 /// In general, futures are composed into large units of work, which are then
 /// spawned as tasks onto an *executor*. The executor is responsible for polling
 /// the future as notifications arrive, until the future terminates.
 ///
-/// This is obtained by the `task::current` function.
+/// The poll function accepts a context, which is usually provided by the executor.
+pub struct Context<'a>(Option<ContextInner<'a>>);


This is certainly quite interesting! I think that for efficiency we will probably want the 'a lifetime parameter on Context, but I could also see how that's not always desired (and it doesn't follow 100% for me in terms of what I might naively expect).

@cramertj do you have thoughts on this? Do you think we should avoid this entirely? Or in general, do you think that the concrete implementation of Context has an "obvious" implementation we should otherwise be doing?

I had tossed around a few ideas here: I think the actual function signature we want is mut ctx: Context<'a>. Context does need to take the locally provided impls of Spawn etc. by reference, as well as the "super" Context, so we do need a lifetime parameter.

alexcrichton · 2018-02-12T19:26:04Z

futures-core/src/task_impl/mod.rs

-            set(&new_task, || f(obj))
-        })
-    }
-
    fn enter<F, R>(&mut self, unpark: BorrowedUnpark, f: F) -> R


The whole Spawn type I think may not make 100% sense in the world where contexts are explicitly passed around. I'd sort of naively expect that we'd have something like:

struct Spawn<F> { inner: F, context: Context, }

but that unfortunately doesn't work well with the lifetime argument on Context right now. In theory though the task-local data should be stored on the Context itself for now rather than in Spawn I think?

I wonder if we could do something like:

struct Context { data: TaskLocalDataMap, // I forget the actual name waker: NotifyHandle, // how to do a wakeup } impl Context { fn new<T: Into<NotifyHandle>>(inner: T) -> Context; // variuos accessors for `data` fn set_waker(&mut self, ...); // maybe? }

One problem I think is that right now we make heavy usage of the lifetime parameters and stack closures and whatnot. In that sense we sort of want there to be a Context type (with no lifetime parameters maybe?) as well as a ContextRef<'a> type which all futures take. Or at least that matches that current scheme... I'm sort of losing myself here anyway

I did try an iteration of this code with context as a member of Spawn. It makes panic safety harder when trying to change the notify handle temporarily for the task because you have to have a way to unset it. But you need mut access to the future and the context at the same time (which rust doesn't like).

alexcrichton · 2018-02-12T19:33:54Z

futures-executor/src/task_runner.rs

@@ -297,7 +297,8 @@ impl Future for SpawnedFuture {
    type Item = bool;
    type Error = bool;

-    fn poll(&mut self) -> Poll<bool, bool> {
-        self.inner.with_task_data(|f| f.poll())
+    fn poll(&mut self, ctx: &mut task::Context) -> Poll<bool, bool> {


At least stylistically for this repo I'd personally prefer cx over ctx, but I may also be too used to rustc

For what it is worth, ctx commonly stands for "context" in programming contexts (can be verified by google search "ctx programming" vs. "cx programming").

+1 for ctx on the bikeshed, if for no other reason than that it will be obvious to folks coming from golang.

It's probably not worth much, but I'm for ctx since I picked it in the first place and changing it would mean I'd have to change it. :)

alexcrichton · 2018-02-12T19:35:16Z

futures/src/lib.rs

@@ -238,6 +238,8 @@ pub mod prelude {
        Poll,
    };

+    pub use futures_core::task::Context as TaskContext;


@cramertj I think you followed the RFC more closely than I, but is this what the RFC concluded?

When the RFC concluded, I said we'd leave it as task::Context, and that further prelude-import-renamings could be addressed via followup discussions/proposals. I'd be inclined to leave this out for now, and instead import task.

That's fine. It's an easy change.

alexcrichton · 2018-02-12T21:12:02Z

Ok so I think this could be a great opportunity to jettison Spawn-the-type! I wonder if we could get away with:

impl Context<'a> {
    pub fn new<F>(f: F, data: &'a TaskData) -> Context<'a> 
        where F: FnMut() -> NotifyHandle + 'a;
}

and I think via that we could eliminate Spawn entirely (the type) along with spawn the function and still have all the information needed for all poll calls, right?

carllerche · 2018-02-12T21:17:28Z

@alexcrichton I think this is good. As you pointed out, Context is the only thing you need to poll a task. There should be no need for Spawn anymore.

alexcrichton · 2018-02-12T21:29:03Z

Hm so with some more thinking, I think we've got two choices of signatures for Future:

trait Future {
    // option 1
    fn poll(&mut self, cx: Context<'_>) -> Poll<T, E>;

    // option 2
    fn poll(&mut self, cx: &mut Context<'_>) -> Poll<T, E>;
}

If we were to go with option 1 I think it also requires a function like:

impl Context<'a> {
    fn reborrow(&'b mut self) -> Context<'b>;
}

to enable use cases like:

impl Future for Select {
    fn poll(&mut self, cx: Context<'_>) -> Poll<T, E> {
        let res1 = self.a.poll(cx);
        let res2 = self.b.poll(cx); //~ ERROR: use of moved value `cx`
        // ...
    }
}

I would personally lean towards &mut Context<'_> I think?

carllerche · 2018-02-12T21:40:35Z

I lean very heavily towards &mut Context. Adding reborrowing fns does not feel great. There also should always be a place on the stack for the Context value (before calling poll).

This doesn't change the functionality and the Context is `()`. This allows having this commit be purely mechanical. Also add some missing `#[must_use]`'s to futures and streams.

alexcrichton · 2018-02-13T19:50:03Z

Ok thanks again for starting this out @ahmedcharles! With #758 extracted from this and merged I think I've finished up most of the rest at #762 so I'm going to close this for now

ahmedcharles mentioned this pull request Feb 9, 2018

Explicit task::Context argument to poll rust-lang-nursery/futures-rfcs#2

Merged

ahmedcharles commented Feb 9, 2018

View reviewed changes

udoprog reviewed Feb 9, 2018

View reviewed changes

cramertj reviewed Feb 12, 2018

View reviewed changes

alexcrichton reviewed Feb 12, 2018

View reviewed changes

cramertj mentioned this pull request Feb 12, 2018

RFC: executor design for futures 0.2 rust-lang-nursery/futures-rfcs#3

Merged

ahmedcharles added 3 commits February 13, 2018 02:24

Add task::Context to poll, etc.

45ee986

This doesn't change the functionality and the Context is `()`. This allows having this commit be purely mechanical. Also add some missing `#[must_use]`'s to futures and streams.

Add panicking function on Context, along with Waker.

f580555

Explicit task::Context argument to poll.

c99431d

alexcrichton closed this Feb 13, 2018

ahmedcharles deleted the context branch February 13, 2018 20:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Explicit task::Context argument to poll. #744

[WIP] Explicit task::Context argument to poll. #744

ahmedcharles commented Feb 9, 2018 •

edited

Loading

udoprog commented Feb 9, 2018 •

edited

Loading

ahmedcharles commented Feb 9, 2018

udoprog commented Feb 9, 2018

ahmedcharles commented Feb 9, 2018

ahmedcharles Feb 9, 2018

ahmedcharles Feb 9, 2018

udoprog Feb 9, 2018

ahmedcharles Feb 9, 2018

ahmedcharles Feb 9, 2018

udoprog Feb 9, 2018

ahmedcharles Feb 9, 2018

ahmedcharles Feb 9, 2018

udoprog Feb 9, 2018

ahmedcharles Feb 9, 2018

cramertj Feb 12, 2018

ahmedcharles Feb 13, 2018

alexcrichton left a comment

alexcrichton Feb 12, 2018

cramertj Feb 12, 2018

alexcrichton Feb 12, 2018

alexcrichton Feb 12, 2018

ahmedcharles Feb 13, 2018

alexcrichton Feb 12, 2018

carllerche Feb 12, 2018

cramertj Feb 13, 2018

ahmedcharles Feb 13, 2018

alexcrichton Feb 12, 2018

cramertj Feb 12, 2018

ahmedcharles Feb 13, 2018

alexcrichton commented Feb 12, 2018

carllerche commented Feb 12, 2018

alexcrichton commented Feb 12, 2018

carllerche commented Feb 12, 2018

alexcrichton commented Feb 13, 2018

[WIP] Explicit task::Context argument to poll. #744

[WIP] Explicit task::Context argument to poll. #744

Conversation

ahmedcharles commented Feb 9, 2018 • edited Loading

udoprog commented Feb 9, 2018 • edited Loading

ahmedcharles commented Feb 9, 2018

udoprog commented Feb 9, 2018

ahmedcharles commented Feb 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Feb 12, 2018

carllerche commented Feb 12, 2018

alexcrichton commented Feb 12, 2018

carllerche commented Feb 12, 2018

alexcrichton commented Feb 13, 2018

ahmedcharles commented Feb 9, 2018 •

edited

Loading

udoprog commented Feb 9, 2018 •

edited

Loading