r=jdlee
Issue updated.
On 2/22/11 4:45 PM, Ed Burns wrote:
> 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
>
>
>
--
Jason Lee
Senior Member of Technical Staff at Oracle
http://blogs.steeplesoft.com