Skip library unloading during VM finalization - #1194

Thank you for your investigation and the update! Wouldn't it make sense to enable dlclose on Macos now?

Wouldn't it make sense to enable dlclose on Macos now?

@larskanis Yes, and I pulled that out into its own PR in #1199. Since it's been disabled for a long time I'd rather an explicit PR than bundling it here, but happy to merge them if you prefer!

At process exit Ruby's shutdown sweep frees everything, even objects that are still referenced. If a native library owns background threads, those threads can wake up in code that isn't mapped anymore.

This seems like a bug in either the native code or in CRuby. Shouldn't these threads be shut down before the GC starts clearing referenced objects?

This seems like a bug in either the native code or in CRuby. Shouldn't these threads be shut down before the GC starts clearing referenced objects?

Ideally, yes. I checked both CRuby 3.4.8 and 4.0.6: they stop Ruby Threads, run ObjectSpace finalizers, then free T_DATA objects (including still-referenced DynamicLibrary handles).

These aren't Ruby threads, though. They're pthreads created inside librdkafka, so CRuby can't see or stop them. rd_kafka_destroy does that, and rdkafka-ruby normally calls it from a finalizer. The gap is that rd_kafka_new can succeed before the Ruby wrapper and its finalizer are installed. If later setup raises, the native client is orphaned and Ruby has nothing left to close.

So I agree the binding should clean up first, and we're fixing that path too. NODELETE is the safety net when cleanup never gets a chance to run; it isn't a replacement for cleanup.

The gap is that rd_kafka_new can succeed before the Ruby wrapper and its finalizer are installed. If later setup raises, the native client is orphaned and Ruby has nothing left to close.

This sounds like an unlikely edge case/a bug in rdkafka-ruby, if it can happen.

I'm negative on this, it's basically leaking memory to workaround missing necessary cleanup.
In your example, it doesn't seem any good to still have thirty threads per client still running, they should be properly shut down.

BTW I wonder if dlclose() does anything when passed RTLD_NODELETE.
Also the man page says:

       RTLD_NODELETE (since glibc 2.2)
              Do not unload the shared object during dlclose().  Consequently, the object's static and global variables are not  reinitialized  if  the
              object is reloaded with dlopen() at a later time.

Which is unsafe if there is another dlopen() for that same library later, and will likely result in a harder-to-diagnose crash.

If changing anything, I would simply not call dlclose().
Using RTLD_NODELETE seems pretty hacky and seems a workaround for something more fundamental going wrong.

I'm coming around to NODELETE being too broad here. I'm working on a fix in rdkafka (karafka/rdkafka-ruby#964).

What if this PR only skipped dlclose once CRuby starts shutting down? Normal GC and explicit unloads would keep working exactly as they do today. There isn't much value in unloading a library while the process is already on its way out, and it avoids changing the default lifetime of every ffi library.

That would also keep #1199 straightforward: enable dlclose on macOS during normal execution, but not from the shutdown sweep. Does that direction sound better?

I'm coming around to NODELETE being too broad here. I'm working on a fix in rdkafka (karafka/rdkafka-ruby#964).

Nice 👍

What if this PR only skipped dlclose once CRuby starts shutting down?

Is that necessary, for what?

Normal GC and explicit unloads would keep working exactly as they do today. There isn't much value in unloading a library while the process is already on its way out, and it avoids changing the default lifetime of every ffi library.

One concern is there can be something very long in at_exit, for example a test suite or webserver running in it.
IOW, at_exit isn't the same as shutting down.

One concern is there can be something very long in at_exit, for example a test suite or webserver running in it.
IOW, at_exit isn't the same as shutting down.

Good point, rb_set_end_proc is too early. I reworked this around a private rooted object with a finalizer. CRuby runs that after all at_exit/END handlers and before forced T_DATA cleanup, so normal GC and at_exit still unload libraries; only the forced sweep skips dlclose.

I still think the ffi part is useful as a safety net. The binding should stop its own threads (karafka/rdkafka-ruby#964 does that for rdkafka) but if cleanup is incomplete, the forced sweep currently turns that into a SIGSEGV by unmapping code under the remaining threads.

I added three child-process checks: ordinary GC still unloads, GC from an at_exit handler registered before ffi still unloads, and forced shutdown no longer crashes with active native threads. The END-proc version fails the at_exit case. In the real rdkafka repro, stock ffi crashed in 13/20 children and this version completed 20/20 cleanly.

Does that work?