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

Explicit `task::Context` argument to `poll` #2

Merged
merged 4 commits into from Feb 12, 2018

Conversation

@cramertj
Collaborator

cramertj commented Feb 1, 2018

Learnability, reasoning, debuggability and no_std-compatibility improvements achieved by making task wakeup handles into explicit arguments.

Rendered

@cramertj cramertj referenced this pull request Feb 1, 2018

Merged

Futures 0.2 #1

@aturon

This comment has been minimized.

Contributor

aturon commented Feb 1, 2018

The discussion about this in #1 mostly centered on the following sentiment:

  • If the motivation for introducing Context is solely learnability, isn't there lower-hanging fruit (like docs) that don't have ergonomic downsides?

I personally do agree with this sentiment, so to me the key question is if a Context argument has benefits beyond learnability. The RFC mentions some (related to no_std compat), but in addition, one effect of having this explicit argument is that you can tell, at a call site, whether a given method involves potentially queuing the task. That seems potentially helpful information to have locally, especially when navigating a foreign codebase.

I think it's a really tough call, and I've long basically been on the fence on this one. But it certainly does give me pause that many, many people in the Rust community who have learned futures have continued to ask for this argument to be explicit.

@carllerche

This comment has been minimized.

carllerche commented Feb 1, 2018

@aturon The majority of the community feedback in the original thread is the opposite. The lack of an explicit argument has not been the primary blocker to learnability. This is also feedback from people who spend time actively teaching futures.

The simple fact is addressing the learnability with docs has not been tried. Changing the API at this point to address learnability is premature given the state of documentation.

I think this comment provides valuable insight into learnability.

Also, the community feedback from those who spend a lot of time implementing Future is that the explicit context argument is just additional boilerplate as in most cases it will not be used directly, instead just ceremoniously passed along.

My personal opinion is that adding an explicit task argument is going to make the experience of working with futures noticeably worse to solve a problem that could be handled with a couple paragraphs of docs + a few examples.

As @djc said:

It seems the main disadvantage for learnability (that is, leakiness of the abstraction) would not be on most authors of Future impls, but only on a pretty select set that implement fairly low-level Future impls (at the level of mio itself, for example). In all of the cases that I tried (dealing with network protocols, near the tokio-io/tokio-proto level of abstraction) it was just composing higher-level futures out of lower-level ones, meaning passing around Context would just be boilerplate.

@aturon

This comment has been minimized.

Contributor

aturon commented Feb 1, 2018

@carllerche

As I said in my comment, I don't think the learnability argument by itself suffices to motivate this change.

The majority of the community feedback in the original thread is the opposite.

There's been a mix of sentiment, including on the very long-running thread on this issue. All I'm saying is that, as you know well, it's something that people have been asking for for quite a long time, and that gives me some pause.

Also, last time we spoke about this, you were very much on the fence. I'm curious what's changed your opinion?

@skade

This comment has been minimized.

skade commented Feb 1, 2018

The summary uses the word "learnability", but then the RFC never uses it again.

In the main paragraphs, it solely uses discoverability, which is clearly different. Discoverability can also be improved by mentioning the existence of the task in the documentation (possibly in the method docs).

The technical arguments for this are very underdeveloped in this RFC, with a mere 2 sentences.

I'd find it more convincing if I found a sketched no_std example there. I find that a worthwhile discussion path, sadly, there's almost no one coming up with a proper example.

@carllerche

This comment has been minimized.

carllerche commented Feb 1, 2018

@aturon mostly that I thought I was crazy and I was the only one who thought the thread-local strategy was a good idea. So, like I said, I wasn't going to block the decision either way and let it go to RFC.

The community feedback tips me back in the "thread-local" camp, but all I'm doing is putting my thoughts on the record and let what happens happen.

@aturon

This comment has been minimized.

Contributor

aturon commented Feb 1, 2018

@skade Yep, I agree that this RFC will need to make the non-learnability motivations much stronger to be a solid sell.

@carllerche

This comment has been minimized.

carllerche commented Feb 5, 2018

Could the expected integration of futures w/ async / await be included here?

For example, would crates like h2 be updated to use async / await instead? Would future implementations be able to use async / await?

How is the task context accessed from async / await fns?

It seems to me that if writing poll fns manually goes away, then the ergonomic downside is significantly reduced, but I have no idea if that would happen or not.

tools to the task system. Deadlocks and lost wakeups are some of the trickiest things
to track down with futures code today, and it's possible that by using a "two stage"
system like the one proposed in this RFC, we will have more room for adding debugging
hooks to track the precise points at which queing for wakeup occurred, and so on.

This comment has been minimized.

@carllerche

carllerche Feb 5, 2018

I don't see how this is true. Making the task argument explicit has no functionality change. You can hook into the task system to add debugging logic today. It is just that nobody has actually done it.

The implicit `Task` argument makes it difficult to tell which functions
aside from `poll` will use the `Task` handle to schedule a wakeup. That has a
few implications.

This comment has been minimized.

@carllerche

carllerche Feb 5, 2018

You can tell which functions will use the Task handle because they return Async.

Today, you can look at any function in the h2 library and know if it requires a task context or not.

This comment has been minimized.

@aturon

aturon Feb 5, 2018

Contributor

You can't tell that easily from the call site, which is the key difference in code reasoning.

This comment has been minimized.

@cramertj

cramertj Feb 5, 2018

Collaborator

There are also functions like need_read which don't return Async but do rely on an implicit Task.

This comment has been minimized.

@seanmonstar

seanmonstar Feb 5, 2018

You can't tell that easily from the call site, which is the key difference in code reasoning.

But you can, it's determined by the return value. Unless you mean when people rely on type inference for the return value and don't actually check that it's Async?

This comment has been minimized.

@vitalyd

vitalyd Feb 5, 2018

I believe I've seen code use Async as a "general purpose" return value to indicate readiness, without necessarily touching a Task internally. Perhaps this practice should be discouraged if the "Async as a return value means we touched a Task" convention is to take hold.

This comment has been minimized.

@liigo

liigo Feb 5, 2018

But you can't easily.

First, it's easy to accidentally call a function that will attempt to access
the task outside of a task context. Doing so will result in a panic, but it
would be better to detect this mistake statically.

This comment has been minimized.

@carllerche

carllerche Feb 5, 2018

This is the only real advantage as far as I can tell. If &mut Context is explicit, you actually can't call it.

However, today with the Async return value, if you accidentally try to call a function and use the return value, you will statically have to interact w/ the Async value, which will let you know the function requires a task context.

If you do something like let _ = my_async_fn().unwrap(), then you will not get compiler warnings.

Second, and relatedly, it is hard to audit a piece of code for which calls
might involve scheduling a wakeup--something that is critical to get right
in order to avoid "lost wakeups", which are far harder to debug than an
explicit panic.

This comment has been minimized.

@carllerche

carllerche Feb 5, 2018

As mentioned above, you can do this by looking at the Async return value, though this does take a bit more work than searching for a variable.

For combinator implementations, this hit is quite minor.
For larger custom future implementations, it remains possible to use TLS internally to recover the existing ergonomics, if desired.

This comment has been minimized.

@carllerche

carllerche Feb 5, 2018

IMO suggesting that people do something non-standard is a weak argument. If the non-standard strategy is better, then why is it not the standard strategy?

Sticking w/ the idioms is the best option.

For larger custom future implementations, it remains possible to use TLS internally to recover the existing ergonomics, if desired.
Finally, and perhaps most importantly, the explicit argument is never seen by users who only use
futures via combinators or async-await syntax, so the ergonomic impact to such users in minimal.

This comment has been minimized.

@carllerche

carllerche Feb 5, 2018

If the explicit argument is never seen by users who ony use futures via combinators or async-await syntax, then why not make the ergonomics solid for those of us who do?

Really, it seems that this proposal hinges on:

  • How often do users transition from combinators only -> writing Async functions?
  • Is it worth making that initial transition smoother at the cost of making the experience of writing Async functions worse?
  • How much smoother will that transition actually be with an explicit task argument?

This comment has been minimized.

@skade

skade Feb 5, 2018

In my experience, there's a point (and it comes pretty quickly) where you want to implement a future on you own. I'd not push that towards being an edge-case.

Then again, these are mostly futures that proxy the calls through to child futures. The biggest danger there, though, is forgetting to make sure there's always a future polled, which leaves the future group in waiting for poll. The task argument doesn't fix that.

This comment has been minimized.

@carllerche

carllerche Feb 5, 2018

The biggest danger there, though, is forgetting to make sure there's always a future polled, which leaves the future group in waiting for poll. The task argument doesn't fix.

That is a good point. The explicit task argument doesn't actually statically prevent you from making the mistake that this RFC is attempting to protect you from.

This comment has been minimized.

@Marwes

Marwes Feb 5, 2018

I'd question the notion that the TLS solution is more ergonomic. While it does reduce the amount of characters need to write a poll implementation that is at best a weak argument in favor of it. On the other hand I'd argue that keeping things explicit improves "ergonomics" just by virtue of being explicit.

This comment has been minimized.

@seanmonstar

seanmonstar Feb 5, 2018

I'd question the notion that the TLS solution is more ergonomic.

Having written a lot of custom impl Future, it definitely is more ergonomic.

This comment has been minimized.

@Marwes

Marwes Feb 5, 2018

@seanmonstar

Having written a lot of custom impl Future, it definitely is more ergonomic.

I assume there is something more than not needing to pass a context that makes this more ergonomic? If so, what is it?

This comment has been minimized.

@liigo

liigo Feb 5, 2018

impl Future is explicit, than

struct Foo;
impl Future for Foo {}
@P-E-Meunier

This comment has been minimized.

P-E-Meunier commented Feb 5, 2018

How much smoother will that transition actually be with an explicit task argument?

As someone who has implemented a number of Futures (in crates like Thrussh and Pleingres, and other unreleased projects), I'd actually also like to question the learnability argument.

I'm not saying futures are easy to learn, it did take me a while to figure out how to implement poll correctly for complex types (for instance an SSL wrapper with bounded memory would be difficult to implement, I don't think tokio-openssl does that), but I don't remember any time where thread-local storage has been a problem: most of the time, my errors were in the logic, i.e. I would return Async::NotReady where in fact no IO operation had happened (or more generally, no other future had returned Async::NotReady). For instance, the current API makes it slightly tricky to test whether at least one of several futures is ready, but I can't think of any better way, because the logic is not simple anyway.

I believe the main thing I had to learn was that, and I fail to see how an extra argument to all functions would make it more explicit.

I must admit I still don't really know how futures get registered for wakeup, but my current mental model seems to work well enough, and I don't believe the extra Context argument would teach me deeper things about that.

@Marwes

This comment has been minimized.

Marwes commented Feb 5, 2018

I feel the whole learnability argument has become somewhat of a red herring (though I do agree with it, as I did not get where wakeups were registered at first and an explicit context would help with that).

Moving on. I'd really like some input from people that want to use futures in no_std environments. If removing TLS makes it possible (or perhaps just makes it easier) to use use futures in such environments that is an indisputable advantage of the explicit solution. Perhaps we may deem those use cases not important enough but that should be carefully weighed against the advantages of TLS.

@llogiq

This comment has been minimized.

llogiq commented Feb 5, 2018

I think it is good to explore the design space here, however caution is of course always advisable.

I find some precedent in keeping state off the call stack in the regex crate, which if memory serves uses some thread local storage to make regexes thread-safe to use (whether or not this feature is widely used I won't argue here). In this case, the explicit version was considered too unwieldy, though it would presumably have had a tiny performance benefit. Of course, futures should be minimal cost even with fine-grained tasks, so what is acceptable for regex may not be acceptable here.

Perhaps @withoutboats recent design trick for Generators could be applied here, too. Make an explicit version for low-level stuff and build wrapper types that bridge them to the implicit higher level trait?

@carllerche

This comment has been minimized.

carllerche commented Feb 5, 2018

@Marwes

To be clear, you can use futures-rs from a no_std environment today (see here.

Making the argument explicit would arguably improve the situation for no_std, but AFAIK, nobody has hit a blocker to date.

@aturon

This comment has been minimized.

Contributor

aturon commented Feb 5, 2018

@matklad

This comment has been minimized.

matklad commented Feb 5, 2018

I am a casual reader of the RFC, however I have a question which other casual readers might have as well :)

The RFC shows changes in the poll signature, so it's clear how the implementation of the Future changes.

But what happens to the (majority) of the code that just uses futures? Should I thread this Context through each combinator? Probably not, but something has to change on the usage site?

@cramertj

This comment has been minimized.

Collaborator

cramertj commented Feb 5, 2018

@matklad If you're just using combinators, nothing should change.

@matklad

This comment has been minimized.

matklad commented Feb 5, 2018

@cramertj hm, I am still puzzled by

but in addition, one effect of having this explicit argument is that you can tell, at a call site, whether a given method involves potentially queuing the task.

I think it means that something changes for the users of futures? It probably would be helpful to see a code example along the lines of "here an echo service in async hyper before and after". Or is there literally no change unless you impl Future for MyType?

@cramertj

This comment has been minimized.

Collaborator

cramertj commented Feb 5, 2018

@matklad There should be literally no change. The sentence you quoted refers to functions like poll_read and need_read which are used by implementors of Future types.

@skade

This comment has been minimized.

skade commented Feb 5, 2018

Just thinking out loud without quite knowing where to put it:

I am wondering how much confusion around poll comes from the simple fact that it comes first in the Futures docs and doesn't make quite clear it shouldn't be called from user code in it's docs. It can be deduced from the text when you read it in full, but it never quite spells it out.

Poll is only of interest when implementing the trait and for executors.

@cramertj

This comment has been minimized.

Collaborator

cramertj commented Feb 5, 2018

it shouldn't be called from user code

I guess this depends on what you consider "user" code. Everyone I know working with futures currently has to implement futures themselves for one reason or another. I think it's good to prioritize explaining poll so that everyone (even people who don't have to manually implement poll) understands the execution model of futures-rs, which is quite different from other async libs.

@skade

This comment has been minimized.

skade commented Feb 5, 2018

I should clarify that: outside of an executor context. poll should either be called from poll or from an executor, not in the code constructing the futures.

I can see an explicit task clarifying that situation. Example:

fn main() {
    let f =  // .... some future

    f.poll() // -> panic
}

as opposed to

fn main() {
    let f =  // .... some future

    f.poll(/* now, where do I get that task from? */)
}

Then again, I wonder how much of an issue that is. People might or might not hit that, reach for an example and go on from there.

@khuey

This comment has been minimized.

khuey commented Feb 5, 2018

I've written a fair amount of futures code over the last two years. I didn't find the current model particularly difficult to learn, though I already had a large amount of experience with event loop and multithreaded programming from my days working on Firefox.

I think making the Task argument to poll explicit is probably a good idea. I'm slightly less convinced that its worth the disruption to the futures ecosystem at this point, though I expect that's likely to be manageable. In the past I've found myself writing a lot of futures chains that boil down to something like "if we have the data already return it, otherwise do a computation asynchronously and return it when done". If you attempt to poll that chain outside of a task, the "have the data already" case generally doesn't needs to wait and will complete just fine. But when we fall into the asynchronous computation branch we'll panic since we cannot wait without a task.

Someone above said that the hardest problems to debug are usually those where a future's poll impl fails to poll another future or ensure that the task is waiting for something to wake up. I tend to agree with that. It would be nice to check that statically but that seems difficult at best (since a future could poll 10 child futures and as long as any one of them waits or polls something that does it's ok).

I am curious what the intended use case for task-local storage is. I've never found myself wanting that. Typically I either have an object that I want to share across futures but it cannot be used concurrently, in which case it goes into TLS, or I have an object that I don't want to share across futures. Perhaps there are other execution models where the task is more meaningful but in futures-cpupool or tokio the task you're running on seems largely arbitrary.

@carllerche

This comment has been minimized.

carllerche commented Feb 5, 2018

I am curious what the intended use case for task-local storage is.

I am currently using it to build a tracing system. The request context can be stored in a task-local and libs can use that when adding tracing data.

This is similar to how tracing is done in finagle.

@yazaddaruvala

This comment has been minimized.

yazaddaruvala commented Feb 5, 2018

One thing I've had issues with is knowing when to use the wake API.

Just exposing the context is not as helpful as using the type system to help educate me. That said, exposing the context is the only way to better utilize the type system.

@cramertj Would it be possible to not just expose context, but use the type system to help educate users about when they need to use the task apis (similar to how, must_use on Result educates users about error handling)?

@cramertj

This comment has been minimized.

Collaborator

cramertj commented Feb 9, 2018

For those of you voicing opposition to preludes: how would you like the trait imports to work? Do you want to use futures::{Future, FutureExt, Stream, StreamExt, Sink, SinkExt}; in order to have all of the combinators available? If the answer is "yes", that's fine, but it seems rather unergonomic to me.

@cramertj

This comment has been minimized.

Collaborator

cramertj commented Feb 9, 2018

@Kixunil It references a trait object internally.

@seanmonstar

This comment has been minimized.

seanmonstar commented Feb 9, 2018

Yes, I think writing a slightly longer import once is better than the cost of every time I come back to the file, or show up to a new one, and wonder where something came from, and then look at the top and see a bunch of glob imports, and then just nope back out of the file.

@sfackler

This comment has been minimized.

Member

sfackler commented Feb 9, 2018

At least in Java-land, * imports are discouraged, and your IDE imports things for you and often folds the import block away by default.

@cramertj

This comment has been minimized.

Collaborator

cramertj commented Feb 9, 2018

@sfackler

your IDE imports things for you

Yeah, that's a key point we're missing that makes a significant difference in ergonomics. It'd also have to be pretty smart to know to implement a trait for the extension methods it adds (although rustc is smart enough to know to suggest the import, so I'd imagine RLS could do something here).

Edit: I should clarify that I don't like * imports in general, either. However, I do think preludes should be available and convenient for people who want to use them. They're an easy way to get started quickly, especially when prototyping or working with libs that have many extension traits (see Diesel's prelude, for example).

@Kixunil

This comment has been minimized.

Kixunil commented Feb 10, 2018

@cramertj why not pass around trait object itself, then? I was also thinking whether dynamic dispatch would hinder performance, but I guess it's mostly used for slow path, so that should be fine.

Speaking about IDEs, maybe that's the answer to our ergonomics problem regarding passing context around: create some standardized marker for IDEs to know that they should auto-fill that argument.

Something like this:

trait Future {
    // type ...

    fn poll(&mut self, #[auto_pass] context) -> Poll<Self::Item, Self::Error>;
}

the IDE would detect all instances where the trait is used and show it like this:

fn poll(&mut self, _) -> Poll<Self::Item, Self::Error> {
    self.foo.poll(_)
}
@matklad

This comment has been minimized.

matklad commented Feb 10, 2018

Speaking about IDEs, maybe that's the answer to our ergonomics problem regarding passing context around: create some standardized marker for IDEs to know that they should auto-fill that argument.

At least from my IDE experience, that's a too complex feature for IDEs to make it work reliably and not get in the way :) That is, it is too complex from the "how do we expose this to the user" side, not from "that's to hard to implement" side.

@Kixunil

This comment has been minimized.

Kixunil commented Feb 10, 2018

@matklad the same way IDEs can collapse blocks of code, they could collapse variables. It's just horizontal, instead of vertical. :)

IDEs should be able to know about the traits thanks to RLS.

@carllerche

This comment has been minimized.

carllerche commented Feb 10, 2018

@cramertj would it be possible to extract the task local proposal to a separate RFC? I think there is more to discuss there but it has been lost in the noise.

@illustrious-you

This comment has been minimized.

illustrious-you commented Feb 10, 2018

@seanmonstar:

That's true. My personal opinion is that a single method like context should be pretty clear, and people won't assume it's calling their own fn context(&self) method. If it were a big deal, though, we could just make them be associated functions instead, like Rc and Arc.

@aturon:

Indeed, but I suspect that WithContext would only want to provide a couple of very well-known methods (for getting the context, etc).

Speaking as someone new to Rust, I'd find this inconsistency startling in practice. Operations that manipulate the pointer should be distinct from operations that act through the pointer. In C++, for example, the methods that act on smart pointers use structure references (.), while methods that act through the pointer use pointer dereferences (->). As I understand it, the respective patterns in Rust are methods and associated functions.

That this is enforced by convention, as opposed to design, does give you the flexibility to implement a competing pattern. Without a clear and compelling reason to break from the convention, however, I'd be wary of doing so. I haven't seen such a reason. To the contrary, your responses concede @glaebhoerl's argument, indicating the decision to break with convention is arbitrary.

Have I missed something?

@seanmonstar

This comment has been minimized.

seanmonstar commented Feb 12, 2018

@zaaq:

To the contrary, your responses concede glaebhoerl's argument, indicating the decision to break with convention is arbitrary.

I can say that personally, it's because I don't particularly agree with the convention.

For instance, if I wrap something in an Arc or an Rc, I know that I can clone the pointer when I need more references. However, thanks to a (IMO ill-advised) clippy lint, people are changing my_thing.clone() into Arc::clone(&my_thing). I find that noisy and unnecessary, since it's at least obvious to me that calling clone on Arc<MyThing> is cloning the Arc.

Now, some of the other part of the convention, like make_mut, were partly made as associated functions in order to not break existing code. They were adding new methods to a smart pointer that already had existed for a long time, and so any code that had MyThing::make_mut(&self) being called from Rc<MyThing> could suddenly cause compilation errors, so it was forced into the convention.

In this case, the smart pointer is new, and so doesn't have to worry about breaking existing code. And I'm convinced that self.context() where self: WithContext<Self> is obvious. I think it would just serve to be annoying if everyone had to write WithContext::context(&mut self) instead.

@carllerche

This comment has been minimized.

carllerche commented Feb 12, 2018

My biggest hesitation of WithContext is that (unless I am missing something, which is entirely possible) it makes it noisier to call a fn on a field. Would you have to do something like:

fn foo(self: WithContext<Self>) {
    self.map(|s| &self.inner).foo_inner()
}

which seems like more effort than just passing an argument around.

@seanmonstar

This comment has been minimized.

seanmonstar commented Feb 12, 2018

Yes, you would need to do some sort of mapping to call methods of fields. I mean, in the end, this is just a compromise since others really wanted explicit arguments and I didn't want to die on this hill.

It does have benefits when calling multiple methods of self that all would have wanted access to context, though.

@cramertj cramertj merged commit 2f457fa into rust-lang-nursery:master Feb 12, 2018

@cramertj

This comment has been minimized.

Collaborator

cramertj commented Feb 12, 2018

Huzzah! This RFC has been merged. We'll start off by introducing task::Context, and we can iterate further on the importing ergonomics as necessary.

@carllerche I've added an unresolved question about the task-local accessors and opened rust-lang-nursery/futures-rs#753 to follow-up.

@pythonesque

This comment has been minimized.

pythonesque commented Apr 7, 2018

@udoprog

I've never dealt with panic safety before, so I'm not sure if this prevents us from being so (we have &mut T, and RefCell for local storage). Is the current TLS context even strictly speaking panic safe?

Sorry to answer your question so late, but: no. TLS (in Rust as it is currently implemented) isn't generally panic safe and can't be detected as such. It's an unfortunate tradeoff, but it's one we made primarily because people don't use TLS in Rust very much... which, not to beat a dead horse, is one of the reasons I'm glad this RFC was accepted :)

@bstrie bstrie referenced this pull request May 12, 2018

Open

Tracking issue for `arbitrary_self_types` #44874

0 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment