Skip to content
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

Change Executors to take &self #1344

Closed
seanmonstar opened this issue Jul 22, 2019 · 5 comments
Closed

Change Executors to take &self #1344

seanmonstar opened this issue Jul 22, 2019 · 5 comments
Milestone

Comments

@seanmonstar
Copy link
Member

The Executor and TypedExecutor traits both take &mut self in their spawn method. While this does make sense logically, it has this downside: you can't share an executor with an Arc (for example, an Arc<dyn Executor>). You can put the executor inside a Mutex, however most executors already can handle synchronization internally, frequently with some lock-free queue.

It might be a good idea to switch the spawn function to only take &self.

An alternative option is to leave them as &mut self, and implement Executor for &T for those that already have internal synchronization.

@seanmonstar seanmonstar added this to the v0.2 milestone Jul 22, 2019
@LucioFranco
Copy link
Member

@seanmonstar is there benefit to having them take &self over just having a cheap clone internally?

@seanmonstar
Copy link
Member Author

The difficulty I ran into was when using it as a trait object, because then you can't make use of a cheap clone.

@LucioFranco
Copy link
Member

Ah that makes a lot of sense, I think that is a good idea then!

@carllerche
Copy link
Member

@seanmonstar the trait obj argument is solid. I am 👍

@hawkw
Copy link
Member

hawkw commented Nov 15, 2019

I think this issue is no longer relevant, as the Executor and TypedExecutor traits no longer exist.

@hawkw hawkw closed this as completed Nov 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants