-
Notifications
You must be signed in to change notification settings - Fork 507
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
Add experimental support for futures #193
Conversation
use std::sync::mpsc::channel; | ||
let (tx, rx) = channel(); | ||
let a = s.spawn_future(lazy(move || Ok::<usize, ()>(rx.recv().unwrap()))); | ||
// ^^^^ FIXME: why is this needed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rx.recv
only requires &self
, so the closure by default captures &rx
, which doesn't live outside scope
, which the future is required to outlast, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I think I thought rx.recv
was fn(self)
That all sounds great to me! To double check my own understanding as well, a lot of this is very similar to basically: fn spawn_rayon<F: Future>(f: F) -> RayonFuture {
let (tx, rx) = oneshot::channel();
rayon.spawn(|| tx.send(f.wait()));
return tx
} Except that this solves a few crucial problems when literally using a "oneshot"
I'm curious, what's your thinking here? E.g. what would a stream look like? I could imagine that
Yeah this is the general trend of "those things that can spawn futures" right now. We're likely to consider a unifying spawn trait (rust-lang/futures-rs#313) in which case this'd come for free. I haven't thought too much about the trait here, but we'd definitely want to consider rayon when designing it!
The general trend is to have
There's some prior art on |
I think that is true.
I don't know! I haven't looked at all at streams really. Maybe I'm wrong and there isn't a role for Rayon here? But that seems surprising to me.
Makes sense. @carllerche also pointed me at this repository also. One thing I noticed there is that some of the wrappers seem like they would result in >1 allocation per future which, yes, I was trying to avoid.
No, but that's interesting. I could add such a method. That said, it doesn't suffice really. The goal of the |
@nikomatsakis oh that all makes sense to me. Looking forward to see how this turns out! |
@alexcrichton fyi I simplified the cancellation semantics per our discussion. Now it just sets a flag and unparks, basically. |
👍 |
@alexcrichton so I was looking at making the handling of |
For now I settled with "remember the most recent |
Yep, that's the same assumption we make throughout the library. |
This permits us tighter ordering bounds.
This avoids an allocation and puts us in complete control.
We had some oversight with the old structure.
`LatchProbe` is a latch that can only be probed.
Also retool and add more tests.
Now that the `Park` trait in futures has no `'static` bound, we can do away with it.
Now, if we call `unpark()`, and that panics, we will propagate this to the enclosing `scope`.
So I think I've hardened the code against most sources of user panics. One thing we are not protected against, but I've decided it's a hopeless battle, is if the future's Examples of what make this so hard: If the future's drop panics, it actually triggers during the I think it makes sense at least for now to accept that if your |
OK, I think this branch is basically ready to go, though it would be good to add a few more tests. |
@cuviper -- would you like to review the logic here? I've gone over it once with @alexcrichton. I decided to keep this under the |
if WorkerThread::current().is_null() { | ||
executor::spawn(self).wait_future() | ||
} else { | ||
panic!("using `wait()` in a Rayon thread is unwise; try `rayon_wait()`") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just do the right thing? i.e. make rayon_wait
the true RayonFuture::wait
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I don't want people to think that calling wait()
in Rayon code is OK. It only works for a RayonFuture
-- any other kind of future will do the wrong thing. Therefore, I want them to write rayon_wait()
so that, if they happen to invoke it on some random future, it will error out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example how this could go wrong if I were encouraging people to call wait
:
let v = scope.spawn_future(f).and_then(|x| x + 1).wait();
Here wait
is being called on an AndThen<RayonFuture<F>>
. But if they had written .rayon_wait()
, then it would have failed to compile. What would work is:
let v = scope.spawn_future(scope.spawn_future(f).and_then(|x| x + 1)).rayon_wait();
Although you'd be better off not spawning twice:
let v = scope.spawn_future(f.and_then(|x| x + 1)).rayon_wait();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to make RayonFuture::wait
a compile-time error, rather than a panic? Probably not, since we don't control the trait, but it's ugly that this will only show up at runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, there is no way to do that; but I think the right thing is for the futures library to offer some sort of hook (perhaps via a thread-local?) to customize how wait
behaves. That said, the truth is that if you are using wait()
-- even rayon_wait()
-- you are probably using futures wrong. The right thing would be to make a "follow-up" future and schedule that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the docs for wait()
do warn that using it can will lead to deadlock, however.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that any references are still valid, or else the user shouldn't be | ||
able to call `poll()`. (The same is true at the time of cancellation, | ||
but that's not important, since `cancel()` doesn't do anything of | ||
interest.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes me nervous, as it probably should since you bothered to document it so carefully. But I don't have any concrete objection, and I trust your intuition on this more than my own, so... 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not the most straight-forward bit of reasoning, so being nervous is reasonable. However, I don't really think there's a reason to be nervous about accessing the result (famous last words...). As I wrote, the fundamental premise of Rust's type system is that T
and E
must either be in scope (i.e., only contain live, valid references) or else no data of that type must actually be reachable (this can occur in corner cases). Since the type of the result contains only T
and E
, it had better be valid or else Rust is pretty fundamentally broken.
What could make you nervous is that I've transmuted the type to hide the other data in the struct, and hence THOSE fields (if they had references) might not be in scope. An example would be the spawn
field, which contains a value of type F
(the future type). However, that shouldn't be a problem, both because the code doesn't access those fields and because we set them to None
(so there is in fact no data of type F
reachable).
@cuviper -- any objection to me landing this? Naturally we can iterate on the API (including the (One thing I wouldn't mind talking over, and I may try to write-up an RFC issue or something, is the relationship of |
No objection, especially since it's marked unstable. :-)
|
Done! Gonna be time for a new release soon, I think. |
This branch adds a new method to a scope:
spawn_future(F)
. Thespawn_future
API allows Rayon to play the role of an executor. That is, it takes some futureF
and gives it life, causing it to start executing. The result is another future, called a rayon future, which can be used to check if the result of F is ready.The role of Rayon and futures
To understand the role Rayon plays here, recall that a future F is basically the plan for an async computation, much like an iterator is a plan for a loop. Thus a future by itself is inert. When you invoke
spawn_future()
, however, Rayon starts to put that plan into action: it pushes a job to a worker thread which will invokepoll()
on the future F. This will trigger various bits of work to be done and may wind up blocked on I/O requests and the like. In the meantime, you get back another future F' that you can use to check on the status of this work or to compose new futures.The simplest usage pattern, where you just want to push some work to another thread and then block on it and use the result, is like so:
Note the use of the
rayon_wait()
method instead ofwait()
--rayon_wait()
will block intelligently, so that even if you are on a Rayon worker thread the system doesn't seize up. However, blocking is not the recommended way to use futures. Instead, it would be better to compose newer and bigger futures that use the result from spawn -- or, better yet, compose the futures before you spawn. If you must block, block at the very end:cc @alexcrichton @aturon @carllerche -- please double-check my understanding here :)
Comparing
spawn()
andspawn_future()
So how does
spawn_future()
compare to the existingscope.spawn()
? The rule of thumb is thatspawn()
is used to launch a computation for side-effects whereasspawn_future()
is used to launch a computation for its result. You can observe the difference when it comes to the result type:spawn()
takes a closure that returns()
, so if you want to get any value out, it must write it somewhere external. In contrast, the future you give tospawn_future()
has a result type.Another place that this difference is important is cancellation. If you drop the future that is returned by
spawn_future()
, that is interpreted as a signal that you no longer care about that result. This will cause the spawned future to stop executing, possibly before its complete. This is a key mechanism used throughout the futures library to signal when results are no longer needed and hence avoid doing useless work. In contrast, once you spawn a task withs.spawn()
, it will always execute. There is no way to cancel it.API questions
API-wise, I've kept this to the bare minimum for the moment, simply adding
spawn_future()
. My general plan however is to do the following:spawn_future_fn()
wrappers that, instead of taking a future, takes a closure and create a future for its result (usingfuture::lazy
). We probably want one for closures that returnResult
(in which case the future is fallible) and one for an "infallible" computation (this would wrap the result of the closure inOk()
, basically).spawn_async()
andspawn_future_async()
. These are analogous to thescope()
methods but they execute outside of any scope, just injecting spawned jobs into the asynchronous thread-pool. The idea is that there is (conceptually) always an outermost scope that you don't have a handle to. This would solve the Servo use-case of wanting to inject work into a parallel thread-pool and query its result later (ideally, you would usespawn_future_async()
for that, since it fits into the "inject job for result" use-case, not "inject job for side-effects").I'd probably pursue all of those in follow-up PRs.
Status
Could use more tests but I think it's good to go.
Work items:
unpark()
could panicunpark()
illegal? rust-lang/futures-rs#318