From e93774a09fe9b20dce0a4e079064476fd0f60081 Mon Sep 17 00:00:00 2001
From: jianfengmao <jianfengmao@deephaven.io>
Date: Thu, 26 Sep 2024 18:18:20 -0600
Subject: [PATCH] Fix a race between PO cleanup thread and Python

---
 src/main/java/org/jpy/PyObject.java           |  2 +-
 src/main/java/org/jpy/PyObjectReferences.java | 70 ++++++++++---------
 .../MultiThreadedEvalTestFixture.java         |  1 -
 3 files changed, 39 insertions(+), 34 deletions(-)

diff --git a/src/main/java/org/jpy/PyObject.java b/src/main/java/org/jpy/PyObject.java
index e0715ec7..746039d8 100644
--- a/src/main/java/org/jpy/PyObject.java
+++ b/src/main/java/org/jpy/PyObject.java
@@ -71,7 +71,7 @@ public static int cleanup() {
     PyObject(long pointer, boolean fromJNI) {
         state = new PyObjectState(pointer);
         if (fromJNI) {
-            if (CLEANUP_ON_INIT && PyLib.hasGil()) {
+            if (CLEANUP_ON_INIT) {
                 REFERENCES.cleanupOnlyUseFromGIL(); // only performs *one* cleanup
             }
             if (CLEANUP_ON_THREAD) {
diff --git a/src/main/java/org/jpy/PyObjectReferences.java b/src/main/java/org/jpy/PyObjectReferences.java
index 1196cf14..6d266042 100644
--- a/src/main/java/org/jpy/PyObjectReferences.java
+++ b/src/main/java/org/jpy/PyObjectReferences.java
@@ -153,45 +153,51 @@ def cleanup(references):
                 sleep_time = 0.1 if size == 1024 else 1.0
                 time.sleep(sleep_time)
          */
-
-        final PyObjectCleanup proxy = asProxy();
-
-        while (!Thread.currentThread().isInterrupted()) {
-            // This blocks on the GIL, acquires the GIL, and then releases the GIL.
-            // For linux, acquiring the GIL involves a pthread_mutex_lock, which does not provide
-            // any fairness guarantees. As such, we need to be mindful of other python users/code,
-            // and ensure we don't overly acquire the GIL causing starvation issues, especially when
-            // there is no cleanup work to do.
-            final int size = proxy.cleanupOnlyUseFromGIL();
-
-
-            // Although, it *does* make sense to potentially take the GIL in a tight loop when there
-            // is a lot of real cleanup work to do. Sleeping for any amount of time may be
-            // detrimental to the cleanup of resources. There is a balance that we want to try to
-            // achieve between producers of PyObjects, and the cleanup of PyObjects (us).
-
-            // It would be much nicer if ReferenceQueue exposed a method that blocked until the
-            // queue was non-empty and *doesn't* remove any items. We can potentially implement this
-            // by using reflection to access the internal lock of the ReferenceQueue in the future.
-
-            if (size == buffer.length) {
-                if (CLEANUP_THREAD_ACTIVE_SLEEP_MILLIS == 0) {
-                    Thread.yield();
+        try {
+            final PyObjectCleanup proxy = asProxy();
+
+            while (!Thread.currentThread().isInterrupted()) {
+                // This blocks on the GIL, acquires the GIL, and then releases the GIL.
+                // For linux, acquiring the GIL involves a pthread_mutex_lock, which does not provide
+                // any fairness guarantees. As such, we need to be mindful of other python users/code,
+                // and ensure we don't overly acquire the GIL causing starvation issues, especially when
+                // there is no cleanup work to do.
+                final int size = proxy.cleanupOnlyUseFromGIL();
+
+
+                // Although, it *does* make sense to potentially take the GIL in a tight loop when there
+                // is a lot of real cleanup work to do. Sleeping for any amount of time may be
+                // detrimental to the cleanup of resources. There is a balance that we want to try to
+                // achieve between producers of PyObjects, and the cleanup of PyObjects (us).
+
+                // It would be much nicer if ReferenceQueue exposed a method that blocked until the
+                // queue was non-empty and *doesn't* remove any items. We can potentially implement this
+                // by using reflection to access the internal lock of the ReferenceQueue in the future.
+
+                if (size == buffer.length) {
+                    if (CLEANUP_THREAD_ACTIVE_SLEEP_MILLIS == 0) {
+                        Thread.yield();
+                    } else {
+                        try {
+                            Thread.sleep(CLEANUP_THREAD_ACTIVE_SLEEP_MILLIS);
+                        } catch (InterruptedException e) {
+                            Thread.currentThread().interrupt();
+                            return;
+                        }
+                    }
                 } else {
                     try {
-                        Thread.sleep(CLEANUP_THREAD_ACTIVE_SLEEP_MILLIS);
+                        Thread.sleep(CLEANUP_THREAD_PASSIVE_SLEEP_MILLIS);
                     } catch (InterruptedException e) {
                         Thread.currentThread().interrupt();
                         return;
                     }
                 }
-            } else {
-                try {
-                    Thread.sleep(CLEANUP_THREAD_PASSIVE_SLEEP_MILLIS);
-                } catch (InterruptedException e) {
-                    Thread.currentThread().interrupt();
-                    return;
-                }
+            }
+        }
+        catch (RuntimeException e) {
+            if (!e.getMessage().contains("PyLib not initialized")) {
+                throw e;
             }
         }
     }
diff --git a/src/test/java/org/jpy/fixtures/MultiThreadedEvalTestFixture.java b/src/test/java/org/jpy/fixtures/MultiThreadedEvalTestFixture.java
index f7ca69f3..1a7ed56b 100644
--- a/src/test/java/org/jpy/fixtures/MultiThreadedEvalTestFixture.java
+++ b/src/test/java/org/jpy/fixtures/MultiThreadedEvalTestFixture.java
@@ -50,5 +50,4 @@ public static void script(String expression, int numThreads) {
             }
         }
     }
-
 }