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 <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()) {
>
>
>
>------------------------------------------------------------------------
>
>---------------------------------------------------------------------
>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 19:14:00 -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
@@ -184,23 +184,28 @@
int
i = 0,
maxBefore = 0;
-
+ List tempListeners = listeners.clone();
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 +225,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()) {