src/share/classes/sun/rmi/server/Activation.java

rev 3756 : 6896297: (rmi) fix ConcurrentModificationException causing TCK failure
Reviewed-by: XXX

@@ -28,10 +28,11 @@
 import java.io.ByteArrayOutputStream;
 import java.io.File;
 import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.InputStream;
+import java.io.ObjectOutputStream;
 import java.io.OutputStream;
 import java.io.PrintStream;
 import java.io.PrintWriter;
 import java.io.Serializable;
 import java.lang.Process;

@@ -132,10 +133,60 @@
  * @author      Ann Wollrath
  * @since       1.2
  */
 public class Activation implements Serializable {
 
+    /*
+     * Synchronization occurs over several objects in this code.
+     * These objects are:
+     *
+     *  - startupLock
+     *  - initLock
+     *  - Activation.this
+     *  - instances of GroupEntry
+     *  - log (a ReliableLog)
+     *  - idTable (a HashMap)
+     *  - groupTable (a HashMap)
+     *
+     * The startupLock and initLock locks are used only during
+     * initialization and shutdown. The lock on Activation.this
+     * is used to guard groupSemaphore and to guard writes to the
+     * shuttingDown flag (which is volatile and which is read
+     * without synchronization).
+     *
+     * Several operations on GroupEntry instances hold a lock on
+     * that instance while performing logging operations and while
+     * while manipulating idTable or groupTable.
+     *
+     * Log operations, notably addLogRecord(), hold a lock on the
+     * log instance. (Methods of the log itself also take locks
+     * on the log object.) The log.snapshot() operation serializes
+     * the log, including instances of Activation (this class),
+     * which in turn must lock idTable and groupTable to prevent
+     * concurrent modification during serialization.
+     *
+     * The idTable and groupTable HashMaps are locked while simple
+     * operations are performed on them. Note that we cannot
+     * straightforwardly convert these to ConcurrentHashMap,
+     * because it has a different serialized form from HashMap.
+     *
+     * During serialization, the writeObject() method of this class
+     * takes locks on groupTable and idTable in that order.
+     * 
+     * Based on the above paths through the code, a partial
+     * ordering for the above locks must be as follows:
+     *
+     * 1. GroupEntry instances
+     * 2. log
+     * 3. groupTable
+     * 4. idTable
+     *
+     * For example, code may lock the log while holding the
+     * lock for a GroupEntry, but code must not lock the log
+     * while holding a lock on groupTable.
+     */
+
     /** indicate compatibility with JDK 1.2 version of class */
     private static final long serialVersionUID = 2921265612698155191L;
 
     private static final byte MAJOR_VERSION = 1;
     private static final byte MINOR_VERSION = 0;

@@ -272,10 +323,23 @@
                 e.printStackTrace();
             }
         }
     }
 
+    private void writeObject(ObjectOutputStream ostream) throws IOException {
+        // Synchronize on groupTable and idTable while writing them to the
+        // object stream in order to avoid ConcurrentModificationException. It
+        // might be preferable to synchronize them individually, but then we'd
+        // have to write the fields individually instead of using
+        // defaultWriteObject().
+        synchronized (groupTable) {
+            synchronized (idTable) {
+                ostream.defaultWriteObject();
+            }
+        }
+    }
+
     private static class SystemRegistryImpl extends RegistryImpl {
 
         private static final String NAME = ActivationSystem.class.getName();
         private final ActivationSystem systemStub;
 

@@ -513,15 +577,16 @@
             checkShutdown();
             RegistryImpl.checkAccess("ActivationSystem.unregisterGroup");
 
             // remove entry before unregister so state is updated before
             // logged
+            GroupEntry entry;
             synchronized (groupTable) {
-                GroupEntry entry = getGroupEntry(id);
+                entry = getGroupEntry(id);
                 groupTable.remove(id);
-                entry.unregisterGroup(true);
             }
+            entry.unregisterGroup(true);
         }
 
         public ActivationDesc setActivationDesc(ActivationID id,
                                                 ActivationDesc desc)
             throws ActivationException, UnknownObjectException, RemoteException