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

Missing Send bound in rayon-futures 0.1 #697

Closed
jClaireCodesStuff opened this issue Sep 18, 2019 · 3 comments
Closed

Missing Send bound in rayon-futures 0.1 #697

jClaireCodesStuff opened this issue Sep 18, 2019 · 3 comments

Comments

@jClaireCodesStuff
Copy link

ScopeFutureExt::spawn_future should also have Send bounds on the output types of F

<F as Future>::Item: Send,
<F as Future>::Err: Send,

Sync and Send are unsafely derived for ScopeFuture and the comment asserts that the implementation of S is more thread-safe than it appears. However this unsafe impl has the side effect of not checking the other types which are members of ScopeFutureContents, including

result: Poll<CUItem<F>, CUError<F>>,
             ^^^^^^^^^  ?Send but will be sent between threads 

I'm porting rayon-futures to use version 0.3 of the futures crates and found this bug by trying to automatically implement Sync and Send. I haven't yet looked into whether it is safe to assume ScopeHandle: Send (it doesn't feel good - adding that trait bound in rayon-core throws errors), but even assuming it's safe I think it would be better to make that assumption only for the scope: Option<S> field and let the compiler check the rest.

I can't promise that I'll have time to fix this within the next few days, so I'm reporting it for triage. But I'll make at least a simple fix my first priority with whatever time I do find.

@cuviper
Copy link
Member

cuviper commented Sep 18, 2019

Yes, I think you're right.

I'm porting rayon-futures to use version 0.3 of the futures crates

See also #679, though that's probably not going to be its final form. @nikomatsakis wants to open an RFC to go through the questions raised in that PR. Regardless, I think we will want something based on std::future and std::task alone, without any extra crate dependency.

Are you already using rayon-futures as it is now? We could really benefit from some real-world use cases to help with the design.

@jClaireCodesStuff
Copy link
Author

Are you already using rayon-futures as it is now?

Not enough to have a particularly useful opinion about the best design. It's more that I read the code apropos to something else (maybe even idle curiosity, more likely a question on Reddit) and realized I have the skills to port it.

Whether rayon should act as an executor or simply offer an async variant of spawn is an interesting question - I'd like to know the answer. I do think the latter would be fairly easy to implement. I was able to get rid of just about everything Notify and replace it with the ArcWake trait. Even if you don't want that dependency, it should be straightforward to write a RawWakerVTable.

I think I have the bug itself solved, but I couldn't figure out how to make a compile_fail test work. The compiler doesn't seem to import additional crates, only rayon_futures itself.

#698

@cuviper
Copy link
Member

cuviper commented Dec 22, 2019

#698 is in rayon-futures 0.1.1.

@cuviper cuviper closed this as completed Dec 22, 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

2 participants