Leaking fiber monitors when running in custom IORuntime using JDK 21 virtual threads about cats-effect HOT 11 CLOSED

Right, but the ConcurrentLinkedQueue will keep growing more and more nodes, pointing to GCed bags.

from cats-effect.

It seems the leak is due to the nodes of the BagReferences ConcurrentLinkedQueue in FiberMonitor, and the (empty) weakref objects in them. As far as I can see, these are never cleaned up, so the same would happen with normal threads too (except those are probably not created in an unbounded number).

from cats-effect.

Ideally we'd be using the carrier thread's local storage, not the virtual thread's for the bag. But that's probably not possible.

Well... https://github.com/openjdk/jdk21u/blob/060c4f7589e7f13febd402f4dac3320f4c032b08/src/java.base/share/classes/jdk/internal/misc/CarrierThreadLocal.java#L35

(I'm not seriously suggesting we use that, just found it interesting.)

from cats-effect.

Thanks, makes sense that we are not the only ones needing that 😇

I'm not seriously suggesting we use that

even if we wanted to these new JVMs make it annoying/impossible to access their internals

from cats-effect.

It seems the leak is due to the nodes of the BagReferences ConcurrentLinkedQueue in FiberMonitor, and the (empty) weakref objects in them.

@djspiewak as Daniel U pointed out, the leak is actually our fault:

We are lacking a "packing" mechanism for BagReferences.

from cats-effect.

Hmm, that's actually really annoying. I think we'll want to rethink that thread-local strategy for virtual threads since there's a 1-to-1 fiber-to-virtual-thread mapping. Ideally we'd be using the carrier thread's local storage, not the virtual thread's for the bag. But that's probably not possible.

so the same would happen with normal threads too (except those are probably not created in an unbounded number).

But we should probably still fix this anyway.

from cats-effect.

You know, I'm actually very surprised that thread locals aren't just cleaned up with the thread itself. Arman is right that we really want the carrier thread here, not the virtual thread, but even with the virtual thread this doesn't feel like a thing that should be leaking (just a thing that would be very inefficient).

from cats-effect.

Right but that's a weak reference, so it shouldn't prevent the cleanup of bag.

from cats-effect.

The solution is probably to do something annoying with ReferenceQueue and periodically check that queue for size. Once the queue gets big enough, we go through and purge out the collected refs in Bags. This would have to be amortized into monitorSuspended or similar, creating a moderate overhead. Probably not enough overhead to worry about gating it to virtual threads, so this would also work to resolve the similar issue with blocking.

We wouldn't need JDK 21 for this (since it's not loom specific), so we could probably resolve it in 3.5.x

from cats-effect.

I've proposed a fix for the general issue in #3964.

But from a performance standpoint I still think that we need a different solution for virtual threads. The current mechanism adds a lot of overhead:

1. a single point of contention for registering every thread
2. allocating an entire WeakBag (intended to be long-lived and hold references to many fibers over its lifetime) for a virtual thread, even though we will only place a single fiber in there
3. nested layers of weak references, which we know that the GC just loves /s
   Specifically we hold a weak reference to a bag which itself hold a weak reference to the fiber.

from cats-effect.

Fixed the leak in #3964 and opened a follow-up.

• #4014

from cats-effect.