It will add a bit more overhead in common case of executing the phase
vs. modification of the listener list; but the change does simplify the
synchronization logic a bit.
+1
Jacob
Jayashri Visvanathan <Jayashri.Visvanathan_at_Sun.COM> wrote on 02/03/2005,
08:17:11 PM:
> Ed suggested cloning the "listeners" in the phase() method instead of
> cloning the add/remove method. That way, we also avoid cloning twice.
> Here is the modified change bundle. Please review.
> Thanks
> -Jayashri
>
> Jayashri Visvanathan wrote:
>
> > 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 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()) {
> >
> >
> >
> >------------------------------------------------------------------------
> >
> >---------------------------------------------------------------------
> >To unsubscribe, e-mail: dev-unsubscribe_at_javaserverfaces.dev.java.net
> >For additional commands, e-mail: dev-help_at_javaserverfaces.dev.java.net
> >
> >