dev@javaserverfaces.java.net

Seeking review for GLASSFISH-15809

From: Ed Burns <edward.burns_at_oracle.com>
Date: Tue, 22 Feb 2011 14:45:54 -0800

Small but fundamental change to FactoryFinder http://java.net/jira/browse/GLASSFISH-15809

The root cause of our virtual server woes is an invalid assumption
FactoryFinder has made since its first commit. The assumption. There
is a 1:1 mapping between ServletContext and web-app ClassLoader. This
assumption is not valid in the case of virtual servers. In a deployment
with N virtual servers, there are N ServletContexts for each web-app
ClassLoader. According to Shing-wai, this is not a bug, and I agree
with him.


SECTION: Modified Files
----------------------------
M jsf-api/src/main/java/javax/faces/FactoryFinder.java

- Instead of having the FactoryManagerCache use ClassLoader alone as the
  key for the FactoryManager instances, use a new class,
  FactoryManagerCacheKey that combines the ClassLoader with the
  ServletContext instance. I had to be careful to handle the case when
  FacesContext.getCurrentInstance() returns null.

M jsf-ri/src/main/java/com/sun/faces/config/ConfigureListener.java

- When an app is undeployed, release its factories, which will cause
  them to be removed from the FactoryManagerCache.

M jsf-api/src/test/java/javax/faces/component/NamingContainerTestCase.java
M jsf-api/src/test/java/javax/faces/component/UIComponentTestCase.java
M jsf-api/src/test/java/javax/faces/validator/ValidatorTestCase.java
M jsf-api/src/test/java/javax/faces/webapp/TagTestCaseBase.java
M jsf-ri/test/com/sun/faces/config/TestFactoryInjection.java
M jsf-ri/build.xml

- add deploy target

M jsf-ri/systest-per-webapp/replace-lifecycle/src/java/com/sun/faces/systest/SimplePhaseListener.java
M jsf-ri/systest-per-webapp/replace-lifecycle/src/java/com/sun/faces/systest/ReplaceLifecycleTestCase.java

- test changes to verify 15809 is fixed.

M lib/jsf-extensions-test-time.jar

- Fix a bug in MockFacesContext that doesn't actually release the threadlocal.


SECTION: Diffs
----------------------------
Index: jsf-api/src/test/java/javax/faces/component/NamingContainerTestCase.java
===================================================================
--- jsf-api/src/test/java/javax/faces/component/NamingContainerTestCase.java (revision 8870)
+++ jsf-api/src/test/java/javax/faces/component/NamingContainerTestCase.java (working copy)
@@ -117,16 +117,16 @@
         request.setAttribute("reqScopeName", "reqScopeValue");
         response = new MockHttpServletResponse();
 
+ externalContext =
+ new MockExternalContext(servletContext, request, response);
+ lifecycle = new MockLifecycle();
+ facesContext = new MockFacesContext(externalContext, lifecycle);
         // Set up Faces API Objects
         FactoryFinder.setFactory(FactoryFinder.APPLICATION_FACTORY,
                                  "com.sun.faces.mock.MockApplicationFactory");
         FactoryFinder.setFactory(FactoryFinder.RENDER_KIT_FACTORY,
                                  "com.sun.faces.mock.MockRenderKitFactory");
 
- externalContext =
- new MockExternalContext(servletContext, request, response);
- lifecycle = new MockLifecycle();
- facesContext = new MockFacesContext(externalContext, lifecycle);
         ApplicationFactory applicationFactory = (ApplicationFactory)
             FactoryFinder.getFactory(FactoryFinder.APPLICATION_FACTORY);
         application = (MockApplication) applicationFactory.getApplication();
Index: jsf-api/src/test/java/javax/faces/component/UIComponentTestCase.java
===================================================================
--- jsf-api/src/test/java/javax/faces/component/UIComponentTestCase.java (revision 8870)
+++ jsf-api/src/test/java/javax/faces/component/UIComponentTestCase.java (working copy)
@@ -164,18 +164,18 @@
         request.setAttribute("reqScopeName", "reqScopeValue");
         response = new MockHttpServletResponse();
 
+ externalContext =
+ new MockExternalContext(servletContext, request, response);
+ Map map = new HashMap();
+ externalContext.setRequestParameterMap(map);
+ lifecycle = new MockLifecycle();
+ facesContext = new MockFacesContext(externalContext, lifecycle);
         // Set up Faces API Objects
         FactoryFinder.setFactory(FactoryFinder.APPLICATION_FACTORY,
                 "com.sun.faces.mock.MockApplicationFactory");
         FactoryFinder.setFactory(FactoryFinder.RENDER_KIT_FACTORY,
                 "com.sun.faces.mock.MockRenderKitFactory");
 
- externalContext =
- new MockExternalContext(servletContext, request, response);
- Map map = new HashMap();
- externalContext.setRequestParameterMap(map);
- lifecycle = new MockLifecycle();
- facesContext = new MockFacesContext(externalContext, lifecycle);
         ApplicationFactory applicationFactory = (ApplicationFactory)
                 FactoryFinder.getFactory(FactoryFinder.APPLICATION_FACTORY);
         application = (MockApplication) applicationFactory.getApplication();
Index: jsf-api/src/test/java/javax/faces/validator/ValidatorTestCase.java
===================================================================
--- jsf-api/src/test/java/javax/faces/validator/ValidatorTestCase.java (revision 8870)
+++ jsf-api/src/test/java/javax/faces/validator/ValidatorTestCase.java (working copy)
@@ -124,16 +124,16 @@
         request.setAttribute("reqScopeName", "reqScopeValue");
         response = new MockHttpServletResponse();
 
+ externalContext =
+ new MockExternalContext(servletContext, request, response);
+ lifecycle = new MockLifecycle();
+ facesContext = new MockFacesContext(externalContext, lifecycle);
         // Set up Faces API Objects
         FactoryFinder.setFactory(FactoryFinder.APPLICATION_FACTORY,
                                  "com.sun.faces.mock.MockApplicationFactory");
         FactoryFinder.setFactory(FactoryFinder.RENDER_KIT_FACTORY,
                                  "com.sun.faces.mock.MockRenderKitFactory");
 
- externalContext =
- new MockExternalContext(servletContext, request, response);
- lifecycle = new MockLifecycle();
- facesContext = new MockFacesContext(externalContext, lifecycle);
         ApplicationFactory applicationFactory = (ApplicationFactory)
             FactoryFinder.getFactory(FactoryFinder.APPLICATION_FACTORY);
         application = (MockApplication) applicationFactory.getApplication();
Index: jsf-api/src/test/java/javax/faces/webapp/TagTestCaseBase.java
===================================================================
--- jsf-api/src/test/java/javax/faces/webapp/TagTestCaseBase.java (revision 8870)
+++ jsf-api/src/test/java/javax/faces/webapp/TagTestCaseBase.java (working copy)
@@ -142,16 +142,16 @@
         pageContext.initialize(servlet, request, response, null,
                                true, 1024, true);
 
+ externalContext =
+ new MockExternalContext(servletContext, request, response);
+ externalContext.setRequestParameterMap(new HashMap());
+ lifecycle = new MockLifecycle();
+ facesContext = new MockFacesContext(externalContext, lifecycle);
         // Set up Faces API Objects
         FactoryFinder.setFactory(FactoryFinder.APPLICATION_FACTORY,
                                  "com.sun.faces.mock.MockApplicationFactory");
         FactoryFinder.setFactory(FactoryFinder.RENDER_KIT_FACTORY,
                                  "com.sun.faces.mock.MockRenderKitFactory");
- externalContext =
- new MockExternalContext(servletContext, request, response);
- externalContext.setRequestParameterMap(new HashMap());
- lifecycle = new MockLifecycle();
- facesContext = new MockFacesContext(externalContext, lifecycle);
         ApplicationFactory applicationFactory = (ApplicationFactory)
             FactoryFinder.getFactory(FactoryFinder.APPLICATION_FACTORY);
         application = (MockApplication) applicationFactory.getApplication();
Index: jsf-api/src/main/java/javax/faces/FactoryFinder.java
===================================================================
--- jsf-api/src/main/java/javax/faces/FactoryFinder.java (revision 8870)
+++ jsf-api/src/main/java/javax/faces/FactoryFinder.java (working copy)
@@ -67,6 +67,8 @@
 import java.lang.reflect.Constructor;
 import java.net.URL;
 import java.net.URLConnection;
+import java.util.Set;
+import javax.faces.context.FacesContext;
 
 
 /**
@@ -680,8 +682,8 @@
      */
     private static final class FactoryManagerCache {
 
- private ConcurrentMap<ClassLoader,Future<FactoryManager>> applicationMap =
- new ConcurrentHashMap<ClassLoader, Future<FactoryManager>>();
+ private ConcurrentMap<FactoryManagerCacheKey,Future<FactoryManager>> applicationMap =
+ new ConcurrentHashMap<FactoryManagerCacheKey, Future<FactoryManager>>();
 
 
         // ------------------------------------------------------ Public Methods
@@ -689,8 +691,10 @@
 
         private FactoryManager getApplicationFactoryManager(ClassLoader cl) {
 
+ FactoryManagerCacheKey key = new FactoryManagerCacheKey(cl, applicationMap);
+
             while (true) {
- Future<FactoryManager> factories = applicationMap.get(cl);
+ Future<FactoryManager> factories = applicationMap.get(key);
                 if (factories == null) {
                     Callable<FactoryManager> callable =
                           new Callable<FactoryManager>() {
@@ -702,7 +706,7 @@
 
                     FutureTask<FactoryManager> ft =
                           new FutureTask<FactoryManager>(callable);
- factories = applicationMap.putIfAbsent(cl, ft);
+ factories = applicationMap.putIfAbsent(key, ft);
                     if (factories == null) {
                         factories = ft;
                         ft.run();
@@ -717,14 +721,14 @@
                                    ce.toString(),
                                    ce);
                     }
- applicationMap.remove(cl);
+ applicationMap.remove(key);
                 } catch (InterruptedException ie) {
                     if (LOGGER.isLoggable(Level.FINEST)) {
                         LOGGER.log(Level.FINEST,
                                    ie.toString(),
                                    ie);
                     }
- applicationMap.remove(cl);
+ applicationMap.remove(key);
                 } catch (ExecutionException ee) {
                     throw new FacesException(ee);
                 }
@@ -735,14 +739,89 @@
 
 
         public void removeApplicationFactoryManager(ClassLoader cl) {
+ FactoryManagerCacheKey key = new FactoryManagerCacheKey(cl, applicationMap);
 
- applicationMap.remove(cl);
+ applicationMap.remove(key);
 
         }
 
     } // END FactoryCache
 
+ private static final class FactoryManagerCacheKey {
+ private ClassLoader cl;
+ private Object context;
 
+ private static final String KEY = FactoryFinder.class.getName() + "." +
+ FactoryManagerCacheKey.class.getSimpleName();
+
+ public FactoryManagerCacheKey(ClassLoader cl,
+ Map<FactoryManagerCacheKey,Future<FactoryManager>> factoryMap) {
+ this.cl = cl;
+ FacesContext facesContext = FacesContext.getCurrentInstance();
+ if (null != facesContext) {
+ Map<String, Object> appMap = facesContext.getExternalContext().getApplicationMap();
+ Object val = appMap.get(KEY);
+ if (null == val) {
+ context = new Long(System.currentTimeMillis());
+ appMap.put(KEY, context);
+ } else {
+ context = val;
+ }
+ } else {
+ // We don't have a FacesContext.
+ // Our only recourse is to inspect the keys of the
+ // factoryMap and see if any of them has a classloader
+ // equal to our argument cl.
+ Set<FactoryManagerCacheKey> keys = factoryMap.keySet();
+ FactoryManagerCacheKey match = null;
+ for (FactoryManagerCacheKey cur : keys) {
+ if (this.cl.equals(cur.cl)) {
+ match = cur;
+ if (null != match) {
+ LOGGER.log(Level.WARNING, "Multiple JSF Applications found on same ClassLoader. Unable to safely determine which FactoryManager instance to use. Defaulting to first match.");
+ }
+ }
+ }
+ if (null != match) {
+ this.context = match.context;
+ }
+ }
+ }
+
+ private FactoryManagerCacheKey() {}
+
+ @Override
+ public boolean equals(Object obj) {
+ if (obj == null) {
+ return false;
+ }
+ if (getClass() != obj.getClass()) {
+ return false;
+ }
+ final FactoryManagerCacheKey other = (FactoryManagerCacheKey) obj;
+ if (this.cl != other.cl && (this.cl == null || !this.cl.equals(other.cl))) {
+ return false;
+ }
+ if (this.context != other.context && (this.context == null || !this.context.equals(other.context))) {
+ return false;
+ }
+ return true;
+ }
+
+ @Override
+ public int hashCode() {
+ int hash = 7;
+ hash = 97 * hash + (this.cl != null ? this.cl.hashCode() : 0);
+ hash = 97 * hash + (this.context != null ? this.context.hashCode() : 0);
+ return hash;
+ }
+
+
+
+
+ }
+
+
     /**
      * Maintains the factories for a single web application.
      */
Index: jsf-ri/test/com/sun/faces/config/TestFactoryInjection.java
===================================================================
--- jsf-ri/test/com/sun/faces/config/TestFactoryInjection.java (revision 8870)
+++ jsf-ri/test/com/sun/faces/config/TestFactoryInjection.java (working copy)
@@ -235,7 +235,9 @@
 
         // invoke the FactoryConfigProcessor
         FactoryConfigProcessor fcp = new FactoryConfigProcessor(false);
+ InitFacesContext initContext = new InitFacesContext(sc);
         fcp.process(sc, new DocumentInfo[]{new DocumentInfo(d, null)});
+ initContext.release();
 
         // now get an FacesContext instance from the Factory and ensure
         // no injection occured.
@@ -264,7 +266,9 @@
         // process the document. This should cause the the InjectionFacesContextFactory
         // to be put into play since there is more than one FacesContextFactory
         // being configured
+ initContext = new InitFacesContext(sc);
         fcp.process(sc, new DocumentInfo[]{new DocumentInfo(d, null)});
+ initContext.release();
 
         // get the FacesContextFactory instance. The top-level factory should
         // be the InjectionFacesContextFactory.
Index: jsf-ri/src/main/java/com/sun/faces/config/ConfigureListener.java
===================================================================
--- jsf-ri/src/main/java/com/sun/faces/config/ConfigureListener.java (revision 8870)
+++ jsf-ri/src/main/java/com/sun/faces/config/ConfigureListener.java (working copy)
@@ -348,6 +348,7 @@
             ApplicationAssociate.setCurrentInstance(null);
             // Release the initialization mark on this web application
             ConfigManager.getInstance().destory(context);
+ FactoryFinder.releaseFactories();
             if (initContext != null) {
                 initContext.release();
             }
Index: jsf-ri/build.xml
===================================================================
--- jsf-ri/build.xml (revision 8870)
+++ jsf-ri/build.xml (working copy)
@@ -700,7 +700,7 @@
 
 
     <target name="passthru">
- <ant antfile="build-tests.xml" target="execute.cactus.tests"
+ <ant antfile="build-tests.xml" target="passthru"
              inheritAll="true">
           <property name="force.no.cluster" value="true" />
         </ant>
@@ -715,6 +715,14 @@
 
     </target>
 
+ <target name="deploy">
+ <ant antfile="build-tests.xml" target="deploy"
+ inheritAll="true">
+ <property name="force.no.cluster" value="true" />
+ </ant>
+
+ </target>
+
     <target name="prepare.cactus.webapp">
         <ant antfile="build-tests.xml" target="prepare.test.webapp"/>
     </target>
Index: jsf-ri/systest-per-webapp/replace-lifecycle/src/java/com/sun/faces/systest/SimplePhaseListener.java
===================================================================
--- jsf-ri/systest-per-webapp/replace-lifecycle/src/java/com/sun/faces/systest/SimplePhaseListener.java (revision 8870)
+++ jsf-ri/systest-per-webapp/replace-lifecycle/src/java/com/sun/faces/systest/SimplePhaseListener.java (working copy)
@@ -43,6 +43,7 @@
 import javax.faces.event.PhaseEvent;
 import javax.faces.event.PhaseId;
 import javax.faces.event.PhaseListener;
+import java.util.Map;
 
 public class SimplePhaseListener implements PhaseListener {
 
@@ -57,8 +58,12 @@
 
 
     public void beforePhase(PhaseEvent event) {
- event.getFacesContext().getExternalContext().getRequestMap().put("beforePhase",
- "beforePhase");
+ Map<String, Object> requestMap =
+ event.getFacesContext().getExternalContext().getRequestMap();
+ String message = requestMap.containsKey("beforePhase") ?
+ requestMap.get("beforePhase").toString() : "";
+ requestMap.put("beforePhase",
+ message + " beforePhase");
         event.getFacesContext().getExternalContext().getRequestMap().put("lifecycleImpl",
                                                                          event.getSource());
     }
Index: jsf-ri/systest-per-webapp/replace-lifecycle/src/java/com/sun/faces/systest/ReplaceLifecycleTestCase.java
===================================================================
--- jsf-ri/systest-per-webapp/replace-lifecycle/src/java/com/sun/faces/systest/ReplaceLifecycleTestCase.java (revision 8870)
+++ jsf-ri/systest-per-webapp/replace-lifecycle/src/java/com/sun/faces/systest/ReplaceLifecycleTestCase.java (working copy)
@@ -128,7 +128,10 @@
 
     public void testReplaceLifecycle() throws Exception {
         HtmlPage page = getPage("/faces/test.jsp");
- assertTrue(-1 != page.asText().indexOf("beforePhase"));
+ String pageText = page.asText();
+ assertTrue(-1 != pageText.indexOf("beforePhase"));
+ // Ensure the phaseListener is only called once.
+ assertTrue(!pageText.matches("(?s).*beforePhase.*beforePhase.*"));
         
     }
 
Index: lib/jsf-extensions-test-time.jar
===================================================================
Cannot display: file marked as a binary type.
svn:mime-type = application/octet-stream



-- 
| edward.burns_at_oracle.com | office: +1 407 458 0017
| homepage:               | http://ridingthecrest.com/