[cp-patches] FYI: Reference / ReferenceQueue thread safety fixes

[Jeroen Frijters]

Hi,

java.lang.ref.Reference and ReferenceQueue had a number of threading /
memory model bugs. ReferenceQueue also synchronized/waited on its own
instance, which makes it vulnerable to deadlocking when outside code
also synchronizes on the object (and this is extra risky in this case
because the ReferenceQueue lock will be taken by the GC when it enqueues
a Reference).

Regards,
Jeroen

2006-08-24  Jeroen Frijters  <[EMAIL PROTECTED]>

        * java/lang/ref/Reference.java
        (queue, nextOnQueue): Made volatile.
        (enqueue): Made thread safe.
        * java/lang/ref/ReferenceQueue.java
        (lock): New field.
        (poll): Removed synchronized.
        (enqueue): Changed to synchronize on lock object, to update
Reference
        state and return success status.
        (dequeue, remove): Synchronize on lock object.

Index: java/lang/ref/Reference.java
===================================================================
RCS file: /cvsroot/classpath/classpath/java/lang/ref/Reference.java,v
retrieving revision 1.8
diff -u -r1.8 Reference.java
--- java/lang/ref/Reference.java        13 Sep 2005 22:19:15 -0000      1.8
+++ java/lang/ref/Reference.java        24 Aug 2006 06:24:38 -0000
@@ -1,5 +1,5 @@
 /* java.lang.ref.Reference
-   Copyright (C) 1999, 2002, 2003 Free Software Foundation, Inc.
+   Copyright (C) 1999, 2002, 2003, 2006 Free Software Foundation, Inc.
 
 This file is part of GNU Classpath.
 
@@ -82,7 +82,7 @@
    * The queue this reference is registered on. This is null, if this
    * wasn't registered to any queue or reference was already enqueued.
    */
-  ReferenceQueue queue;
+  volatile ReferenceQueue queue;
 
   /**
    * Link to the next entry on the queue.  If this is null, this
@@ -91,7 +91,7 @@
    * (not to null, that value is used to mark a not enqueued
    * reference).  
    */
-  Reference nextOnQueue;
+  volatile Reference nextOnQueue;
 
   /**
    * This lock should be taken by the garbage collector, before
@@ -166,11 +166,10 @@
    */
   public boolean enqueue() 
   {
-    if (queue != null && nextOnQueue == null)
+    ReferenceQueue q = queue;
+    if (q != null)
       {
-       queue.enqueue(this);
-       queue = null;
-       return true;
+       return q.enqueue(this);
       }
     return false;
   }
Index: java/lang/ref/ReferenceQueue.java
===================================================================
RCS file: /cvsroot/classpath/classpath/java/lang/ref/ReferenceQueue.java,v
retrieving revision 1.4
diff -u -r1.4 ReferenceQueue.java
--- java/lang/ref/ReferenceQueue.java   2 Jul 2005 20:32:39 -0000       1.4
+++ java/lang/ref/ReferenceQueue.java   24 Aug 2006 06:24:43 -0000
@@ -1,5 +1,5 @@
 /* java.lang.ref.ReferenceQueue
-   Copyright (C) 1999 Free Software Foundation, Inc.
+   Copyright (C) 1999, 2006 Free Software Foundation, Inc.
 
 This file is part of GNU Classpath.
 
@@ -63,6 +63,12 @@
   private Reference first;
 
   /**
+   * This is the lock that protects our linked list and is used to signal
+   * a thread waiting in remove().
+   */
+  private final Object lock = new Object();
+
+  /**
    * Creates a new empty reference queue.
    */
   public ReferenceQueue()
@@ -76,7 +82,7 @@
    * @return a reference on the queue, if there is one,
    * <code>null</code> otherwise.  
    */
-  public synchronized Reference poll()
+  public Reference poll()
   { 
     return dequeue();
   }
@@ -84,14 +90,23 @@
   /**
    * This is called by reference to enqueue itself on this queue.  
    * @param ref the reference that should be enqueued.
+   * @return true if successful, false if not.
    */
-  synchronized void enqueue(Reference ref)
+  final boolean enqueue(Reference ref)
   {
-    /* last reference will point to itself */
-    ref.nextOnQueue = first == null ? ref : first;
-    first = ref;
-    /* this wakes only one remove thread. */
-    notify();
+    synchronized (lock)
+      {
+        if (ref.queue != this)
+          return false;
+
+        /* last reference will point to itself */
+        ref.nextOnQueue = first == null ? ref : first;
+        ref.queue = null;
+        first = ref;
+        /* this wakes only one remove thread. */
+        lock.notify();
+        return true;
+      }
   }
 
   /**
@@ -100,13 +115,16 @@
    */
   private Reference dequeue()
   {
-    if (first == null)
-      return null;
+    synchronized (lock)
+      {
+        if (first == null)
+          return null;
 
-    Reference result = first;
-    first = (first == first.nextOnQueue) ? null : first.nextOnQueue;
-    result.nextOnQueue = null;
-    return result;
+        Reference result = first;
+        first = (first == first.nextOnQueue) ? null : first.nextOnQueue;
+        result.nextOnQueue = null;
+        return result;
+      }
   }
 
   /**
@@ -118,12 +136,13 @@
    * <code>null</code> if timeout period expired.  
    * @exception InterruptedException if the wait was interrupted.
    */
-  public synchronized Reference remove(long timeout)
+  public Reference remove(long timeout)
     throws InterruptedException
   {
-    if (first == null)
+    synchronized (lock)
       {
-       wait(timeout);
+        if (first == null)
+          lock.wait(timeout);
       }
 
     return dequeue();