TOMEE-4654 - Allow the component naming context to be made read-only

arquillian/arquillian-tomee-embedded/src/test/java/org/apache/openejb/arquillian/embedded/EmbeddedTomEEContainerTest.java

@@ -71,6 +71,11 @@ public void restServiceIsDeployed() throws Exception {
         assertEquals("foo", read);
     }
 
+    /**
+     * EE.5.3.4 and Enterprise Beans 10.4.4 require the component naming context to be read only, but
+     * TomEE only enforces that when openejb.forceReadOnlyAppNamingContext is turned on, so that
+     * applications writing into their own naming context keep working. Without it the write succeeds.
+     */
     @Test
     public void testEjbCanCreateSubContextByDefault() throws Exception {
     	String originalValue = System.getProperty(javax.naming.Context.INITIAL_CONTEXT_FACTORY);
@@ -79,11 +84,11 @@ public void testEjbCanCreateSubContextByDefault() throws Exception {
     	}
         try {
 	        String result = ejb.createSubContext();
-	        assertEquals("Cannot create sub context", "created", result);
+	        assertEquals("created", result);
         } finally {
         	if(originalValue == null) {
         		System.clearProperty(javax.naming.Context.INITIAL_CONTEXT_FACTORY);
-        	} 
+        	}
         }
     }
 }

container/openejb-core/src/main/java/org/apache/openejb/AppContext.java

@@ -30,6 +30,7 @@
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.concurrent.atomic.AtomicInteger;
 
 /**
  * @version $Rev$ $Date$
@@ -42,6 +43,8 @@ public class AppContext extends DeploymentContext {
     private final Context appJndiContext;
     private final boolean standaloneModule;
     private boolean cdiEnabled;
+    private boolean readOnlyAppNamingContext;
+    private final AtomicInteger pendingLateModules = new AtomicInteger();
     private WebBeansContext webBeansContext;
     private final Collection<Injection> injections = new HashSet<>();
     private final Map<String, Object> bindings = new HashMap<>();
@@ -59,6 +62,41 @@ public AppContext(final String id, final SystemInstance systemInstance, final Cl
         this.standaloneModule = standaloneModule;
     }
 
+    /**
+     * Whether the component naming contexts of this application must be marked read only once its
+     * deployments have been created, as required by EE.5.3.4 and Enterprise Beans 10.4.4. The flag is
+     * recorded when the application is configured but only applied after the container's own bindings
+     * are done, so that modules deployed later - the web modules of an ear, for instance - are covered
+     * too.
+     */
+    public boolean isReadOnlyAppNamingContext() {
+        return readOnlyAppNamingContext;
+    }
+
+    public void setReadOnlyAppNamingContext(final boolean readOnlyAppNamingContext) {
+        this.readOnlyAppNamingContext = readOnlyAppNamingContext;
+    }
+
+    /**
+     * How many modules are still to be deployed after createApplication returns - the web modules of
+     * an ear, which TomcatWebAppBuilder starts one by one. The application scoped naming context stays
+     * writable until the last of them is in, so the container can keep binding into it.
+     */
+    public void setPendingLateModules(final int count) {
+        pendingLateModules.set(count);
+    }
+
+    /**
+     * Consumes one deployment pass.
+     *
+     * @return true when no further module can bind into the application naming context
+     */
+    public boolean lastModuleDeployed() {
+        // the count is how many passes are still to come after this one, so a pass only closes the
+        // application context when there is nothing left to consume
+        return pendingLateModules.get() <= 0 || pendingLateModules.getAndDecrement() <= 0;
+    }
+
     public Collection<Injection> getInjections() {
         return injections;
     }

container/openejb-core/src/main/java/org/apache/openejb/assembler/classic/Assembler.java

@@ -35,6 +35,7 @@
 import org.apache.openejb.Injection;
 import org.apache.openejb.JndiConstants;
 import org.apache.openejb.MethodContext;
+import org.apache.openejb.ModuleContext;
 import org.apache.openejb.NoSuchApplicationException;
 import org.apache.openejb.OpenEJBException;
 import org.apache.openejb.OpenEJBRuntimeException;
@@ -282,7 +283,7 @@ public class Assembler extends AssemblerTool implements org.apache.openejb.spi.A
     public static final String TIMER_STORE_CLASS = "timerStore.class";
     private static final ReentrantLock lock = new ReentrantLock(true);
     public static final String OPENEJB_TIMERS_ON = "openejb.timers.on";
-    static final String FORCE_READ_ONLY_APP_NAMING = "openejb.forceReadOnlyAppNamingContext";
+    public static final String FORCE_READ_ONLY_APP_NAMING = "openejb.forceReadOnlyAppNamingContext";
 
     public static final Class<?>[] VALIDATOR_FACTORY_INTERFACES = new Class<?>[]{ValidatorFactory.class, Serializable.class};
     public static final Class<?>[] VALIDATOR_INTERFACES = new Class<?>[]{Validator.class};
@@ -835,6 +836,11 @@ private AppContext createApplication(final AppInfo appInfo, ClassLoader classLoa
                 }
 
                 final AppContext appContext = new AppContext(appInfo.appId, SystemInstance.get(), classLoader, globalJndiContext, appJndiContext, appInfo.standaloneModule);
+                //required by spec EE.5.3.4, applied once the deployments exist - see setAppNamingContextReadOnly
+                appContext.setReadOnlyAppNamingContext(isReadOnlyAppNaming(appInfo));
+                // isSkip defers the web modules of an ear to the web app builders, so the application
+                // naming context has to stay writable until the last of them has been started
+                appContext.setPendingLateModules(appInfo.webAppAlone ? 0 : appInfo.webApps.size());
                 for (final Entry<Object, Object> entry : appInfo.properties.entrySet()) {
                     if (! Module.class.isInstance(entry.getValue())) {
                         appContext.getProperties().put(entry.getKey(), entry.getValue());
@@ -1093,11 +1099,6 @@ private AppContext createApplication(final AppInfo appInfo, ClassLoader classLoa
                 systemInstance.fireEvent(new AssemblerAfterApplicationCreated(appInfo, appContext, allDeployments));
                 logger.info("createApplication.success", appInfo.path);
 
-                //required by spec EE.5.3.4
-                if(setAppNamingContextReadOnly(allDeployments)) {
-                    logger.info("createApplication.naming", appInfo.path);
-                }
-
                 return appContext;
             } catch (final ValidationException | DeploymentException ve) {
                 throw ve;
@@ -1117,20 +1118,65 @@ private AppContext createApplication(final AppInfo appInfo, ClassLoader classLoa
         }
     }
 
+    /**
+     * Marks the component naming contexts of the deployments that were just created read only, as
+     * required by EE.5.3.4 and Enterprise Beans 10.4.4.
+     *
+     * This runs at the end of {@link #startEjbs} rather than at the end of {@link #createApplication}
+     * on purpose, for two reasons.
+     *
+     * The web modules of an ear are skipped by {@link #isSkip} and deployed later, from
+     * TomcatWebAppBuilder, so marking in createApplication would both miss their java:comp and make
+     * the container's own bindings - JndiBuilder binds app/&lt;module&gt;/&lt;bean&gt; into the app context -
+     * fail with OperationNotSupportedException on that later pass. And the containers themselves bind
+     * comp/EJBContext, comp/WebServiceContext and comp/TimerService into each bean enc while
+     * deploying, which happens in startEjbs, after initEjbs has returned.
+     *
+     * The bean contexts are closed as soon as their own deployment pass is done. The shared
+     * application context has to wait until no further module can bind into it, which for an ear
+     * means its last web module.
+     *
+     * @return true if anything was marked read only
+     */
     boolean setAppNamingContextReadOnly(final List<BeanContext> allDeployments) {
-        if("true".equals(SystemInstance.get().getProperty(FORCE_READ_ONLY_APP_NAMING, "false"))) {
-            for(BeanContext beanContext : allDeployments) {
-                Context ctx = beanContext.getJndiContext();
-
-                if(IvmContext.class.isInstance(ctx)) {
-                    IvmContext.class.cast(ctx).setReadOnly(true);
-                } else if(ContextHandler.class.isInstance(ctx)) {
-                    ContextHandler.class.cast(ctx).setReadOnly();
-                }
-            }
-            return true;
+        final AppContext appContext = appContextOf(allDeployments);
+        if (appContext == null || !appContext.isReadOnlyAppNamingContext()) {
+            return false;
+        }
+
+        for (final BeanContext beanContext : allDeployments) {
+            markReadOnly(beanContext.getJndiContext());
+        }
+
+        if (appContext.lastModuleDeployed()) {
+            markReadOnly(appContext.getAppJndiContext());
+        }
+        return true;
+    }
+
+    /**
+     * Off by default: making the component naming context read only is what the specification
+     * requires, but turning it on for everyone would break the applications that write into their
+     * own naming context after deployment. It stays opt-in until that can be a release wide
+     * behaviour change.
+     *
+     * The application scoped setting wins over the container wide one so a single application can
+     * opt in or out without the whole container following it.
+     */
+    private static boolean isReadOnlyAppNaming(final AppInfo appInfo) {
+        final String globalDefault = SystemInstance.get().getProperty(FORCE_READ_ONLY_APP_NAMING, "false");
+        final String value = appInfo == null
+                ? globalDefault
+                : appInfo.properties.getProperty(FORCE_READ_ONLY_APP_NAMING, globalDefault);
+        return Boolean.parseBoolean(value);
+    }
+
+    private static void markReadOnly(final Context ctx) {
+        if (IvmContext.class.isInstance(ctx)) {
+            IvmContext.class.cast(ctx).setReadOnly(true);
+        } else if (ContextHandler.class.isInstance(ctx)) {
+            ContextHandler.class.cast(ctx).setReadOnly();
         }
-        return false;
     }
 
     private List<String> getDuplicates(final AppInfo appInfo) {
@@ -1709,7 +1755,23 @@ public void startEjbs(final boolean start, final List<BeanContext> allDeployment
                     throw new OpenEJBException("Error starting '" + deployment.getEjbName() + "'.  Exception: " + t.getClass() + ": " + t.getMessage(), t);
                 }
             }
+
+            // the containers bind comp/EJBContext and friends into the bean encs while deploying, so
+            // the naming contexts can only be closed for writing once all of that is done
+            if (setAppNamingContextReadOnly(allDeployments)) {
+                logger.info("createApplication.naming", appContextOf(allDeployments).getId());
+            }
+        }
+    }
+
+    private static AppContext appContextOf(final List<BeanContext> allDeployments) {
+        for (final BeanContext beanContext : allDeployments) {
+            final ModuleContext moduleContext = beanContext.getModuleContext();
+            if (moduleContext != null && moduleContext.getAppContext() != null) {
+                return moduleContext.getAppContext();
+            }
         }
+        return null;
     }
 
     @SuppressWarnings("unchecked")