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

spawn_unsafe should require that F: Send #54

Closed
jonhoo opened this issue Feb 15, 2016 · 4 comments · Fixed by #55
Closed

spawn_unsafe should require that F: Send #54

jonhoo opened this issue Feb 15, 2016 · 4 comments · Fixed by #55

Comments

@jonhoo
Copy link
Contributor

jonhoo commented Feb 15, 2016

The spawn_unsafe method in Crossbeam should include a bound on the function f: F that it is also Send. Otherwise, you may end up calling functions that access non-thread-safe things like UnsafeCell. This also brings the definition more in line with that of std::thread::spawn. Specifically, the signature should be:

pub unsafe fn spawn_unsafe<'a, F>(f: F) -> JoinHandle<()> 
where F: FnOnce() + Send + 'a
@aturon
Copy link
Collaborator

aturon commented Feb 16, 2016

The bounds were deliberately left off to provide a maximally flexible, unsafe spawning function -- i.e., to cover even the case where you wanted to work with a non-Send closure unsafely.

That said, the flexibility isn't needed for scoped threads, and of course does add to the overall danger of this function. Arguably, this treatment of Send might be better accomplished through a newtype wrapper anyway. So I'm open to adding the bound.

@jonhoo
Copy link
Contributor Author

jonhoo commented Feb 16, 2016

An alternative might be to provide two unsafe functions instead of one, where one requires Send, and its use is recommended where possible.

@aturon
Copy link
Collaborator

aturon commented Feb 16, 2016

Given that there's not a pressing need for a version without Send, I'd accept a PR that just added Send to the current method.

@jonhoo
Copy link
Contributor Author

jonhoo commented Feb 16, 2016

PR submitted: #55.

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

Successfully merging a pull request may close this issue.

2 participants