Skip to content

/rayon

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

Tighten Send and Sync requirements #443

Merged
merged 4 commits into from Sep 25, 2017

Conversation

Reviewers

@nikomatsakis nikomatsakis

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@cuviper @nikomatsakis
@cuviper
Copy link
Member

cuviper commented Sep 19, 2017

This is a breaking change to rayon-core, necessary for soundness.

  • Scope::spawn now requires Send for the closure. Fixes #430.

  • ThreadPool::install now requires Send for the return value.

  • (unstable) internal::Task now requires Send + Sync instead of just
    Send, so its use of Arc properly implements Send and Sync too.
@cuviper

cuviper reviewed Sep 19, 2017
View changes

rayon-core/src/scope/mod.rs Outdated
@@ -278,7 +285,7 @@ impl<'scope> Scope<'scope> {

// the `Scope` is not send or sync, and we only give out
// pointers to it from within a worker thread
debug_assert!(!WorkerThread::current().is_null());
debug_assert!(!worker_thread.is_null());

This comment has been minimized.

Sign in to view
Copy link
@cuviper

cuviper Sep 19, 2017

Author Member

Note, Scope now does implement Sync, so this comment isn't true anymore. We need to evaluate whether that's actually safe. I think here, this !is_null() should probably be a full assert!, and perhaps we assert that the current registry matches that saved in the Scope too.
@cuviper

cuviper reviewed Sep 19, 2017
View changes

rayon-core/src/scope/mod.rs Outdated
@@ -278,7 +285,7 @@ impl<'scope> Scope<'scope> {

// the `Scope` is not send or sync, and we only give out
// pointers to it from within a worker thread
debug_assert!(!WorkerThread::current().is_null());
debug_assert!(!worker_thread.is_null());

let worker_thread = &*worker_thread;
worker_thread.push(job_ref);

This comment has been minimized.

Sign in to view
Copy link
@cuviper

cuviper Sep 19, 2017

Author Member

Maybe this can just switch to scope.registry.inject_or_push(job_ref), so if the scope does get carried out of the threadpool, it still spawns back to the right pool. The 'scope lifetime should still ensure the right stuff...

This comment has been minimized.

Sign in to view
Copy link
@cuviper

cuviper Sep 19, 2017

Author Member

Done.

@cuviper cuviper requested a review from nikomatsakis Sep 19, 2017

@cuviper cuviper force-pushed the cuviper:job-sync branch from f780bcd to dea9acf Sep 19, 2017

@nikomatsakis

nikomatsakis reviewed Sep 22, 2017
View changes

rayon-core/src/scope/mod.rs Outdated

let worker_thread = &*worker_thread;
worker_thread.push(job_ref);
self.registry.inject_or_push(job_ref);

This comment has been minimized.

Sign in to view
Copy link
@nikomatsakis

nikomatsakis Sep 22, 2017

Member

Let's add a comment here saying that it would be unsafe to just invoke push before we don't know that the scope was not somehow sent to another thread.

This comment has been minimized.

Sign in to view
Copy link
@cuviper

cuviper Sep 23, 2017

Author Member

Done.
@nikomatsakis

nikomatsakis requested changes Sep 22, 2017
View changes

Copy link
Member

nikomatsakis left a comment

Left a nit with a minor documentation change, but otherwise looks good.

cuviper added some commits Sep 19, 2017

@cuviper
Tighten Send and Sync requirements 
This is a **breaking change** to `rayon-core`, necessary for soundness.

- `Scope::spawn` now requires `Send` for the closure.  Fixes #430.

- `ThreadPool::install` now requires `Send` for the return value.

- (unstable) `internal::Task` now requires `Send + Sync` instead of just
`Send`, so its use of `Arc` properly implements `Send` and `Sync` too.
cfd59b4
@cuviper
use `inject_or_push` in `Scope::spawn`
Loading status checks…
829c1ea

@cuviper cuviper force-pushed the cuviper:job-sync branch from dea9acf to 829c1ea Sep 23, 2017

@cuviper cuviper referenced this pull request Sep 23, 2017

Closed

Nesting `.install` calls causes deadlock #446

cuviper added some commits Sep 23, 2017

@cuviper
Allow `ThreadPool::install` from within the pool 
This adds `Registry::in_worker()`, akin to the free `fn in_worker()` but
for a particular thread pool.  If this is called from a thread that's
part of that pool, it just executes the `op` directly.

`ThreadPool::install()` now simply forwards to `Registry::in_worker()`.

Fixes #446.
03e5d2d
@cuviper
Document `install()` inter-pool deadlocks
Loading status checks…
23a4489
@cuviper

This comment has been minimized.

Sign in to view
Copy link
Member Author

cuviper commented Sep 23, 2017

Added a fix for #446, since that also needed the change to install() here.
@nikomatsakis

nikomatsakis approved these changes Sep 25, 2017
View changes

@nikomatsakis nikomatsakis merged commit 3217551 into rayon-rs:master Sep 25, 2017
2 checks passed

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@cuviper cuviper deleted the cuviper:job-sync branch Oct 6, 2017

@cuviper cuviper referenced this pull request Dec 7, 2018

Merged

scope scheduling RFC #1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.