Hidden pitfalls in Conc::Map's MRI strategy?

[thedarkone]

There is a report by a Rails user (rails/rails#24627) that on MRI Concurrent::Map's @backend (a pure Ruby Hash) is throwing "insertion during iteration" errors.

As pointed out by @jeremy, there are couple of candidates:

NonConcurrentMapBackend#clear

The method calls @backend.clear, the C implementation (https://github.com/ruby/ruby/blob/2f6821a7c4e7a52d/hash.c#L1488) of Hash#clear might invoke rb_hash_foreach, that in turn bumps iter level and after some preparation dispatches into st_foreach_check (https://github.com/ruby/ruby/blob/2f6821a7c4e7a52d/st.c#L890).

While in st_foreach_check I don't see anyway for MRI to yield to Ruby land (release GVL), except for the provided iterator fun, but in the case of Hash#clear https://github.com/ruby/ruby/blob/2f6821a7c4e7a52d/hash.c#L1465 clear_i also doesn't yield to Ruby.

If the code never releases GVL, it is "atomic", the elevated iter level is also atomic and is never exposed to other Ruby code.

Conclusion

NonConcurrentMapBackend#clear is safe: Hash#clear never yields GVL.

---

NonConcurrentMapBackend#value?

The method calls @backend.value?, the C implementation (https://github.com/ruby/ruby/blob/2f6821a7c4e7a52d/hash.c#L2050) of Hash#value? uses rb_hash_foreach with rb_equal (https://github.com/ruby/ruby/blob/2f6821a7c4e7a52d/hash.c#L2029) callback, rb_equal might invoke custom Ruby-level methods.

Conclusion

NonConcurrentMapBackend#value? is not safe: Hash#value? might yield GVL.

Solution

Remove custom value? impl in NonConcurrentMapBackend, let the default method (https://github.com/ruby-concurrency/concurrent-ruby/blob/master/lib/concurrent/map.rb#L147) do the work.

---

NonConcurrentMapBackend#initialize_copy

The method doesn't call Hash#initialize_copy, the copying work is performed higher up in Map#populate_from (https://github.com/ruby-concurrency/concurrent-ruby/blob/master/lib/concurrent/map.rb#L216), this in turn relies on Map#each_pair.

Conclusion

NonConcurrentMapBackend#initialize_copy is safe as long as Map#each_pair is safe.

---

NonConcurrentMapBackend#each_pair

The method calls @backend.dup (in dupped_backend). Hash duping is then done via Hash#initialize_copy, the C implementation (https://github.com/ruby/ruby/blob/2f6821a7c4e7a52d/hash.c#L1581) uses st_copy to copy the underlying hash table structure, while doing so, it doesn't yield to Ruby. After the copying is done, the new copy is rehashed by rb_hash_rehash, however we don't care about what happens in rb_hash_rehash: it operates on a thread local copy.

Conclusion

NonConcurrentMapBackend#each_pair is safe: Hash#dup might yield GVL only while working on thread local data.

---

So we need to fix value? otherwise everything looks fine?

[jeremy]

Consider #inspect as well, since default implementation inspects an objects ivars. It'll inspect the backend, iterating it.

[jdantonio]

👍 to everything @thedarkone said. You are the expert on this. I trust your judgement. Thanks again for looking into it. And not just looking into it, but diving so deep into the Ruby internals and providing such great detail. You are awesome!