Address dangling threads in Timeout module

[ProfessorEugene]

Some JVM-based projects (e.g. puppetlabs/puppetserver) that embed JRuby using ScriptingContainer and use the timeout module suffer thread leaks when recycling ScriptingContainers. Such users appear to expect ScriptingContainer#terminate to clean up all allocated resources (within reason) including threads.

This PR is an attempt to do so by shutting down the ScheduledThreadPoolExecutor created during the initialization of the Timeout module on Ruby teardown.

[headius]

This is a good find, and I agree it should be patched something like you did here. Unfortunately this is not quite the right logic.

The finalizer logic puts the Finalizer objects in a weak map, with the expectation being that the caller will hold a hard reference. Once the caller is no longer a live, the finalizer will no longer be alive, and the associated code will run.

In this case you do not hold a reference to the finalizer, so it is immediately dereferenced and likely to trigger shutdown of the Timeout executor prematurely.

Unfortunately it seems like we don't have an easy way to register a snippit of code to be run at exit from Java in the same way we have at_exit in Ruby.

I think this change would work, but I do wonder if we should add something to org.jruby.Ruby for easily adding Java-based exit hooks... thoughts @enebo @kares?

diff --git a/core/src/main/java/org/jruby/ext/timeout/Timeout.java b/core/src/main/java/org/jruby/ext/timeout/Timeout.java
index 35afa6517f..4712030203 100644
--- a/core/src/main/java/org/jruby/ext/timeout/Timeout.java
+++ b/core/src/main/java/org/jruby/ext/timeout/Timeout.java
@@ -45,6 +45,7 @@ import org.jruby.RubyThread;
 import org.jruby.RubyTime;
 import org.jruby.anno.JRubyMethod;
 import org.jruby.exceptions.RaiseException;
+import org.jruby.javasupport.JavaUtil;
 import org.jruby.runtime.Helpers;
 import org.jruby.runtime.Block;
 import org.jruby.runtime.ThreadContext;
@@ -68,6 +69,9 @@ public class Timeout {
                 new ScheduledThreadPoolExecutor(Runtime.getRuntime().availableProcessors(), new DaemonThreadFactory());
         executor.setRemoveOnCancelPolicy(true);
         timeout.setInternalVariable(EXECUTOR_VARIABLE, executor);
+
+        // for at_exit shutdown from the Ruby code
+        timeout.setInstanceVariable("@" + EXECUTOR_VARIABLE, JavaUtil.convertJavaToUsableRubyObject(timeout.getRuntime(), executor));
     }
 
 
diff --git a/lib/ruby/stdlib/timeout.rb b/lib/ruby/stdlib/timeout.rb
index 2c0a0e69d0..d4fed865b1 100644
--- a/lib/ruby/stdlib/timeout.rb
+++ b/lib/ruby/stdlib/timeout.rb
@@ -56,6 +56,12 @@ end
 # Load executor-based timeout logic into Timeout mod
 JRuby::Util.load_ext('org.jruby.ext.timeout.Timeout')
 
+# Register an at_exit hook to shut down the Timeout executor
+at_exit do
+  executor = Timeout.instance_variable_get(:@__executor__)
+  executor.shutdown
+end
+
 def timeout(*args, &block)
   warn "Object##{__method__} is deprecated, use Timeout.timeout instead.", uplevel: 1
   Timeout.timeout(*args, &block)

[ProfessorEugene]

@headius - oops, missed that finalizers were a WeakHashMap! The original change behaved exactly as you expected in the CI tests (premature shutdown). Moved the shutdown call directly to timeout.rb via fb6a10c as you suggested

[headius]

That change looks good to me. 👍

[kares]

👍 on being able to achieve the same (register an at_exit callable) from Java

[headius]

@ProfessorEugene @kares See #6133 for an improved API.