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

[documentation] Concurrent::Set documentation is ambiguous, possibly misleading. #900

Closed
grncdr opened this issue Jan 26, 2021 · 1 comment · Fixed by #911
Closed

[documentation] Concurrent::Set documentation is ambiguous, possibly misleading. #900

grncdr opened this issue Jan 26, 2021 · 1 comment · Fixed by #911
Labels
bug A bug in the library or documentation. high-priority Should be done ASAP. looking-for-contributor We are looking for a contributor to help with this issue.

Comments

@grncdr
Copy link

grncdr commented Jan 26, 2021

The docs for Concurrent::Set state (emphasis mine)

This version locks against the object itself for every method call, ensuring only one thread can be reading or writing at a time. This includes iteration methods like #each.

However, on CRuby, the implementation of Concurrent::Set is just the built-in Set (ref).

Clearly, the built-in set does not perform any locking around method calls. This caused a few bugs for me when I used Concurrent::Set it for sharing state between threads. I had one thread iterating the set with #each and other threads adding/deleting items from the set. This failed, since CRuby Set doesn't support mutating a set during iteration.

Would a PR updating the docs to call this out explicitly be welcome? I would propose something like:

This version locks against the object itself for every method call, ensuring only one thread can be reading or writing at a time. This includes iteration methods like #each.

NOTE: On CRuby this is simply an alias for Set and does not support concurrent modification during iteration. Prefer Concurrent::Map instead.

* Ruby implementation:             Ruby
* `concurrent-ruby` version:       1.1.8, but this applies to all versions afaict
@pitr-ch pitr-ch added bug A bug in the library or documentation. high-priority Should be done ASAP. looking-for-contributor We are looking for a contributor to help with this issue. labels Jan 27, 2021
@pitr-ch
Copy link
Member

pitr-ch commented Jan 27, 2021

Hi @grncdr, thanks for the report. It would be better to fix the implementation on CRuby though. It should also lock during the iteration since that is the behaviour of Concurrent::Set on JRuby and TruffleRuby. Could you contribute the fix please?

flavorjones added a commit to flavorjones/concurrent-ruby that referenced this issue Apr 20, 2021
flavorjones added a commit to flavorjones/concurrent-ruby that referenced this issue Apr 20, 2021
@pitr-ch pitr-ch assigned grncdr and unassigned grncdr Jun 3, 2021
pitr-ch pushed a commit to flavorjones/concurrent-ruby that referenced this issue Jun 3, 2021
pitr-ch pushed a commit to flavorjones/concurrent-ruby that referenced this issue Jun 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug in the library or documentation. high-priority Should be done ASAP. looking-for-contributor We are looking for a contributor to help with this issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants