Issue Details (XML | Word | Printable)

Key: JAVASERVERFACES_SPEC_PUBLIC-9
Type: Bug Bug
Status: Resolved Resolved
Resolution: Fixed
Priority: Critical Critical
Assignee: Ed Burns
Reporter: rogerk
Votes: 0
Watchers: 0
Operations

If you were logged in you would be able to see more operations.
javaserverfaces-spec-public

Phase Listener "afterPhase" Call in Finally Block

Created: 24/Aug/04 05:46 AM   Updated: 04/Mar/10 02:09 PM   Resolved: 04/Mar/10 02:09 PM
Component/s: Uncategorized
Affects Version/s: 1.2
Fix Version/s: 1.2

Time Tracking:
Not Specified

Environment:

Operating System: All
Platform: All


Issuezilla Id: 9
Tags:
Participants: Ed Burns, jhook, rogerk and Ryan Lubke


 Description  « Hide

For phase listeners, possibly modify the phase processing to allow the
afterPhase method to be called in a finally block. If an error occurs in one of
the Phases, the afterPhase would never be called. This may prevent resource
cleanup and cause memory leaks in developer implementations.

Additional comments from adamwiner Thu Aug 12 20:18:13 +0000 2004 -------

This behavior should be specified in the 1.2 spec. I don't think anything
prevents it from being implemented in 1.1, but the EG should be queried.



Ed Burns added a comment - 30/Aug/04 11:17 AM

up to p2


Ed Burns added a comment - 07/Oct/04 11:47 AM

take ownership


Ed Burns added a comment - 19/Oct/04 11:52 AM

started EG discussion.


Ryan Lubke added a comment - 22/Oct/04 06:11 AM

Section 11.3 currently states that if beforePhase() is called on a particular
PhaseListener instance, the JSF implementation must guarantee that afterPhase()
is called.

The RI however, doesn't currently do this:

c.s.f.lifecycle.LifecycleImpl.java
--------------------------------------------------------

// Notify the "beforePhase" method of interested listeners (ascending)
synchronized (listeners) {
if (listeners.size() > 0) {
PhaseEvent event = new PhaseEvent(context, phaseId, this);
for (int i = 0; i < listeners.size(); i++) {
PhaseListener listener = (PhaseListener) listeners.get;
if (phaseId.equals(listener.getPhaseId()) ||
PhaseId.ANY_PHASE.equals(listener.getPhaseId())) { listener.beforePhase(event); }
}
}
}

// Execute this phase itself (if still needed)
if (!skipping(phaseId, context)) { phase.execute(context); }

// Notify the "afterPhase" method of interested listeners (descending)
synchronized (listeners) {
if (listeners.size() > 0) {
PhaseEvent event = new PhaseEvent(context, phaseId, this);
for (int i = listeners.size() - 1; i >= 0; i--) {
PhaseListener listener = (PhaseListener) listeners.get;
if (phaseId.equals(listener.getPhaseId()) ||
PhaseId.ANY_PHASE.equals(listener.getPhaseId())) { listener.afterPhase(event); }
}
}
}

--------------------------------------------------------


Ed Burns added a comment - 25/Oct/04 02:35 PM

Issue: jsf-spec9

Implement section 11.3 of the spec to ensure that if a beforePhase
listener is called, then the afterPhase listener is called, regardless
of what happens in the interim.

Automated test forthcoming.

M jsf-ri/src/com/sun/faces/lifecycle/LifecycleImpl.java

SECTION: RI Changes

  • modify the phase() method to put a try-catch block around the
    beforePhase calls, keeping track of the exception status of the
    listeners. Put the phase.execute() inside of a try{} block, and the
    afterPhase calls inside of a finally{} block, making sure to only
    execute the listeners that successfully received the beforePhase
    call().

M jsf-ri/src/com/sun/faces/util/Util.java

  • added getStackTraceString() method:
  • <p>Leverage the Throwable.getStackTrace() method to produce a
  • String version of the stack trace, with a "\n" before each
  • line.</p>
    *
  • @return the String representation ofthe stack trace obtained by
  • calling getStackTrace() on the passed in exception. If null is
  • passed in, we return the empty String.

SECTION: RI Diffs

Index: jsf-ri/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
diff -u -r1.45 LifecycleImpl.java
— jsf-ri/src/com/sun/faces/lifecycle/LifecycleImpl.java 31 Mar 2004 18:48:30
-0000 1.45
+++ jsf-ri/src/com/sun/faces/lifecycle/LifecycleImpl.java 25 Oct 2004 21:30:05 -0000
@@ -181,38 +181,57 @@
log.trace("phase(" + phaseId.toString() + "," + context + ")");
}

  • // Notify the "beforePhase" method of interested listeners (ascending)
  • synchronized (listeners) {
  • if (listeners.size() > 0) {
  • PhaseEvent event = new PhaseEvent(context, phaseId, this);
  • for (int i = 0; i < listeners.size(); i++) {
  • PhaseListener listener = (PhaseListener) listeners.get;
  • if (phaseId.equals(listener.getPhaseId()) ||
  • PhaseId.ANY_PHASE.equals(listener.getPhaseId())) { - listener.beforePhase(event); - }
  • }
  • }
  • }
    + int
    + i = 0,
    + maxBefore = 0;
  • // Execute this phase itself (if still needed)
  • if (!skipping(phaseId, context)) { - phase.execute(context); - }
    -
  • // Notify the "afterPhase" method of interested listeners (descending)
  • synchronized (listeners) {
  • if (listeners.size() > 0) {
  • PhaseEvent event = new PhaseEvent(context, phaseId, this);
  • for (int i = listeners.size() - 1; i >= 0; i--) {
  • PhaseListener listener = (PhaseListener) listeners.get;
  • if (phaseId.equals(listener.getPhaseId()) ||
  • PhaseId.ANY_PHASE.equals(listener.getPhaseId())) { - listener.afterPhase(event); - }
  • }
  • }
  • }
    + 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++)
    Unknown macro: {+ PhaseListener listener = (PhaseListener) listeners.get(i);+ if (phaseId.equals(listener.getPhaseId()) ||+ PhaseId.ANY_PHASE.equals(listener.getPhaseId())) { + listener.beforePhase(event); + }+ maxBefore = i;+ }

    + }
    + }
    + }
    + catch (Throwable e)
    Unknown macro: {+ if (log.isTraceEnabled()) { + log.trace("phase(" + phaseId.toString() + "," + context + + ") threw exception: " + e + " " + e.getMessage() + + "\n" + Util.getStackTraceString(e)); + }+ }

    +
    + try
    Unknown macro: { + // Execute this phase itself (if still needed)+ if (!skipping(phaseId, context)) { + phase.execute(context); + }+ }

    + finally {
    + // 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--)
    Unknown macro: {+ PhaseListener listener = (PhaseListener) listeners.get(i);+ if (phaseId.equals(listener.getPhaseId()) ||+ PhaseId.ANY_PHASE.equals(listener.getPhaseId())) { + listener.afterPhase(event); + }+ }

    + }
    + }
    + }

}

Index: jsf-ri/src/com/sun/faces/util/Util.java
===================================================================
RCS file: /cvs/javaserverfaces-sources/jsf-ri/src/com/sun/faces/util/Util.java,v
retrieving revision 1.144
diff -u -r1.144 Util.java
— jsf-ri/src/com/sun/faces/util/Util.java 12 Oct 2004 14:39:55 -0000 1.144
+++ jsf-ri/src/com/sun/faces/util/Util.java 25 Oct 2004 21:30:06 -0000
@@ -1455,6 +1455,19 @@
return segmentIndex;
}

+ public static String getStackTraceString(Throwable e) {
+ if (null == e) { + return ""; + }
+
+ StackTraceElement stacks[] = e.getStackTrace();
+ StringBuffer sb = new StringBuffer();
+ for (int i = 0; i < stacks.length; i++) { + sb.append(stacks[i].toString() + "\n"); + }
+ return sb.toString();
+ }
+

} // end of class Util


jhook added a comment - 26/Oct/04 07:44 AM

2 Things:
If we are dealing with the listener pattern, is it appropriate to allow a
listener to stop execution to other listeners? (maxBefore ?)

Also, I'm wondering if synchornization blocks around the listeners are required?
If a phase listener takes 2 seconds to process under some condition, it will
have the whole application locked for 2 seconds for everyone. I might need to
revist my Java in a Nutshell book...

The result of the changes below would allow phases to be added, but would not
show up until the NEXT phase. That might be a negative, but it would help avoid
possibly lengthy synchronization blocks.

public void addPhaseListener(PhaseListener listener) {
// .......
synchronized (listeners) { ArrayList temp = (ArrayList)listeners.clone(); temp.add(listener); listeners = temp; }
// .......
}

public void removePhaseListener(PhaseListener listener) {
// .......
synchronized (listeners) { ArrayList temp = (ArrayList) listeners.clone(); temp.remove(listener); listeners = temp; }
// .......
}

When you actually execute, just get yourself a pointer to 'listeners' so if the
pointer changes for 'listeners', we still have reference to the original list
for this threads execution.

// grab our reference to the current list in memory
List ourListeners = listeners;

// Notify the "beforePhase" method of listeners
if (ourListeners.size() > 0) {
// declare references BEFORE loops for speed
PhaseListener listener = null;
PhaseId listenerPhaseId = null;
PhaseEvent event = new PhaseEvent(context, phaseId, this);

for (i = 0; i < ourListeners.size(); i++) {
listener = (PhaseListener) ourListeners.get;
listenerPhaseId = listener.getPhaseId();
if (phaseId.equals(listenerPhaseId) ||
PhaseId.ANY_PHASE.equals(listenerPhaseId)) {
try { listener.beforePhase(event); }
catch (Throwable e) { // do logging }
}
}
}

try {
if (!skipping(phaseId, context) { phase.execute(context); }
}
finally {
if (ourListeners.size() > 0) {
// declare references BEFORE loops for speed
PhaseListener listener = null;
PhaseId listenerPhaseId = null;
PhaseEvent event = new PhaseEvent(context, phaseId, this);

for (i = 0; i < ourListeners.size(); i++) {
listener = (PhaseListener) ourListeners.get;
listenerPhaseId = listener.getPhaseId();
if (phaseId.equals(listenerPhaseId) ||
PhaseId.ANY_PHASE.equals(listenerPhaseId)) {
try { listener.afterPhase(event); }
catch (Throwable e) { // do logging }
}
}
}
}


Ed Burns added a comment - 27/Oct/04 09:46 AM

Hello Jacob,

Thanks for your review. As Ryan pointed out, the spec says that if
beforePhase() is called on a particular PhaseListener instance, the JSF
implementation must guarantee that afterPhase() is called. The
maxBefore int verifies that this requirement is met strictly. If the
beforePhase listener doesn't get called, then the afterPhase listener
doesn't get called either.

Regarding threading, you have a point, but it's an implementation point,
not a spec one. I've filed
<https://javaserverfaces.dev.java.net/issues/show_bug.cgi?id=66> for
this. Once I've checked in issue 9, can you produce a change-bundle off
the trunk that implements this?

Ed


Ed Burns added a comment - 09/Dec/04 08:24 AM

fixed in EDR


Ed Burns added a comment - 24/Nov/09 07:48 AM

Prepare to delete "spec" subcomponent.


Ed Burns added a comment - 04/Mar/10 02:09 PM

Move all to 1.2