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

Make Concurrent::Set thread-safe on CRuby #906

Conversation

flavorjones
Copy link
Contributor

See #900 for original issue pointing out that Concurrent::Set#each (and other methods as well) are not thread-safe because the implementation was short-circuited to be ::Set.

This PR adds two commits:

  • one introducing test coverage for thread-safe usage of #each combined with #add and #delete
  • a second introducing a CRuby-specific Set implementation that uses Monitor to synchronize across all instance methods

This approach, using a new method ThreadSafe::Util.make_synchronized_on_cruby, borrows heavily from the approach used for Rubinius (see ThreadSafe::Util.make_synchronized_on_rbx).

Please let me know if this approach is too naïve, I fear I may be missing something obvious.

@flavorjones
Copy link
Contributor Author

The test failures don't seem to be related to this change.

@chrisseaton
Copy link
Member

Yes sorry it looks like the CI has bit-rotted out from underneath us. We should switch to GitHub Actions quickly.

@pitr-ch pitr-ch self-requested a review June 3, 2021 08:22
@pitr-ch pitr-ch added the enhancement Adding features, adding tests, improving documentation. label Jun 3, 2021
@pitr-ch pitr-ch force-pushed the flavorjones-900-cruby-concurrent-set branch from 74b3329 to f37ef9f Compare June 3, 2021 09:00
@pitr-ch
Copy link
Member

pitr-ch commented Jun 4, 2021

Replaced by #911

@pitr-ch pitr-ch closed this Jun 4, 2021
@flavorjones flavorjones deleted the flavorjones-900-cruby-concurrent-set branch June 5, 2021 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Adding features, adding tests, improving documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants