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

ioquatix · 2023-01-11T02:29:34Z

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 #962

ioquatix · 2023-01-11T05:52:09Z

This PR also enables the tests under JRuby.

eregon · 2023-01-11T17:25:26Z

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).

concurrent-ruby/lib/concurrent-ruby/concurrent/atomic/ruby_thread_local_var.rb

Line 10 in eac0237

# Each thread has a (lazily initialized) array of thread-local variable values

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:

8dd7e17 ThreadLocalVar #22 : this actually used thread_variable_get
Fix Thread leaking in ThreadLocalVariables #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 · 2023-01-11T17:39:06Z

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 · 2023-01-11T17:44:55Z

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 · 2023-01-11T20:30:54Z

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 · 2023-01-11T21:25:36Z

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 · 2023-01-11T22:03:05Z

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 · 2023-01-12T02:24:04Z

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.

lib/concurrent-ruby/concurrent/atomic/thread_local_var.rb

lib/concurrent-ruby/concurrent/atomic/lock_local_var.rb

lib/concurrent-ruby/concurrent/atomic/locals.rb

ioquatix · 2023-01-13T09:05:07Z

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

lib/concurrent-ruby/concurrent/atomic/lock_local_var.rb

eregon · 2023-01-21T13:35:20Z

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

…cope. # Conflicts: # lib/concurrent-ruby/concurrent/atomic/reentrant_read_write_lock.rb # spec/concurrent/atomic/reentrant_read_write_lock_spec.rb

ioquatix · 2023-01-21T20:27:41Z

Okay, rebase done.

lib/concurrent-ruby/concurrent/atomic/java_thread_local_var.rb

lib/concurrent-ruby/concurrent/atomic/locals.rb

eregon

I don't see any specs for FiberLocalVar, this is necessary as we introduce a new public class.

lib/concurrent-ruby/concurrent/atomic/thread_local_var.rb

eregon

I added specs for FiberLocalVar.
Should be good to go now, just waiting CI.
Thank you for the PR!

ioquatix · 2023-01-23T20:38:46Z

lib/concurrent-ruby/concurrent/atomic/locals.rb

    end

    def locals!
      thread = Thread.current
-      locals = thread.thread_variable_get(@name)
+      locals = thread.thread_variable_get(:concurrent_thread_locals)


This change means that multiple instances of the class will clobber each other.

Yes but since we only need 1 instance we support just that and not more

Okay, fair enough.

ioquatix force-pushed the native-thread-locals branch 8 times, most recently from 7e6667c to 21cea96 Compare January 11, 2023 05:41

ioquatix changed the title ~~Use Thread.current.local_variable... for implementing thread locals.~~ Fix ReentrantReadWriteLock implementation when Mutex is per-fiber. Jan 11, 2023

ioquatix force-pushed the native-thread-locals branch 4 times, most recently from 3efea70 to 3b50565 Compare January 12, 2023 02:13

eregon reviewed Jan 12, 2023

View reviewed changes

ioquatix requested a review from eregon January 13, 2023 08:49

ioquatix force-pushed the native-thread-locals branch 2 times, most recently from 7d83f9f to b001be7 Compare January 13, 2023 08:56

eregon reviewed Jan 13, 2023

View reviewed changes

lib/concurrent-ruby/concurrent/atomic/lock_local_var.rb Outdated Show resolved Hide resolved

ioquatix added 2 commits January 22, 2023 09:24

Introduce FiberLocalVar and LockLocalVar for alignment with Mutex s…

5b5b53d

…cope. # Conflicts: # lib/concurrent-ruby/concurrent/atomic/reentrant_read_write_lock.rb # spec/concurrent/atomic/reentrant_read_write_lock_spec.rb

Revert back to shared implementation of "locals" storage array.

6827b55

Mark the mutex_owned_per_thread? method as private.

b1f4fee

ioquatix force-pushed the native-thread-locals branch from 8586727 to b1f4fee Compare January 21, 2023 20:24

Add missing require "concurrent/set.rb".

50b538c

Fix missing method on_cruby?.

4959d76

ioquatix force-pushed the native-thread-locals branch from 6f76667 to 4959d76 Compare January 21, 2023 21:42

eregon reviewed Jan 23, 2023

View reviewed changes

lib/concurrent-ruby/concurrent/atomic/java_thread_local_var.rb Show resolved Hide resolved

eregon reviewed Jan 23, 2023

View reviewed changes

lib/concurrent-ruby/concurrent/atomic/locals.rb Show resolved Hide resolved

eregon reviewed Jan 23, 2023

View reviewed changes

lib/concurrent-ruby/concurrent/atomic/thread_local_var.rb Show resolved Hide resolved

eregon force-pushed the native-thread-locals branch from 87ad3a8 to 1db8e40 Compare January 23, 2023 13:21

Various fixes for ThreadLocalVar and FiberLocalVar

9c09274

eregon force-pushed the native-thread-locals branch from 1db8e40 to 9c09274 Compare January 23, 2023 14:02

eregon approved these changes Jan 23, 2023

View reviewed changes

eregon merged commit 6dcc9b8 into ruby-concurrency:master Jan 23, 2023

ioquatix commented Jan 23, 2023

View reviewed changes

ioquatix deleted the native-thread-locals branch January 23, 2023 20:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ioquatix commented Jan 11, 2023 •

edited

Loading

ioquatix commented Jan 11, 2023

eregon commented Jan 11, 2023 •

edited

Loading

eregon commented Jan 11, 2023

eregon commented Jan 11, 2023

ioquatix commented Jan 11, 2023

eregon commented Jan 11, 2023

ioquatix commented Jan 11, 2023 •

edited

Loading

ioquatix commented Jan 12, 2023 •

edited

Loading

ioquatix commented Jan 13, 2023

eregon commented Jan 21, 2023

ioquatix commented Jan 21, 2023

eregon left a comment

eregon left a comment

ioquatix Jan 23, 2023

eregon Jan 23, 2023

ioquatix Jan 23, 2023

Fix ReentrantReadWriteLock implementation when Mutex is per-fiber. #983

Fix ReentrantReadWriteLock implementation when Mutex is per-fiber. #983

Conversation

ioquatix commented Jan 11, 2023 • edited Loading

ioquatix commented Jan 11, 2023

eregon commented Jan 11, 2023 • edited Loading

eregon commented Jan 11, 2023

eregon commented Jan 11, 2023

ioquatix commented Jan 11, 2023

eregon commented Jan 11, 2023

ioquatix commented Jan 11, 2023 • edited Loading

ioquatix commented Jan 12, 2023 • edited Loading

ioquatix commented Jan 13, 2023

eregon commented Jan 21, 2023

ioquatix commented Jan 21, 2023

eregon left a comment

Choose a reason for hiding this comment

eregon left a comment

Choose a reason for hiding this comment

ioquatix Jan 23, 2023

Choose a reason for hiding this comment

eregon Jan 23, 2023

Choose a reason for hiding this comment

ioquatix Jan 23, 2023

Choose a reason for hiding this comment

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

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

ioquatix commented Jan 11, 2023 •

edited

Loading

eregon commented Jan 11, 2023 •

edited

Loading

ioquatix commented Jan 11, 2023 •

edited

Loading

ioquatix commented Jan 12, 2023 •

edited

Loading