Skip to content

Commit

Permalink
Fix a race between PO cleanup thread and Python
Browse files Browse the repository at this point in the history
  • Loading branch information
jmao-denver committed Oct 18, 2024
1 parent a1fb456 commit e93774a
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 34 deletions.
2 changes: 1 addition & 1 deletion src/main/java/org/jpy/PyObject.java
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
70 changes: 38 additions & 32 deletions src/main/java/org/jpy/PyObjectReferences.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,5 +50,4 @@ public static void script(String expression, int numThreads) {
}
}
}

}

0 comments on commit e93774a

Please sign in to comment.