From: "Eregon (Benoit Daloze) via ruby-core" <ruby-core@...>
Date: 2023-01-16T11:53:41+00:00
Subject: [ruby-core:111831] [Ruby master Feature#19333] Setting (Fiber Local|Thread Local|Fiber Storage) to nil should delete value in order to avoid memory leaks.

Issue #19333 has been updated by Eregon (Benoit Daloze).


The current solution/workaround to both of these memory leaks is to use concurrent-ruby's [ThreadLocalVar](https://ruby-concurrency.github.io/concurrent-ruby/master/Concurrent/ThreadLocalVar.html), which is the reason that exists.

The idea to remove on assigning `nil` looks elegant at first sight but has multiple problems:
* The user might use `nil` as a "valid value" and with this change it will be more expensive to remove the fiber local than to just assign, also more expensive if later that fiber local is set again to non-nil (delete+insert vs set+set).
* This is problematic when implementing fiber locals with Shapes, which TruffleRuby currently does, because it causes a lot more Shape polymorphism if fiber locals can be removed, so makes fiber locals slower.
* It doesn't solve the leak itself, it would need a finalizer per fiber local, which is pretty heavy for this feature AND we can't put a finlizer on the Symbol since the Symbol is never collected. So this seems unusable for fiber/thread locals, it would need an extra wrapping object for every usage, or explicitly clearing the fiber local in a begin/ensure/end pattern.

For those reasons I'm rather against this.
I think it's a rare enough problem that it's OK to rely on concurrent-ruby for this feature.
Many fiber/thread local usages are fine because they don't use a random part in the Symbol and so it's a small number of them.
It is a problem when there are many fiber/thread locals, IMO those should concurrent-ruby's ThreadLocalVar/FiberLocalVar.

> That being said, the other option is just not to encourage people to have long running threads that accumulate cruft over time.

That's a hard sell, at least the not having long running threads part. On all current Rubies the time to create a thread is significant and so reusing threads e.g. via a thread pool is a necessity for performance (e.g., in threaded web servers like Puma). (maybe CRuby can do M-N threading in the future, but this is not possible on JVM AFAIK because e.g. ReentrantLock relies on java.lang.Thread identity and not RubyThread/RubyFiber identity)

----------------------------------------
Feature #19333: Setting (Fiber Local|Thread Local|Fiber Storage) to nil should delete value in order to avoid memory leaks.
https://bugs.ruby-lang.org/issues/19333#change-101240

* Author: ioquatix (Samuel Williams)
* Status: Open
* Priority: Normal
----------------------------------------
As it stands, Fiber Locals, Thread Locals and Fiber Storage have no way of deleting key-value associations.

```ruby
100.times do |i|
  name = :"variable-#{i}"
  Thread.current[name] = 10
end
```

Because of this, dynamically generated associations can leak over time. This is worse for things like Threads that might be pooled (or maybe an argument against user-space pooling).

In any case, having a way to delete those associations would allow application code to at least delete the associations when they no longer make sense.

I propose that assigning `nil` to "locals" or "storage" should effectively delete them.

e.g.

```ruby
100.times do |i|
  name = :"variable-#{i}"
  Thread.current[name] = 10
  Thread.current[name] = nil # delete association
end
```

A more invasive alternative would be to define new interfaces like `Thread::Local`, `Fiber::Local` and `Fiber::Storage::Local` (or something) which correctly clean up on GC.



-- 
https://bugs.ruby-lang.org/
 ______________________________________________
 ruby-core mailing list -- ruby-core@ml.ruby-lang.org
 To unsubscribe send an email to ruby-core-leave@ml.ruby-lang.org
 ruby-core info -- https://ml.ruby-lang.org/mailman3/postorius/lists/ruby-core.ml.ruby-lang.org/