Skip to content

fixes #1616 tighten up synchronization and conditional logic #1688

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+57 −53
Open

fixes #1616 tighten up synchronization and conditional logic #1688

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9002b63
fixes #1616 tighten up synchronization and conditional logic
brettwooldridge Sep 26, 2025
5fac28f
ensure that object is referenceable past the enqueue point
brettwooldridge Oct 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 57 additions & 53 deletions src/com/sun/jna/internal/Cleaner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import java.lang.ref.PhantomReference;
import java.lang.ref.Reference;
import java.lang.ref.ReferenceQueue;
import java.util.concurrent.TimeUnit;
import java.util.logging.Level;
import java.util.logging.Logger;

Expand All @@ -35,84 +36,87 @@
* objects. It replaces the {@code Object#finalize} based resource deallocation
* that is deprecated for removal from the JDK.
*
* <p><strong>This class is intented to be used only be JNA itself.</strong></p>
* <p><strong>This class is intended to be used only be JNA itself.</strong></p>
*/
public class Cleaner {
private static final Cleaner INSTANCE = new Cleaner();
private static final Logger logger = Logger.getLogger(Cleaner.class.getName());
private static final long CLEANER_LINGER_TIME = TimeUnit.SECONDS.toMillis(30);

public static Cleaner getCleaner() {
return INSTANCE;
}

private final ReferenceQueue<Object> referenceQueue;
private Thread cleanerThread;
private boolean cleanerRunning;
private CleanerRef firstCleanable;

private Cleaner() {
referenceQueue = new ReferenceQueue<>();
}

public synchronized Cleanable register(Object obj, Runnable cleanupTask) {
// The important side effect is the PhantomReference, that is yielded
// after the referent is GCed
return add(new CleanerRef(this, obj, referenceQueue, cleanupTask));
}
public Cleanable register(final Object obj, final Runnable cleanupTask) {
// The important side effect is the PhantomReference, that is yielded after the referent is GCed
final CleanerRef ref = new CleanerRef(obj, referenceQueue, cleanupTask);

private synchronized CleanerRef add(CleanerRef ref) {
synchronized (referenceQueue) {
if (firstCleanable == null) {
firstCleanable = ref;
} else {
synchronized (this) {
if (firstCleanable != null) {
ref.setNext(firstCleanable);
firstCleanable.setPrevious(ref);
firstCleanable = ref;
}
if (cleanerThread == null) {
Logger.getLogger(Cleaner.class.getName()).log(Level.FINE, "Starting CleanerThread");
cleanerThread = new CleanerThread();
firstCleanable = ref;

if (!cleanerRunning) {
logger.log(Level.FINE, "Starting CleanerThread");
Thread cleanerThread = new CleanerThread();
cleanerThread.start();
cleanerRunning = true;
}
return ref;
}

// Ensure that obj is referencable past the enqueue point.
if (obj == null) {
throw new IllegalArgumentException("Cleaner object cannot be null");
}

return ref;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point we need the equivalent of Reference.reachabilityFence on obj. If the caller does not retain a strong reference, we need to ensure, that the reference is kept at least until the reference cleaner is completely enqueued. As observed in the last iteration early GC can happen: #1684 (comment).

In the comment I suggested to use an empty sychronized block.

Copy link
Contributor Author

@brettwooldridge brettwooldridge Oct 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I don't know how I lost the change I had made, but I did somehow. The way that I intended to solve it, and had at some point but must have reverted it, was to insert this before the return ref:

// Ensure that obj is referencable past the enqueue point.
if (obj == null) {
   throw new IllegalArgumentException("Cleaner object cannot be null");
}

I believe that should address it. Can you test it?

If that does not fix it, remove synchronized from the method signature, and do this:

public Cleanable register(final Object obj, final Runnable cleanupTask) {
   // The important side effect is the PhantomReference, that is yielded after the referent is GCed
   final CleanerRef ref = new CleanerRef(obj, referenceQueue, cleanupTask);

   synchronized (this) {
        // everything else
        ...
   }

   // Ensure that obj is referencable past the enqueue point.
   if (obj == null) {
      throw new IllegalArgumentException("Cleaner object cannot be null");
   }
   return ref;
}

EDIT: I updated the pull request with this later fix.

Copy link
Contributor Author

@brettwooldridge brettwooldridge Oct 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this change should work. The compiler cannot reorder the conditional before the synchronized block, and it cannot eliminate the reference bc it cannot know whether it is null or not.

By the way, really nice work on this class. Outside of this change, I simply don't see any way it could possibly be more efficient. I love seeing code like this.

}

private synchronized boolean remove(CleanerRef ref) {
synchronized (referenceQueue) {
boolean inChain = false;
if (ref == firstCleanable) {
firstCleanable = ref.getNext();
inChain = true;
}
if (ref.getPrevious() != null) {
ref.getPrevious().setNext(ref.getNext());
}
if (ref.getNext() != null) {
ref.getNext().setPrevious(ref.getPrevious());
}
if (ref.getPrevious() != null || ref.getNext() != null) {
inChain = true;
}
ref.setNext(null);
ref.setPrevious(null);
return inChain;
private synchronized boolean remove(final CleanerRef ref) {
final CleanerRef prev = ref.getPrevious();
final CleanerRef next = ref.getNext();
boolean inChain = false;

if (ref == firstCleanable) {
firstCleanable = next;
inChain = true;
}
if (prev != null) {
prev.setNext(next);
inChain = true;
}
if (next != null) {
next.setPrevious(prev);
inChain = true;
}
return inChain;
}

private static class CleanerRef extends PhantomReference<Object> implements Cleanable {
private final Cleaner cleaner;
private final Runnable cleanupTask;
private CleanerRef previous;
private CleanerRef next;

public CleanerRef(Cleaner cleaner, Object referent, ReferenceQueue<? super Object> q, Runnable cleanupTask) {
super(referent, q);
this.cleaner = cleaner;
public CleanerRef(final Object referent, final ReferenceQueue<? super Object> queue, final Runnable cleanupTask) {
super(referent, queue);
this.cleanupTask = cleanupTask;
}

@Override
public void clean() {
if(cleaner.remove(this)) {
if (INSTANCE.remove(this)) {
previous = null;
next = null;
cleanupTask.run();
}
}
Expand All @@ -121,27 +125,25 @@ CleanerRef getPrevious() {
return previous;
}

void setPrevious(CleanerRef previous) {
void setPrevious(final CleanerRef previous) {
this.previous = previous;
}

CleanerRef getNext() {
return next;
}

void setNext(CleanerRef next) {
void setNext(final CleanerRef next) {
this.next = next;
}
}

public static interface Cleanable {
public void clean();
public interface Cleanable {
void clean();
}

private class CleanerThread extends Thread {

private static final long CLEANER_LINGER_TIME = 30000;

public CleanerThread() {
super("JNA Cleaner");
setDaemon(true);
Expand All @@ -151,20 +153,19 @@ public CleanerThread() {
public void run() {
while (true) {
try {
Reference<? extends Object> ref = referenceQueue.remove(CLEANER_LINGER_TIME);
Reference<?> ref = referenceQueue.remove(CLEANER_LINGER_TIME);
if (ref instanceof CleanerRef) {
((CleanerRef) ref).clean();
} else if (ref == null) {
synchronized (referenceQueue) {
Logger logger = Logger.getLogger(Cleaner.class.getName());
synchronized (INSTANCE) {
if (firstCleanable == null) {
cleanerThread = null;
logger.log(Level.FINE, "Shutting down CleanerThread");
cleanerRunning = false;
break;
} else if (logger.isLoggable(Level.FINER)) {
StringBuilder registeredCleaners = new StringBuilder();
for(CleanerRef cleanerRef = firstCleanable; cleanerRef != null; cleanerRef = cleanerRef.next) {
if(registeredCleaners.length() != 0) {
for (CleanerRef cleanerRef = firstCleanable; cleanerRef != null; cleanerRef = cleanerRef.next) {
if (registeredCleaners.length() != 0) {
registeredCleaners.append(", ");
}
registeredCleaners.append(cleanerRef.cleanupTask.toString());
Expand All @@ -178,6 +179,9 @@ public void run() {
// our reference queue, well, there is no way to separate
// the two cases.
// https://groups.google.com/g/jna-users/c/j0fw96PlOpM/m/vbwNIb2pBQAJ
synchronized (INSTANCE) {
cleanerRunning = false;
}
break;
} catch (Exception ex) {
Logger.getLogger(Cleaner.class.getName()).log(Level.SEVERE, null, ex);
Expand Down
Loading