You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@myfaces.apache.org by lu...@apache.org on 2010/07/10 00:35:36 UTC
svn commit: r962713 - in
/myfaces/core/trunk/impl/src/main/java/org/apache/myfaces:
application/ApplicationImpl.java lifecycle/LifecycleFactoryImpl.java
lifecycle/LifecycleImpl.java
Author: lu4242
Date: Fri Jul 9 22:35:36 2010
New Revision: 962713
URL: http://svn.apache.org/viewvc?rev=962713&view=rev
Log:
MYFACES-2804 LifecycleImpl _firstRequestProcessed should be marked as volatile
Modified:
myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationImpl.java
myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/lifecycle/LifecycleFactoryImpl.java
myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/lifecycle/LifecycleImpl.java
Modified: myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationImpl.java
URL: http://svn.apache.org/viewvc/myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationImpl.java?rev=962713&r1=962712&r2=962713&view=diff
==============================================================================
--- myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationImpl.java (original)
+++ myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationImpl.java Fri Jul 9 22:35:36 2010
@@ -187,7 +187,7 @@ public class ApplicationImpl extends App
private ProjectStage _projectStage;
- private boolean _firstRequestProcessed = false;
+ private volatile boolean _firstRequestProcessed = false;
private final Map<Class<?>, List<ListenerFor>> _classToListenerForMap = new HashMap<Class<?>, List<ListenerFor>>() ;
private final Map<Class<?>, List<ResourceDependency>> _classToResourceDependencyMap = new HashMap<Class<?>, List<ResourceDependency>>() ;
Modified: myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/lifecycle/LifecycleFactoryImpl.java
URL: http://svn.apache.org/viewvc/myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/lifecycle/LifecycleFactoryImpl.java?rev=962713&r1=962712&r2=962713&view=diff
==============================================================================
--- myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/lifecycle/LifecycleFactoryImpl.java (original)
+++ myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/lifecycle/LifecycleFactoryImpl.java Fri Jul 9 22:35:36 2010
@@ -18,12 +18,13 @@
*/
package org.apache.myfaces.lifecycle;
+import java.util.Iterator;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+
import javax.faces.FacesException;
import javax.faces.lifecycle.Lifecycle;
import javax.faces.lifecycle.LifecycleFactory;
-import java.util.HashMap;
-import java.util.Iterator;
-import java.util.Map;
/**
* @author Manfred Geiler (latest modification by $Author$)
@@ -32,7 +33,13 @@ import java.util.Map;
*/
public class LifecycleFactoryImpl extends LifecycleFactory
{
- private final Map<String, Lifecycle> _lifecycles = new HashMap<String, Lifecycle>();
+ /**
+ * At start we used synchronized blocks for addLifecycle and getLifecycle. But thinking about it,
+ * use a ConcurrentHashMap is better, because retrieval operations (including get) generally
+ * do not block, and it is more often retrieval (at begin of all requests) than addition (when
+ * startup listener is called and configuration occur).
+ */
+ private final Map<String, Lifecycle> _lifecycles = new ConcurrentHashMap<String, Lifecycle>();
public LifecycleFactoryImpl()
{
@@ -41,6 +48,9 @@ public class LifecycleFactoryImpl extend
public void purgeLifecycle()
{
+ // Note this is not safe, because if by some coincidence one thread call getLifecycle between
+ // the two lines below it will throw IllegalArgumentException, but this method is not supposed
+ // to be called in production, so it is ok.
_lifecycles.clear();
addLifecycle(LifecycleFactory.DEFAULT_LIFECYCLE, new LifecycleImpl());
}
@@ -48,28 +58,28 @@ public class LifecycleFactoryImpl extend
@Override
public void addLifecycle(String id, Lifecycle lifecycle)
{
- synchronized (_lifecycles)
- {
+ //synchronized (_lifecycles)
+ //{
if (_lifecycles.get(id) != null)
{
throw new IllegalArgumentException("Lifecycle with id '" + id + "' already exists.");
}
_lifecycles.put(id, lifecycle);
- }
+ //}
}
@Override
public Lifecycle getLifecycle(String id) throws FacesException
{
- synchronized (_lifecycles)
- {
+ //synchronized (_lifecycles)
+ //{
Lifecycle lifecycle = _lifecycles.get(id);
if (lifecycle == null)
{
throw new IllegalArgumentException("Unknown lifecycle '" + id + "'.");
}
return lifecycle;
- }
+ //}
}
@Override
Modified: myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/lifecycle/LifecycleImpl.java
URL: http://svn.apache.org/viewvc/myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/lifecycle/LifecycleImpl.java?rev=962713&r1=962712&r2=962713&view=diff
==============================================================================
--- myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/lifecycle/LifecycleImpl.java (original)
+++ myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/lifecycle/LifecycleImpl.java Fri Jul 9 22:35:36 2010
@@ -18,8 +18,8 @@
*/
package org.apache.myfaces.lifecycle;
-import java.util.ArrayList;
import java.util.List;
+import java.util.concurrent.CopyOnWriteArrayList;
import java.util.logging.Level;
import java.util.logging.Logger;
@@ -57,16 +57,38 @@ public class LifecycleImpl extends Lifec
*/
public static final String FIRST_REQUEST_PROCESSED_PARAM = "org.apache.myfaces.lifecycle.first.request.processed";
- private PhaseExecutor[] lifecycleExecutors;
- private PhaseExecutor renderExecutor;
+ private final PhaseExecutor[] lifecycleExecutors;
+ private final PhaseExecutor renderExecutor;
- private final List<PhaseListener> _phaseListenerList = new ArrayList<PhaseListener>();
+ /**
+ * Initially, for ensure thread safety we used synchronization blocks and a cached
+ * _phaseListenerArray and that works. The intention is ensure atomicity between
+ * _phaseListenerList and _phaseListenerArray, but thinking more about it use
+ * CopyOnWriteArrayList and do not use _phaseListenerArray is a lot better.
+ *
+ * Most times, we have few instances of PhaseListener registered, so the advantage of
+ * use _phaseListenerArray is overcome by do not have a synchronization block on getPhaseListeners().
+ * Additionally, it is more often to perform traversals than insertions/removals and
+ * we can expect only 2 calls for getPhaseListeners() per request (so only two copy
+ * operations of a very small list).
+ */
+ private final List<PhaseListener> _phaseListenerList = new CopyOnWriteArrayList<PhaseListener>(); // new ArrayList();
- private boolean _firstRequestProcessed = false;
+ /**
+ * This variable should be marked as volatile to ensure all threads can see it
+ * after the first request is processed. Note that LifecycleImpl instance could be
+ * shared by multiple requests at the same time, so this is relevant to prevent
+ * multiple updates to FIRST_REQUEST_PROCESSED_PARAM. Really since the value
+ * only changes from false to true, have a racy single check here does not harm, but
+ * note in this case the semantic of the variable must be preserved.
+ */
+ private volatile boolean _firstRequestProcessed = false;
/**
* Lazy cache for returning _phaseListenerList as an Array.
+ *
+ * Replaced by _phaseListenerList CopyOnWriteArrayList
*/
- private PhaseListener[] _phaseListenerArray = null;
+ //private PhaseListener[] _phaseListenerArray = null;
public LifecycleImpl()
{
@@ -292,11 +314,11 @@ public class LifecycleImpl extends Lifec
{
throw new NullPointerException("PhaseListener must not be null.");
}
- synchronized (_phaseListenerList)
- {
+ //synchronized (_phaseListenerList)
+ //{
_phaseListenerList.add(phaseListener);
- _phaseListenerArray = null; // reset lazy cache array
- }
+ //_phaseListenerArray = null; // reset lazy cache array
+ //}
}
@Override
@@ -306,25 +328,26 @@ public class LifecycleImpl extends Lifec
{
throw new NullPointerException("PhaseListener must not be null.");
}
- synchronized (_phaseListenerList)
- {
+ //synchronized (_phaseListenerList)
+ //{
_phaseListenerList.remove(phaseListener);
- _phaseListenerArray = null; // reset lazy cache array
- }
+ //_phaseListenerArray = null; // reset lazy cache array
+ //}
}
@Override
public PhaseListener[] getPhaseListeners()
{
- synchronized (_phaseListenerList)
- {
+ //synchronized (_phaseListenerList)
+ //{
// (re)build lazy cache array if necessary
- if (_phaseListenerArray == null)
- {
- _phaseListenerArray = _phaseListenerList.toArray(new PhaseListener[_phaseListenerList.size()]);
- }
- return _phaseListenerArray;
- }
+ //if (_phaseListenerArray == null)
+ //{
+ // _phaseListenerArray = _phaseListenerList.toArray(new PhaseListener[_phaseListenerList.size()]);
+ //}
+ //return _phaseListenerArray;
+ //}
+ return _phaseListenerList.toArray(new PhaseListener[_phaseListenerList.size()]);
}
private void publishException (Throwable e, PhaseId phaseId, FacesContext facesContext)
@@ -345,11 +368,15 @@ public class LifecycleImpl extends Lifec
{
if(!_firstRequestProcessed)
{
- _firstRequestProcessed = true;
-
+ // The order here is important. First it is necessary to put
+ // the value on application map before change the value here.
+ // If multiple threads reach this point concurrently, the
+ // variable will be written on the application map at the same
+ // time but always with the same value.
facesContext.getExternalContext().getApplicationMap()
- .put(FIRST_REQUEST_PROCESSED_PARAM, Boolean.TRUE);
+ .put(FIRST_REQUEST_PROCESSED_PARAM, Boolean.TRUE);
+
+ _firstRequestProcessed = true;
}
}
-
-}
+}
\ No newline at end of file