Fix `ReentrantReadWriteLock` implementation when Mutex is per-fiber.

[ioquatix]

Ruby 3.0+ is per-fiber Mutex. Currently, it's possible to get errors when using ReentrantReadWriteLock on different fibers. Let's fix that.

Fixes https://github.com/ruby-concurrency/concurrent-ruby/issues/962

[ioquatix]

This PR also enables the tests under JRuby.

[eregon]

Use Thread.current.local_variable... for implementing thread locals.

Unfortunately I think we cannot just do that, and this is the reason that Concurrent::ThreadLocalVar exists (AFAIK):

• When the Concurrent::ThreadLocalVar is no longer reachable, the values held on each thread are released, and so the values corresponding to that ThreadLocalVar can be GC'd and are not leaked, unlike when using Thread#thread_variable_set (same semantics as Java ThreadLocal). https://github.com/ruby-concurrency/concurrent-ruby/blob/eac0237f54b18a9bdee3fb24ae25ee6b2136acfb/lib/concurrent-ruby/concurrent/atomic/ruby_thread_local_var.rb#L10 explains some of that.
• Those instances are anonymous, the key is the instance, and they cannot be accessed in any way but through the ThreadLocalVar instance, which means there isn't the problem of a thread local accessed by another Thread which Thread#thread_variable_get allows. That's easy to workaround though.

Maybe the first point can be addressed with some ObjectSpace.define_finalizer on the ThreadLocalVar. But we would also need a weak set of Thread instances or something. That could be an array/set of Threads + a finalizer per Thread, a bit similar to what RubyThreadLocalVar does. Maybe we could just use Thread.list for that, but 1) that doesn't exist for Fibers (that's the big problem), 2) this might iterate far more threads than threads which ever had a ThreadLocalVar value set (maybe no big deal).

Some pointers I could find about Concurrent::ThreadLocalVar:

• https://github.com/ruby-concurrency/concurrent-ruby/commit/8dd7e175d04b87dd7534054db1d229274c0a2f31 https://github.com/ruby-concurrency/concurrent-ruby/pull/22 : this actually used thread_variable_get
• https://github.com/ruby-concurrency/concurrent-ruby/pull/164 is about fixing the leaks
• https://github.com/ruby-concurrency/concurrent-ruby/pulls?q=is%3Apr+ThreadLocalVar

There should definitely be better docs on Concurrent::ThreadLocalVar to explain how it differs to Ruby's thread_variable_get. I'll try to add that at some point.

[eregon]

Something interesting is JavaThreadLocalVar is already per-Fiber, since it's per java.lang.Thread and JRuby uses java.lang.Thread (or java.lang.VirtualThread) to implement Fibers.

I think we should probably create a FiberLocalVar, based on JavaThreadLocalVar and RubyThreadLocalVar, and sharing the logic with RubyThreadLocalVar to avoid code duplication (ideally it should only be Fiber.current vs Thread.current changed).

[eregon]

The rest of the PR looks good from a quick look.

To give an example issue with the current PR: each ReentrantReadWriteLock instance leaks a number of bytes per thread (i.e. a Ruby core thread-local/fiber-local variable, so at least the key which is a Symbol, the value and the internal hash entry), even after the ReentrantReadWriteLock is GC'd. That leak only goes away when the Thread instance is GC'd, which might never happen (with thread pools, etc).

[ioquatix]

That makes sense. I don’t think there is any way in Ruby to delete a thread local or fiber local variable In general. Maybe we can nullify it using a finalizer.

[eregon]

That makes sense. I don’t think there is any way in Ruby to delete a thread local or fiber local variable In general. Maybe we can nullify it using a finalizer.

Indeed, there is no such thing as thread_variable_remove or for fiber locals in Ruby (currently at least). So we cannot even use one Ruby thread-local (i.e., thread_variable_get) per ThreadLocalVar instance, because that would leak the internal Hash entry on the Thread and the key Symbol (and per Thread which used that ThreadLocalVar), even if we would set the value to nil via a finalizer. That's probably the main reason why the current implementation uses a single Ruby thread-local for all ThreadLocalVar instances, to avoid that leak.

Aside: I wonder if this is partly why I didn't mind the :"foo_#{object_id}" workaround for Fiber storage variables, because anyway those also don't support removal and keep a strong reference to their key + value until the Fiber is gone, regardless of whether the key would become unreachable (at least currently, maybe something that could be improved in Ruby, but unlikely because incompatible and would likely have a big perf trade-off). Hence if being able to GC a thread/fiber-local is wanted, then one needs concurrent-ruby's ThreadLocalVar/FiberLocalVar.

[ioquatix]

One way to deal with this would be to assume assigning nil deletes the value - i.e. nil is indistinguishable from unset. This seems like a problem we should solve in Ruby and avoid hacking around here (even if we need a short term fix).

[ioquatix]

I've reverted back to the shared array implementation, and reworked it into class AbstractLocals, class ThreadLocals and class FiberLocals. We could perhaps make the name a little more specific, e.g. AbstractLocalsArray or something but I honestly don't have a strong opinion about it.

Generally, it seems okay. I'm a little concerned about the O(number of ECs) cost of deleting a local. If we are expecting people to have 100s or 100,000s of threads/fibers etc, this is a non-trivial cost.

A better solution might be to use a linked list for the @all_locals. Each EC would use a linked list node which is removed when it goes out of scope. With a little cunning, it might be possible to retain the similar synchronisation free approach.

[ioquatix]

@eregon I've done another pass over this, squashed all the commits, etc. Let me know if you want any further changes made.

[eregon]

@ioquatix Could you rebase? (feel free to squash your changes to make the rebase easier) Then I'll review.

[ioquatix]

Okay, rebase done.