Remove some synchronization in Reference to avoid deadlock #20694

pshipton · 2024-11-27T19:59:58Z

Issue #13823

pshipton · 2024-11-27T20:00:44Z

jenkins test sanity plinux jdk8

jcl/src/java.base/share/classes/java/lang/ref/Reference.java

pshipton · 2024-11-28T02:43:21Z

The successful PR build https://openj9-jenkins.osuosl.org/job/PullRequest-OpenJ9/6578/

jcl/src/java.base/share/classes/java/lang/ref/Reference.java

babsingh · 2024-11-28T17:02:59Z

The deadlock happens because the two locks are acquired in different order in the two code paths.

20:55:13  "Finalizer thread" Id=29 BLOCKED on java.lang.ref.ReferenceQueue@49d657dd owned by "Thread-11" Id=33
20:55:13  	at java.lang.ref.ReferenceQueue.enqueue(ReferenceQueue.java:179) // Acquire ReferenceQueue's monitor
20:55:13  	at java.lang.ref.Reference.enqueueImpl(Reference.java:147) // Acquire Reference's monitor
20:55:13  


20:55:13  "Thread-11" Id=33 BLOCKED on java.util.logging.LogManager$LoggerWeakRef@596d6465 owned by "Finalizer thread" Id=29
20:55:13  	at java.lang.ref.Reference.dequeue(Reference.java:195) // Acquire Reference's monitor
20:55:13  	at java.lang.ref.ReferenceQueue.poll(ReferenceQueue.java:85) // Acquire ReferenceQueue's monitor
20:55:13  	at java.util.logging.LogManager.drainLoggerRefQueueBounded(LogManager.java:1130)
20:55:13  	at java.util.logging.LogManager.addLogger(LogManager.java:1160)
20:55:13  	at java.util.logging.LogManager.demandLogger(LogManager.java:556)
20:55:13  	at java.util.logging.Logger.demandLogger(Logger.java:455)
20:55:13  	at java.util.logging.Logger.getLogger(Logger.java:502)
20:55:13  	at TestLogConfigurationDeadLock$AddLogger.run(TestLogConfigurationDeadLock.java:175)

Removing the synchronized block in dequeue might change Reference.state while enqueue is being executed. This can lead to incorrect behavior.

babsingh · 2024-11-28T17:13:19Z

The correct fix is to avoid the different order in which the two locks are acquired. Example: In ReferenceQueue, we can avoid invoking Reference.dequeue() from within the synchronized(this) block.

public Reference<? extends T> poll () {
        Reference ref;

        /* Optimization to return immediately and not synchronize if there is nothing in the queue */
        if(empty) {
                return null;
        }
        synchronized(this) {
                if(empty) {
                        return null;
                }
                ref = references[head];
                references[head++] = null;
                if(head == references.length) {
                        head = 0;
                }
                if(head == tail) {
                        empty = true;
                }
        }
        ref.dequeue(); // Moved outside the synchronized block
        return ref;
}

public Reference<? extends T> remove(long timeout) throws IllegalArgumentException, InterruptedException {
        if (timeout < 0) throw new IllegalArgumentException();

        Reference ref;
        synchronized(this) {
                if(empty) {
                        wait(timeout);
                        if(empty) return null;
                }
                ref = references[head];
                references[head++] = null;
                if(head == references.length) {
                        head = 0;
                }
        }
        ref.dequeue(); // Moved outside the synchronized block
        synchronized(this) { // Not sure if it matters to notifyAll() after invoking dequeue()
                if(head == tail) {
                        empty = true;
                } else {
                        notifyAll();
                }
        }
        return ref;
}

@dmitripivkine Does it matter if we move the dequeue operation at the end? Also, is there a requirement to notifyAll after dequeue or can we notifyAll first and then invoke dequeue?

keithc-ca · 2024-11-28T17:47:08Z

correct fix is to avoid the different order in which the two locks are acquired

Because this is a public class, we don't have complete control over the synchronization order.

Removing synchronization in isEnqueued() is safe because there are no values of state that might be observed that weren't possible before.

All of the uses of dequeue() are in ReferenceQueue - none of them require synchronization within dequeue().

babsingh · 2024-11-28T20:25:45Z

All of the uses of dequeue() are in ReferenceQueue - none of them require synchronization within dequeue().

Once the synchronized block in Reference.dequeue is removed, there will be data contention in updating Reference.state when ReferenceQueue.poll or ReferenceQueue.remove is invoked concurrently with Reference.enqueue.

pshipton · 2024-11-28T21:01:10Z

I don't think the change to remove() is correct in #20694 (comment)

I've pushed a new commit (I can squash before merge) to ensure lock ordering and consistency between enqueue and dequeue.

jcl/src/java.base/share/classes/java/lang/ref/Reference.java

babsingh · 2024-11-28T21:28:59Z

I don't think the change to remove() is correct in #20694 (comment)

head and empty fields need to be updated in a single synchronized block. Spreading the operations across two synchronized blocks will lead to inconsistency in updating those fields. My initial proposal, before updating my previous comment, was the following:

public Reference<? extends T> remove(long timeout) throws IllegalArgumentException, InterruptedException {
        if (timeout < 0) throw new IllegalArgumentException();

        Reference ref;
        boolean notifyAll = true;
        synchronized(this) {
                if(empty) {
                        wait(timeout);
                        if(empty) return null;
                }
                ref = references[head];
                references[head++] = null;
                if(head == references.length) {
                        head = 0;
                }
                if(head == tail) {
                        empty = true;
                        notifyAll = false;
                }
        }
        ref.dequeue(); // Moved outside the synchronized block
        if (notifyAll) {
                synchronized(this) {
                        notifyAll();
                }
        }
        return ref;
}

pshipton · 2024-11-29T13:29:59Z

jenkins test sanity plinux jdk8

pshipton · 2024-11-29T13:33:23Z

jcl/src/java.base/share/classes/java/lang/ref/ReferenceQueue.java

 	/*[PR CMVC 96472] deadlock loading sun.misc.Cleaner */
 	/*[PR 102259] call Cleaner.clean(), do not enqueue */
 	if (reference instanceof Cleaner) {
 		reference.dequeue();
 		((Cleaner)reference).clean();
-		return true;
+		reference.setEnqueued();


I suspect this, and the other a few lines below, should be removed, but it's what was done previously.

I don't think we want this call to setEnqueued(), nor the one on line 213.

keithc-ca · 2024-11-29T15:33:51Z

jcl/src/java.base/share/classes/java/lang/ref/ReferenceQueue.java

 	/*[PR CMVC 96472] deadlock loading sun.misc.Cleaner */
 	/*[PR 102259] call Cleaner.clean(), do not enqueue */
 	if (reference instanceof Cleaner) {
 		reference.dequeue();
 		((Cleaner)reference).clean();
-		return true;
+		reference.setEnqueued();


I don't think we want this call to setEnqueued(), nor the one on line 213.

jcl/src/java.base/share/classes/java/lang/ref/ReferenceQueue.java

keithc-ca · 2024-11-29T15:37:28Z

jcl/src/java.base/share/classes/java/lang/ref/Reference.java

+	}
+
+	/**
+	 * Called when a Reference has been added to it's ReferenceQueue.


Please correct the spelling of "its".

pshipton · 2024-11-29T15:49:56Z

Previous PR build https://openj9-jenkins.osuosl.org/job/PullRequest-OpenJ9/6597/ passed.

pshipton · 2024-11-29T15:55:38Z

jenkins compile plinux jdk8

pshipton · 2024-11-29T15:57:59Z

jenkins compile plinux jdk11

pshipton · 2024-11-29T23:18:46Z

Testing
8: https://trssrtp1.fyre.ibm.com/resultSummary?parentId=674a31739ce27a3b442690e4
11: https://trssrtp1.fyre.ibm.com/resultSummary?parentId=674a4ab39ce27a3b442717cb

pshipton · 2024-12-02T13:12:16Z

Just some known problems in the test results, the non-network failures passed in reruns.

jcl/src/java.base/share/classes/java/lang/ref/ReferenceQueue.java

jcl/src/java.base/share/classes/java/lang/ref/Reference.java

pshipton · 2024-12-02T15:05:42Z

jenkins compile plinux jdk8,jdk21

jcl/src/java.base/share/classes/java/lang/ref/ReferenceQueue.java

pshipton · 2024-12-02T16:13:36Z

~~Updated.~~

pshipton · 2024-12-02T16:19:33Z

jenkins compile amac jdk21

pshipton · 2024-12-02T16:38:00Z

Created ibmruntimes/openj9-openjdk-jdk#900 and a backport for jdk23. I accidentally pushed the same change to jdk21. We shouldn't remove the constructor until z/OS and valhalla are also updated.

keithc-ca · 2024-12-02T16:42:38Z

jcl/src/java.base/share/classes/java/lang/ref/Reference.java

 		T tempReferent = referent;
 		synchronized(this) {
 			/* Static order for the following code (DO NOT CHANGE) */
-			tempQueue = queue;
+			final ReferenceQueue tempQueue = queue;
 			queue = null;
 			if (state == STATE_ENQUEUED || tempQueue == null) {


Perhaps this test could be reordered to (sometimes) avoid a volatile read of state?

if ((null == tempQueue) || (STATE_ENQUEUED == state)) {

keithc-ca · 2024-12-02T17:03:37Z

jcl/src/java.base/share/classes/java/lang/ref/Reference.java

-					reprocess();
-				}
+			tempQueue.enqueue(this);
+			if (null != tempReferent) {


Lines 200 and 204 should be consistent: the null (or the constant) go on the left or the right, but not a mix of both, please.

STATE_ENQUEUED is also a constant: it should go on the left just like null.

keithc-ca · 2024-12-02T18:30:36Z

jcl/src/java.base/share/classes/java/lang/ref/Reference.java

-					reprocess();
-				}
+			tempQueue.enqueue(this);
+			if (null != tempReferent) {


STATE_ENQUEUED is also a constant: it should go on the left just like null.

keithc-ca · 2024-12-02T18:31:42Z

jcl/src/java.base/share/classes/java/lang/ref/ReferenceQueue.java

 ReferenceQueue(int value) {
 	this();
 }


This constructor can also be removed now that ibmruntimes/openj9-openjdk-jdk#899 and back-ports have been merged.

Not yet, see #20694 (comment)

Right, I didn't consider z/OS or Valhalla.

Issue eclipse-openj9#13823 Signed-off-by: Peter Shipton <[email protected]>

Signed-off-by: Peter Shipton <[email protected]>

pshipton added the comp:vm label Nov 27, 2024

pshipton requested review from babsingh and keithc-ca November 27, 2024 21:52

keithc-ca requested changes Nov 27, 2024

View reviewed changes

jcl/src/java.base/share/classes/java/lang/ref/Reference.java Outdated Show resolved Hide resolved

pshipton force-pushed the refqueue branch from a854d00 to 2c741e2 Compare November 28, 2024 02:43

keithc-ca reviewed Nov 28, 2024

View reviewed changes

jcl/src/java.base/share/classes/java/lang/ref/Reference.java Outdated Show resolved Hide resolved

pshipton force-pushed the refqueue branch from 2c741e2 to bcf15a7 Compare November 28, 2024 14:26

keithc-ca approved these changes Nov 28, 2024

View reviewed changes

pshipton force-pushed the refqueue branch from 2f005d0 to 83e3657 Compare November 28, 2024 21:02

keithc-ca requested changes Nov 28, 2024

View reviewed changes

jcl/src/java.base/share/classes/java/lang/ref/Reference.java Outdated Show resolved Hide resolved

pshipton force-pushed the refqueue branch from 83e3657 to 8984f63 Compare November 28, 2024 21:05

pshipton force-pushed the refqueue branch from 8984f63 to 6b85f39 Compare November 29, 2024 13:27

pshipton commented Nov 29, 2024

View reviewed changes

keithc-ca requested changes Nov 29, 2024

View reviewed changes

pshipton force-pushed the refqueue branch from 6b85f39 to 09f850c Compare November 29, 2024 15:50

keithc-ca requested changes Dec 2, 2024

View reviewed changes

jcl/src/java.base/share/classes/java/lang/ref/ReferenceQueue.java Show resolved Hide resolved

jcl/src/java.base/share/classes/java/lang/ref/Reference.java Show resolved Hide resolved

pshipton force-pushed the refqueue branch 2 times, most recently from 7ffa7c4 to 2ecb6c9 Compare December 2, 2024 15:05

keithc-ca requested changes Dec 2, 2024

View reviewed changes

jcl/src/java.base/share/classes/java/lang/ref/ReferenceQueue.java Outdated Show resolved Hide resolved

keithc-ca mentioned this pull request Dec 2, 2024

Simplify constructor of NativeReferenceQueue ibmruntimes/openj9-openjdk-jdk#899

Merged

keithc-ca reviewed Dec 2, 2024

View reviewed changes

pshipton force-pushed the refqueue branch from 923c4e9 to 2fb1af5 Compare December 2, 2024 16:48

keithc-ca reviewed Dec 2, 2024

View reviewed changes

pshipton force-pushed the refqueue branch from 2fb1af5 to fcd9e5b Compare December 2, 2024 17:08

keithc-ca requested changes Dec 2, 2024

View reviewed changes

pshipton added 2 commits December 2, 2024 13:35

Remove some synchronization in Reference to avoid deadlock

66b7d16

Issue eclipse-openj9#13823 Signed-off-by: Peter Shipton <[email protected]>

Remove unused ReferenceQueue methods from jdk21+

9f5f6b0

Signed-off-by: Peter Shipton <[email protected]>

pshipton force-pushed the refqueue branch from fcd9e5b to 9f5f6b0 Compare December 2, 2024 18:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove some synchronization in Reference to avoid deadlock #20694

Remove some synchronization in Reference to avoid deadlock #20694

pshipton commented Nov 27, 2024

pshipton commented Nov 27, 2024

pshipton commented Nov 28, 2024

babsingh commented Nov 28, 2024 •

edited

Loading

babsingh commented Nov 28, 2024 •

edited

Loading

keithc-ca commented Nov 28, 2024

babsingh commented Nov 28, 2024

pshipton commented Nov 28, 2024

babsingh commented Nov 28, 2024 •

edited

Loading

pshipton commented Nov 29, 2024

pshipton Nov 29, 2024

keithc-ca Nov 29, 2024

keithc-ca Nov 29, 2024

keithc-ca Nov 29, 2024

pshipton commented Nov 29, 2024

pshipton commented Nov 29, 2024

pshipton commented Nov 29, 2024

pshipton commented Nov 29, 2024

pshipton commented Dec 2, 2024

pshipton commented Dec 2, 2024

pshipton commented Dec 2, 2024 •

edited

Loading

pshipton commented Dec 2, 2024

pshipton commented Dec 2, 2024 •

edited

Loading

keithc-ca Dec 2, 2024

pshipton Dec 2, 2024

pshipton Dec 2, 2024

keithc-ca Dec 2, 2024

pshipton Dec 2, 2024

keithc-ca Dec 2, 2024

keithc-ca Dec 2, 2024

keithc-ca Dec 2, 2024

pshipton Dec 2, 2024

keithc-ca Dec 2, 2024

Remove some synchronization in Reference to avoid deadlock #20694

Are you sure you want to change the base?

Remove some synchronization in Reference to avoid deadlock #20694

Conversation

pshipton commented Nov 27, 2024

pshipton commented Nov 27, 2024

pshipton commented Nov 28, 2024

babsingh commented Nov 28, 2024 • edited Loading

babsingh commented Nov 28, 2024 • edited Loading

keithc-ca commented Nov 28, 2024

babsingh commented Nov 28, 2024

pshipton commented Nov 28, 2024

babsingh commented Nov 28, 2024 • edited Loading

pshipton commented Nov 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pshipton commented Nov 29, 2024

pshipton commented Nov 29, 2024

pshipton commented Nov 29, 2024

pshipton commented Nov 29, 2024

pshipton commented Dec 2, 2024

pshipton commented Dec 2, 2024

pshipton commented Dec 2, 2024 • edited Loading

pshipton commented Dec 2, 2024

pshipton commented Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

babsingh commented Nov 28, 2024 •

edited

Loading

babsingh commented Nov 28, 2024 •

edited

Loading

babsingh commented Nov 28, 2024 •

edited

Loading

pshipton commented Dec 2, 2024 •

edited

Loading

pshipton commented Dec 2, 2024 •

edited

Loading