synchronization problem in EJBMethodInterceptor

[David Jencks <da...@yahoo.com>]

We've found a synchronization problem in EJBMethodInterceptor, see  
http://issues.apache.org/jira/browse/GERONIMO-2079.

  kevan pointed out that although the fix makes operationMap work, it  
leaves all the other fields in an indeterminate state. We need  
something better.

My first thought was to initialize everything in the constructor or  
readObject method. This won't work because in general the target  
container won't be deployed when a proxy as part of an ejb ref is  
deserialized. There's no way around this, since the target might be  
in an app that hasn't started or even been installed/deployed yet.

My current thought is to try to put all the info into a data object  
which is put into a volatile field. Comments and suggestions very  
welcome.

The diff from my first (broken) fix is here:

--- branches/v2_1/openejb2/modules/core/src/java/org/openejb/proxy/ 
EJBMethodInterceptor.java	2006-06-03 19:29:45 UTC (rev 2664)
+++ branches/v2_1/openejb2/modules/core/src/java/org/openejb/proxy/ 
EJBMethodInterceptor.java	2006-06-06 04:40:11 UTC (rev 2665)
@@ -5,6 +5,7 @@
  import java.lang.reflect.Method;
  import java.rmi.NoSuchObjectException;
  import java.rmi.RemoteException;
+
  import javax.ejb.EJBException;
  import javax.ejb.EJBObject;
  import javax.ejb.Handle;
@@ -46,7 +47,7 @@
      /**
       * Map from interface method ids to vop ids.
       */
-    private transient int[] operationMap;
+    private volatile transient int[] operationMap;

      /**
       * Metadata for the proxy
@@ -79,11 +80,12 @@
      // This really should be on a bean by bean basis.
      static String localCopyProperty = null;
      static boolean openejbLocalcopy = true;
+
      static {
          localCopyProperty = System.getProperty("openejb.localcopy");
          if (localCopyProperty == null) localCopyProperty = "true";
          boolean openejbLocalcopy = localCopyProperty.equals 
("true") ? true : false;
-        log.info("OPENEJB: Calls to remote interfaces will "+ 
(openejbLocalcopy?"":"not ")+"have their values copied.");
+        log.info("OPENEJB: Calls to remote interfaces will " +  
(openejbLocalcopy ? "" : "not ") + "have their values copied.");
      }


@@ -179,15 +181,24 @@

      private EJBInvocation createEJBInvocation(Method method,  
MethodProxy methodProxy, Object[] args) throws Throwable {
          // fault in the operation map if we don't have it yet
+        //make a local copy to avoid accessing main memory more than  
once
+        int[] operationMap = this.operationMap;
          if (operationMap == null) {
-            try {
-                loadContainerInfo();
-            } catch (ContainerNotFoundException e) {
-                if (!interfaceType.isLocal()) {
-                    throw new NoSuchObjectException(e.getMessage());
-                } else {
-                    throw new NoSuchObjectLocalException(e.getMessage 
());
+            synchronized (this) {
+                //Note that operationMap is volatile, so this double  
checked locking should work
+                //according to William Pugh, at least for jdk >= 1.5
+                if (this.operationMap == null) {
+                    try {
+                        loadContainerInfo();
+                    } catch (ContainerNotFoundException e) {
+                        if (!interfaceType.isLocal()) {
+                            throw new NoSuchObjectException 
(e.getMessage());
+                        } else {
+                            throw new NoSuchObjectLocalException 
(e.getMessage());
+                        }
+                    }
                  }
+                operationMap = this.operationMap;
              }
          }

thanks
david jencks