Implement a Send'able Handle for the single-threaded Runtime to spawn… #340

sdroege · 2018-05-03T14:02:45Z

… new tasks

sdroege · 2018-05-03T14:06:39Z

src/runtime/current_thread/runtime.rs

+        // Create an mpsc channel for scheduling new tasks on this runtime
+        // from a different thread
+        // XXX: Should this use a bounded channel with an arbitrary limit instead, and then
+        // panic spawning if the channel is full?


Are there any opinions on this?

Also some further work is needed so that run() ignores our channel instead of just running forever

I'm not sure how to make run() not wait. The way how I solved that in my code is that I'm not using a channel but just a Mutex<VecDequeue> and then use turn() instead of run() and unpark whenever a future is scheduled.

Any better ideas?

... let's just move it into the executor directly. There there's control over all that :)

sdroege · 2018-05-03T14:07:10Z

src/runtime/current_thread/runtime.rs

+        // XXX: Should this use a bounded channel with an arbitrary limit instead, and then
+        // panic spawning if the channel is full?
+        let (spawn_sender, spawn_receiver) = mpsc::unbounded();
+        executor.spawn(spawn_receiver.for_each(|future| {


We might want to do this only the first time that handle() is called, but it complicates things slightly. Opinions?

carllerche · 2018-05-11T18:06:10Z

Ping me when you think this is reviewable 👍

sdroege · 2018-05-11T18:41:44Z

Thanks, will do! Unfortunately got side-tracked with other things, but hopefully will have some time this weekend

sdroege · 2018-05-14T11:58:56Z

Sorry for the delay, this is now updated to have the handle implemented directly on the executor (and re-exported from the runtime). And it has a test

@carllerche this should be good for reviewing now. Only open question from my side is whether this should actually be an unbounded channel, and if there should be a standalone function current_thread::handle that gives the handle of the current thread's executor

tikue · 2018-05-15T18:12:29Z

Spawning a future is already an unbounded action, so making the channel unbounded isn't changing anything.

sdroege · 2018-05-18T06:39:12Z

@tikue That was also my thought, and if you spawn too many futures you'll probably have another problem already before that channel is becoming one

sdroege · 2018-05-29T16:39:07Z

Any other comments, how should we move forward with this? :)

carllerche

Sorry for the delay (stuff came up)! Looks like a great start. I posted some feedback inline.

carllerche · 2018-05-29T18:19:16Z

src/executor/current_thread/mod.rs

@@ -304,10 +311,16 @@ impl<P: Park> CurrentThread<P> {
    pub fn new_with_park(park: P) -> Self {
        let unpark = park.unpark();

+        // XXX: Should this use a bounded channel with an arbitrary limit instead, and then
+        // panic spawning if the channel is full?
+        let (spawn_sender, spawn_receiver) = mpsc::unbounded();


Unbounded is fine here. Generally, spawn fns do not have back pressure. Instead, they either succeed or fail. One accepted pattern is for the executor to have a max # of tasks and spawn to fail if that is reached. This feature can be punted to another PR though.

Removed the comment

carllerche · 2018-05-29T18:22:45Z

src/executor/current_thread/mod.rs

+    spawn_handle: Handle,
+
+    /// Receiver for futures spawned from other threads
+    spawn_receiver: executor::Spawn<mpsc::UnboundedReceiver<Box<Future<Item = (), Error = ()> + Send + 'static>>>


This can probably be a plain old queue. There is no need to pull in all the future enabled stuff since this is for an executor. The easiest thing here is probably to use std::sync::mpsc and try_recv.

Thanks, it indeed is much simpler with a normal mpsc queue. Changed it accordingly

carllerche · 2018-05-29T18:23:29Z

src/executor/current_thread/mod.rs

+        let notify = borrow.scheduler.notify();
+
+        loop {
+            let res = borrow.enter(self.enter, || {


A lot of this can probably be simplified by not using the future enabled channel.

carllerche · 2018-05-29T18:25:04Z

src/executor/current_thread/mod.rs

+    ///
+    /// This function panics if the spawn fails. Failure occurs if the `CurrentThread`
+    /// instance of the `Handle` does not exist anymore.
+    pub fn spawn<F>(&self, future: F)


For forwards compat, we will probably want to return Result here (matching https://docs.rs/tokio/0.1.6/tokio/executor/trait.Executor.html#tymethod.spawn).

carllerche · 2018-05-29T18:25:21Z

src/runtime/current_thread/runtime.rs

@@ -8,6 +8,9 @@ use futures::Future;

 use std::io;

+// Re-export Handle from here
+pub use executor::current_thread::Handle;


For forwards compat, I would wrap this in a new type.

…entThread executor to spawn new tasks

…ed on the Handle works

…ability

…bility ... and consistency with the other spawn() functions.

…rentThread executor This simplifies the code quite a bit and is doing all we need anyway.

sdroege · 2018-05-30T11:11:09Z

Applied all the review comments in separate commits and rebased everything on top of latest master. Thanks for the review!

sdroege · 2018-06-04T17:57:52Z

@carllerche is there anything else left to be done here?

carllerche

Looks good to me 👍 Thanks for sticking with it.

sdroege commented May 3, 2018

View reviewed changes

sdroege force-pushed the current-thread-handle branch from a4dbc16 to 36145e0 Compare May 14, 2018 11:56

carllerche requested changes May 29, 2018

View reviewed changes

sdroege added 2 commits May 30, 2018 13:31

Implement a Send'able Handle for the single-threaded Runtime and Curr…

c45fdcd

…entThread executor to spawn new tasks

Add another test that ensures that unparking because of futures spawn…

b028dfd

…ed on the Handle works

sdroege force-pushed the current-thread-handle branch from 5e60587 to f5c5ae5 Compare May 30, 2018 11:10

sdroege added 3 commits May 30, 2018 14:10

Use a newtype wrapper for the Handle in the Runtime for future extens…

4250bb1

…ability

Return Result<(), SpawnError> from Handle::spawn() for future extensa…

ef468e1

…bility ... and consistency with the other spawn() functions.

Use a non-future-enabled mpsc channel for spawning futures on the Cur…

f66538a

…rentThread executor This simplifies the code quite a bit and is doing all we need anyway.

sdroege force-pushed the current-thread-handle branch from f5c5ae5 to f66538a Compare May 30, 2018 11:10

carllerche approved these changes Jun 5, 2018

View reviewed changes

carllerche merged commit 0d41ba7 into tokio-rs:master Jun 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement a Send'able Handle for the single-threaded Runtime to spawn… #340

Implement a Send'able Handle for the single-threaded Runtime to spawn… #340

sdroege commented May 3, 2018

sdroege May 3, 2018

sdroege May 4, 2018

sdroege May 7, 2018

sdroege May 3, 2018

carllerche commented May 11, 2018

sdroege commented May 11, 2018

sdroege commented May 14, 2018

tikue commented May 15, 2018

sdroege commented May 18, 2018

sdroege commented May 29, 2018

carllerche left a comment

carllerche May 29, 2018

sdroege May 30, 2018

carllerche May 29, 2018

sdroege May 30, 2018

carllerche May 29, 2018

sdroege May 30, 2018

carllerche May 29, 2018

sdroege May 30, 2018

carllerche May 29, 2018

sdroege May 30, 2018

sdroege commented May 30, 2018

sdroege commented Jun 4, 2018

carllerche left a comment

Implement a Send'able Handle for the single-threaded Runtime to spawn… #340

Implement a Send'able Handle for the single-threaded Runtime to spawn… #340

Conversation

sdroege commented May 3, 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

carllerche commented May 11, 2018

sdroege commented May 11, 2018

sdroege commented May 14, 2018

tikue commented May 15, 2018

sdroege commented May 18, 2018

sdroege commented May 29, 2018

carllerche 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

sdroege commented May 30, 2018

sdroege commented Jun 4, 2018

carllerche left a comment

Choose a reason for hiding this comment