ruby-concurrency / concurrent-ruby

Modern concurrency tools including agents, futures, promises, thread pools, supervisors, and more. Inspired by Erlang, Clojure, Scala, Go, Java, JavaScript, and classic concurrency patterns.
https://ruby-concurrency.github.io/concurrent-ruby/
Other
5.71k stars 420 forks source link

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

Closed grncdr closed 3 years ago

grncdr commented 3 years ago

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 commented 3 years ago

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?