Hi Jacob, All,
Thanks for quick review and comments.
Here is a revised change bundle incorporating Jacob's feedback. Please
review.
Thanks
-Jayashri
jacob_at_hookom.net wrote:
>If a Lifecycle is shared across multiple threads (FacesServlet)....
>
>To be able to use a temporary reference, it requires that the List it
>points to is immutable. So the other part of the solution occurs
>within the add/remove phase listener. Similar to commons-collection's
>FastArrayList implementation (ala Craig), upon mutation, the internal
>list is cloned and the clone is mutated, not the original. Once the
>cloned version is mutated, then assign it to the internal reference.
>This assures that the temporary reference for any given thread will
>never change within a process, even if the Lifecycle's listener list
>changes for succeeding threads.
>
>Jacob
>
>
>Jayashri Visvanathan <Jayashri.Visvanathan_at_Sun.COM> wrote on 02/03/2005,
>06:34:52 PM:
>
>
>>M src/com/sun/faces/lifecycle/LifecycleImpl.java
>>Fix for bug 6223295. Get a pointer to 'listeners' so that
>>we still have reference to the original list for the current
>>thread. As a result, any listener added would not show up
>>until the NEXT phase but we want to avoid the lengthy
>>synchronization block. Due to this, "listeners" should be
>>modified only via add/remove methods and must never be updated directly.
>>
>>
>
>---------------------------------------------------------------------
>To unsubscribe, e-mail: dev-unsubscribe_at_javaserverfaces.dev.java.net
>For additional commands, e-mail: dev-help_at_javaserverfaces.dev.java.net
>
>
>
Index: src/com/sun/faces/lifecycle/LifecycleImpl.java
===================================================================
RCS file: /cvs/javaserverfaces-sources/jsf-ri/src/com/sun/faces/lifecycle/LifecycleImpl.java,v
retrieving revision 1.45.28.1
diff -u -r1.45.28.1 LifecycleImpl.java
--- src/com/sun/faces/lifecycle/LifecycleImpl.java 1 Feb 2005 22:43:53 -0000 1.45.28.1
+++ src/com/sun/faces/lifecycle/LifecycleImpl.java 3 Feb 2005 18:44:14 -0000
@@ -45,7 +45,7 @@
// The set of PhaseListeners registered with this Lifecycle instance
- private List listeners = new ArrayList();
+ private ArrayList listeners = new ArrayList();
// The set of Phase instances that are executed by the execute() method
@@ -133,7 +133,9 @@
+ "," + listener);
}
synchronized (listeners) {
- listeners.add(listener);
+ ArrayList temp = (ArrayList)this.listeners.clone();
+ temp.add(listener);
+ listeners = temp;
}
}
@@ -164,7 +166,9 @@
+ "," + listener);
}
synchronized (listeners) {
- listeners.remove(listener);
+ ArrayList temp = (ArrayList)this.listeners.clone();
+ temp.remove(listener);
+ listeners = temp;
}
}
@@ -184,23 +188,28 @@
int
i = 0,
maxBefore = 0;
-
+ List tempListeners = listeners;
try {
- // Notify the "beforePhase" method of interested listeners
+ // Notify the "beforePhase" method of interested listeners
// (ascending)
- synchronized (listeners) {
- if (listeners.size() > 0) {
- PhaseEvent event = new PhaseEvent(context, phaseId, this);
- for (i = 0; i < listeners.size(); i++) {
- PhaseListener listener = (PhaseListener) listeners.get(i);
- if (phaseId.equals(listener.getPhaseId()) ||
- PhaseId.ANY_PHASE.equals(listener.getPhaseId())) {
- listener.beforePhase(event);
- }
- maxBefore = i;
- }
- }
- }
+ // Fix for bug 6223295. Get a pointer to 'listeners' so that
+ // we still have reference to the original list for the current
+ // thread. As a result, any listener added would not show up
+ // until the NEXT phase but we want to avoid the lengthy
+ // synchronization block. Due to this, "listeners" should be
+ // modified only via add/remove methods and must never be updated
+ // directly.
+ if (tempListeners.size() > 0) {
+ PhaseEvent event = new PhaseEvent(context, phaseId, this);
+ for (i = 0; i < tempListeners.size(); i++) {
+ PhaseListener listener = (PhaseListener)tempListeners.get(i);
+ if (phaseId.equals(listener.getPhaseId()) ||
+ PhaseId.ANY_PHASE.equals(listener.getPhaseId())) {
+ listener.beforePhase(event);
+ }
+ maxBefore = i;
+ }
+ }
}
catch (Throwable e) {
if (log.isTraceEnabled()) {
@@ -220,18 +229,17 @@
try {
// Notify the "afterPhase" method of interested listeners
// (descending)
- synchronized (listeners) {
- if (listeners.size() > 0) {
- PhaseEvent event = new PhaseEvent(context, phaseId, this);
- for (i = maxBefore; i >= 0; i--) {
- PhaseListener listener = (PhaseListener) listeners.get(i);
- if (phaseId.equals(listener.getPhaseId()) ||
- PhaseId.ANY_PHASE.equals(listener.getPhaseId())) {
- listener.afterPhase(event);
- }
- }
- }
- }
+ if (tempListeners.size() > 0) {
+ PhaseEvent event = new PhaseEvent(context, phaseId, this);
+ for (i = maxBefore; i >= 0; i--) {
+ PhaseListener listener = (PhaseListener)
+ tempListeners.get(i);
+ if (phaseId.equals(listener.getPhaseId()) ||
+ PhaseId.ANY_PHASE.equals(listener.getPhaseId())) {
+ listener.afterPhase(event);
+ }
+ }
+ }
}
catch (Throwable e) {
if (log.isTraceEnabled()) {