Skip to content

executor: add spawner #2

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

Merged
1 commit merged into from
Jul 23, 2020
Merged

executor: add spawner #2

1 commit merged into from
Jul 23, 2020

Conversation

Keruspe
Copy link
Contributor

@Keruspe Keruspe commented Jul 23, 2020

This allows spawning a task on an executor from outside of it

Fixes #1

@ghost
Copy link

ghost commented Jul 23, 2020

Would this also need the LocalSpawner counterpart?

Perhaps instead of Task::<()>::spawner() it would make more sense to have Spawner::get() or Spawner::current() :)

Does this really require changes to multitask? We could have:

pub struct Executor {
    ex: Arc<multitask::Executor>,
}

pub struct Spawner {
    ex: Arc<multitask::Executor>,
}

@Keruspe
Copy link
Contributor Author

Keruspe commented Jul 23, 2020

Sure, don't know why I didn't sync of the Arc version :)

I did the Task::spawner to be symmetrical with Task::spawn but Spawner::current() looks a lot like the Handle::current() from tokio, fine by me

@ghost
Copy link

ghost commented Jul 23, 2020

Another argument for current() is that it matches https://doc.rust-lang.org/nightly/std/thread/fn.current.html

@Keruspe
Copy link
Contributor Author

Keruspe commented Jul 23, 2020

Wrt the LocalSpawner counterpart, I'm not sure if that's necessary. We can add it for symmetry, but if you want to spawn something on the local executor, thus on the current thread, there's a huge change you're already in it right?

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just minor nits...

@ghost
Copy link

ghost commented Jul 23, 2020

I guess you're right, LocalSpawner would be kind of useless :)

This allows spawning a task on an executor from outside of it

Fixes smol-rs#1

Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com>
@Keruspe
Copy link
Contributor Author

Keruspe commented Jul 23, 2020

Fixed the comments

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@ghost ghost merged commit 202e2bd into smol-rs:master Jul 23, 2020
@ghost
Copy link

ghost commented Jul 23, 2020

Published in v0.1.2

@Keruspe
Copy link
Contributor Author

Keruspe commented Jul 23, 2020

Thanks!

Do you think smol will reexport this or will this stay in the async_executor namespace?

@ghost
Copy link

ghost commented Jul 23, 2020

I think it will stay.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Allow spawning task from outside an Executor
1 participant