dev@javaserverfaces.java.net

Review: Remove synchronized block in Lifecycle.Phase()

From: Jayashri Visvanathan <Jayashri.Visvanathan_at_Sun.COM>
Date: Thu, 03 Feb 2005 09:34:52 -0800

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.


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 17:33:39 -0000
@@ -184,23 +184,30 @@
         int
             i = 0,
             maxBefore = 0;
-
+ List tempListeners = listeners;
         try {
             // 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;
- }
- }
- }
+ // Notify the "beforePhase" method of interested listeners
+ // (ascending)
+ // 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 +227,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()) {