Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.0.9, 2.0.13, 2.1.3, 2.1.7
    • Component/s: General
    • Labels:
      None
    • Environment:
      myface core trunk

      Description

      UIViewRoot:
      try

      { source.broadcast(event); }

      catch (AbortProcessingException e)

      { ExceptionQueuedEventContext exceptionContext = new ExceptionQueuedEventContext(context, e, source, context.getCurrentPhaseId()); context.getApplication().publishEvent(context, ExceptionQueuedEvent.class, exceptionContext); // Abortion return false; }

      Problem 1:
      <h:inputText valueChangeListener="#

      {bean.processValueChange}

      ">

      MethodExpressionValueChangeListener wraps all exceptions to AbortProcessingException and therefore exception is queued

      Problem 2:
      <h:inputText >
      <f:valueChangeListener binding="#

      {bean}

      " />
      </h:inputText>
      ValueChangeListenerHandler does not wrap exception to AbortProcessingException and therefore exception is not queued in this block (but it is queued from phase executor but without component info)

      Problem 3: JSF spec 2.1 :
      "Clarification made: throwing an AbortProcessingException tells an implementation that no further broadcast of the
      current event occurs. Does not affect future events."
      But I think that code in UIViewRoot makes opposite: // Abortion return false;

      1. MYFACES-3199-v2.patch
        5 kB
        Martin Kočí
      2. UIViewRoot.patch
        4 kB
        Martin Kočí

        Issue Links

          Activity

          Hide
          Leonardo Uribe added a comment -

          found small error on review, when AbortProcessingException is queued, event processing should continue for the same phase (remove return true when APE is found and let it return correctly at the end of the method)

          Show
          Leonardo Uribe added a comment - found small error on review, when AbortProcessingException is queued, event processing should continue for the same phase (remove return true when APE is found and let it return correctly at the end of the method)
          Hide
          Leonardo Uribe added a comment -

          Yes, sounds reasonable unify in this part, so all APE cases can be handled as expected in custom ExceptionHandler implementations. I updated the code to include the changes proposed on the second patch. Thanks for the fix.

          Show
          Leonardo Uribe added a comment - Yes, sounds reasonable unify in this part, so all APE cases can be handled as expected in custom ExceptionHandler implementations. I updated the code to include the changes proposed on the second patch. Thanks for the fix.
          Hide
          Martin Kočí added a comment -

          I forgot to mention the new version of patch in my previous comment: MYFACES-3199-v2.patch. That version queues extracted APE (not the wrapped one) because many custom Exception Handlers do only simple instanceof check. One example is org.apache.myfaces.context.MyFacesExceptionHandlerWrapperImpl.shouldSkip(Throwable)
          Patch v2 also modifies corresponding test case.

          Show
          Martin Kočí added a comment - I forgot to mention the new version of patch in my previous comment: MYFACES-3199 -v2.patch. That version queues extracted APE (not the wrapped one) because many custom Exception Handlers do only simple instanceof check. One example is org.apache.myfaces.context.MyFacesExceptionHandlerWrapperImpl.shouldSkip(Throwable) Patch v2 also modifies corresponding test case.
          Hide
          Leonardo Uribe added a comment -

          I see. I can't remember one example, but I suppose a custom implementation of javax.faces.view.facelets.MetaRule can inject that. So there is justification for that check.

          Show
          Leonardo Uribe added a comment - I see. I can't remember one example, but I suppose a custom implementation of javax.faces.view.facelets.MetaRule can inject that. So there is justification for that check.
          Hide
          Martin Kočí added a comment -

          I added the APE extraction in UIViewRoot because of custom MethodExpressionXYZListener, for example MethodExpressionSortListener or MethodExpressionReturnListener

          Show
          Martin Kočí added a comment - I added the APE extraction in UIViewRoot because of custom MethodExpressionXYZListener, for example MethodExpressionSortListener or MethodExpressionReturnListener
          Hide
          Leonardo Uribe added a comment -

          Ok, now I get it.

          MK>> Problem1 : UIViewRoot._broadcastAll queues APE into ExceptionQueuedEvent and
          MK>> check for "any of the causes of the exception are an APE" is missing in code

          The check is done in the wrappers. See javax.faces.event.MethodExpressionActionListener, javax.faces.event.MethodExpressionValueChangeListener. I think an additional check on UIViewRoot is not necessary.

          MK>> ad Problem 2) queuing the source component is required to solve the mail goal
          MK>> of MYFACES-3053: "any time there is an error in any part of the
          MK>> lifecycle, the user should see not just a cryptic stack trace, but also the
          MK>> *component* - including file and line number - that triggered the problem"

          The patch has some side effects. UIViewRoot._process(FacesContext context, PhaseId phaseId, PhaseProcessor processor) as a try {} catch block for RuntimeExceptions, to execute afterPhase listeners and then rethrow the exception. Later, on LifecycleImpl the exception is catched and finally published as an EventQueuedException.

          In few words, the change proposed here is just publish EventQueuedException for non APE exceptions in UIViewRoot._broadcastAll too, but preserve the effect of stop broadcasting when a non APE exception is received.

          If you provide a patch removing the check for nested APE, and if no objections I'll review it and commit it.

          Show
          Leonardo Uribe added a comment - Ok, now I get it. MK>> Problem1 : UIViewRoot._broadcastAll queues APE into ExceptionQueuedEvent and MK>> check for "any of the causes of the exception are an APE" is missing in code The check is done in the wrappers. See javax.faces.event.MethodExpressionActionListener, javax.faces.event.MethodExpressionValueChangeListener. I think an additional check on UIViewRoot is not necessary. MK>> ad Problem 2) queuing the source component is required to solve the mail goal MK>> of MYFACES-3053 : "any time there is an error in any part of the MK>> lifecycle, the user should see not just a cryptic stack trace, but also the MK>> * component * - including file and line number - that triggered the problem" The patch has some side effects. UIViewRoot._process(FacesContext context, PhaseId phaseId, PhaseProcessor processor) as a try {} catch block for RuntimeExceptions, to execute afterPhase listeners and then rethrow the exception. Later, on LifecycleImpl the exception is catched and finally published as an EventQueuedException. In few words, the change proposed here is just publish EventQueuedException for non APE exceptions in UIViewRoot._broadcastAll too, but preserve the effect of stop broadcasting when a non APE exception is received. If you provide a patch removing the check for nested APE, and if no objections I'll review it and commit it.
          Hide
          Martin Kočí added a comment -

          a example:
          <h:commandButton value="ok" action="#

          {testBean.actionNPE}

          " />

          <h:commandButton value="ok" actionListener="#

          {testBean.actionListenerNPE}

          " />

          <h:commandButton value="ok">
          <f:actionListener binding="#

          {actionListenerNPE}

          " />
          </h:commandButton>

          in all three cases returns exceptionQueuedEventContext.getComponent null in ExceptionHandler
          For action is exception wrapped into FacesException with message: "Error calling action method of component with id " + clientId (ActionListenerImpl, line 120)
          For actionListeners are exception wrapped into FacesException with message: "Exception while calling broadcast on component :" + getComponentLocation (UIComponentBase line 430)

          Goal:
          1) get rid of FacesException wrapping with different messages and
          2) queue source component where problem occurred

          this issue (MYFACES-3199) should provide 2) - source component available within exceptionQueuedEventContext.getComponent() - allows clients to format and display error in unified manner

          Show
          Martin Kočí added a comment - a example: <h:commandButton value="ok" action="# {testBean.actionNPE} " /> <h:commandButton value="ok" actionListener="# {testBean.actionListenerNPE} " /> <h:commandButton value="ok"> <f:actionListener binding="# {actionListenerNPE} " /> </h:commandButton> in all three cases returns exceptionQueuedEventContext.getComponent null in ExceptionHandler For action is exception wrapped into FacesException with message: "Error calling action method of component with id " + clientId (ActionListenerImpl, line 120) For actionListeners are exception wrapped into FacesException with message: "Exception while calling broadcast on component :" + getComponentLocation (UIComponentBase line 430) Goal: 1) get rid of FacesException wrapping with different messages and 2) queue source component where problem occurred this issue ( MYFACES-3199 ) should provide 2) - source component available within exceptionQueuedEventContext.getComponent() - allows clients to format and display error in unified manner
          Hide
          Martin Kočí added a comment -

          concrete example: every queued exceptionEvent should provide not null component form exceptionQueuedEventContext.getComponent if exception occured in component-related place.

          The main goal of whole MYFACES-3053 is to unify error handling , so f:actionListener and actionListener and their consistent behaviour are part of the goal.

          Please see attached UIViewRoot.patch if it breaks something - the purpose is queue component with ExceptionQueuedEventContext and thats all.

          Show
          Martin Kočí added a comment - concrete example: every queued exceptionEvent should provide not null component form exceptionQueuedEventContext.getComponent if exception occured in component-related place. The main goal of whole MYFACES-3053 is to unify error handling , so f:actionListener and actionListener and their consistent behaviour are part of the goal. Please see attached UIViewRoot.patch if it breaks something - the purpose is queue component with ExceptionQueuedEventContext and thats all.
          Hide
          Leonardo Uribe added a comment -

          I think this should be discussed on the Expert Group. The changes done in myfaces is just an interpretation of the spec, in order to preserve consistency. For example, if an ActionListener is added using f:actionListener or using <h:commandXXX actionListener="..."/>, the same behavior should be seen by the user and so on.

          It is very difficult to analyze this problem without concrete examples demostrating why the code should be changed. I know there are parts that can be improved but the changes we can do are limited with the current descriptions on the spec. If no objections I'll close again this issue, because no further advance can be done from MyFaces side.

          Show
          Leonardo Uribe added a comment - I think this should be discussed on the Expert Group. The changes done in myfaces is just an interpretation of the spec, in order to preserve consistency. For example, if an ActionListener is added using f:actionListener or using <h:commandXXX actionListener="..."/>, the same behavior should be seen by the user and so on. It is very difficult to analyze this problem without concrete examples demostrating why the code should be changed. I know there are parts that can be improved but the changes we can do are limited with the current descriptions on the spec. If no objections I'll close again this issue, because no further advance can be done from MyFaces side.
          Hide
          Martin Kočí added a comment -

          to publish or not to publish APE:

          APE is used to control code flow in similar way like ValidatorException or ConvertException: if VE or CE occurs, something (update model) is aborted. In case of APE is aborted delivery of current event to remaining listeners. Because VE and CE are not published -> don't publish APE too

          But there is a small difference: VE and CE are "converted" to FacesMessage instances and displayed to user - it means there is a feedback "somethings happened - conversion/validation was not successful for reason XY". In case of APE is no suitable place to indicate for user "this action was aborted for reason XY" - this speaks for publishing: a user (of myfaces) can create a facesMessage there.

          Show
          Martin Kočí added a comment - to publish or not to publish APE: APE is used to control code flow in similar way like ValidatorException or ConvertException: if VE or CE occurs, something (update model) is aborted. In case of APE is aborted delivery of current event to remaining listeners. Because VE and CE are not published -> don't publish APE too But there is a small difference: VE and CE are "converted" to FacesMessage instances and displayed to user - it means there is a feedback "somethings happened - conversion/validation was not successful for reason XY". In case of APE is no suitable place to indicate for user "this action was aborted for reason XY" - this speaks for publishing: a user (of myfaces) can create a facesMessage there.
          Hide
          Martin Kočí added a comment -

          >1) execute one listener in try catch block and catch (Exception e)
          > 2) if
          > 2a) exception is instance of APE or any of the causes of the exception are an APE, DON'T publish ExceptionQueuedEvent > and terminate processing for current event _

          Problem1 : UIViewRoot._broadcastAll queues APE into ExceptionQueuedEvent and check for "any of the causes of the exception are an APE" is missing in code

          > 2b) for any other exception publish ExceptionQueuedEvent
          Problem2 : This is done partially : in LifecycleImpl.publishException but without source component = the most important type here - the component - is missing

          ad Problem 1) : publishing APE into event is not a big problem; I cannot find in spec if this is required. Only Application.publishEvent(FacesContext, Class<? extends SystemEvent>, Object) says: the (APE) exception must be logged with Level.SEVERE " -> not to queue

          ad Problem 2) queuing the source component is required to solve the mail goal of MYFACES-3053: "any time there is an error in any part of the lifecycle, the user should see not just a cryptic stack trace, but also the *component* - including file and line number - that triggered the problem"

          I'll reopen this issues to discuss the problems.

          Show
          Martin Kočí added a comment - >1) execute one listener in try catch block and catch (Exception e) > 2) if > 2a) exception is instance of APE or any of the causes of the exception are an APE, DON'T publish ExceptionQueuedEvent > and terminate processing for current event _ Problem1 : UIViewRoot._broadcastAll queues APE into ExceptionQueuedEvent and check for "any of the causes of the exception are an APE" is missing in code > 2b) for any other exception publish ExceptionQueuedEvent Problem2 : This is done partially : in LifecycleImpl.publishException but without source component = the most important type here - the component - is missing ad Problem 1) : publishing APE into event is not a big problem; I cannot find in spec if this is required. Only Application.publishEvent(FacesContext, Class<? extends SystemEvent>, Object) says: the (APE) exception must be logged with Level.SEVERE " -> not to queue ad Problem 2) queuing the source component is required to solve the mail goal of MYFACES-3053 : "any time there is an error in any part of the lifecycle, the user should see not just a cryptic stack trace, but also the * component * - including file and line number - that triggered the problem" I'll reopen this issues to discuss the problems.
          Hide
          Leonardo Uribe added a comment -

          With the analysis done on MYFACES-3301, it becomes clear how to solve this problem.

          The first part we need to do is make clear the "context" behind the sentence:

          "... If that fails for any reason, throw an AbortProcessingException, including the cause of the failure. ..."

          After thinking a lot about it, we should not interpret it literally. Instead, the conclusion is the part is an advice for the developer implementing a listener in the method expressions that could be wrapped by this class. In other words, it is responsibility of the developer to throw AbortProcessingException if it "fails for any reason". That has a lot more sense than throw an APE for each exception without questions.

          With the previous problem understood, how can we solve the inconsistent behavior for problem 1 and 2? Just do what was already agreed but with a change:

          1) execute one listener in try catch block and catch (Exception e)
          2) if
          2a) exception is instance of APE or any of the causes of the exception are an APE, don't publish ExceptionQueuedEvent and terminate processing for current event (is this as such in the spec that an APE has to be queued?)
          2b) for any other exception publish ExceptionQueuedEvent [REMOVE THE FOLLOWING FRAGMENT --- and continue broadcast processing ---]
          3) handle queued exception in exception handler
          3a) default exception handler : ignore AbortProcessingException
          3b) custom exception handler: can handle all unexpected exception (for example remove them from queue)
          4) all unhandled exception are rethrow and error page is displayed

          We can't continue broadcast processing if other exception is thrown, because that requires a change in the spec, so we are stuck in this part.

          Anyway, the previous algorithm is consistent, it has sense and comply with the spec.

          Show
          Leonardo Uribe added a comment - With the analysis done on MYFACES-3301 , it becomes clear how to solve this problem. The first part we need to do is make clear the "context" behind the sentence: "... If that fails for any reason, throw an AbortProcessingException, including the cause of the failure. ..." After thinking a lot about it, we should not interpret it literally. Instead, the conclusion is the part is an advice for the developer implementing a listener in the method expressions that could be wrapped by this class. In other words, it is responsibility of the developer to throw AbortProcessingException if it "fails for any reason". That has a lot more sense than throw an APE for each exception without questions. With the previous problem understood, how can we solve the inconsistent behavior for problem 1 and 2? Just do what was already agreed but with a change: 1) execute one listener in try catch block and catch (Exception e) 2) if 2a) exception is instance of APE or any of the causes of the exception are an APE, don't publish ExceptionQueuedEvent and terminate processing for current event (is this as such in the spec that an APE has to be queued?) 2b) for any other exception publish ExceptionQueuedEvent [REMOVE THE FOLLOWING FRAGMENT --- and continue broadcast processing ---] 3) handle queued exception in exception handler 3a) default exception handler : ignore AbortProcessingException 3b) custom exception handler: can handle all unexpected exception (for example remove them from queue) 4) all unhandled exception are rethrow and error page is displayed We can't continue broadcast processing if other exception is thrown, because that requires a change in the spec, so we are stuck in this part. Anyway, the previous algorithm is consistent, it has sense and comply with the spec.
          Hide
          Leonardo Uribe added a comment -

          Committed solution for problem 3. To implement it correctly, a linked list was used to keep track aborted events, and remove them, so an APE only affects the related event and not other ones.

          Show
          Leonardo Uribe added a comment - Committed solution for problem 3. To implement it correctly, a linked list was used to keep track aborted events, and remove them, so an APE only affects the related event and not other ones.
          Hide
          Martin Kočí added a comment -

          > 2a) exception is instance of APE or any of the causes of the exception are an APE, don't publish
          > ExceptionQueuedEvent and terminate processing for current event (is this as such in the spec that an APE has to be queued?)

          Good question.

          A) from specification document "6.2.1 Default ExceptionHandler implementation" - " ... upon encountering the first such Exception that is not an instance of javax.faces.event.AbortProcessingException ... "

          B) from JavaDoc Application.publishEvent : "... invoking the processListener method causes an javax.faces.event.AbortProcessingException to be thrown, processing of the listeners must be aborted, no further processing of the listeners for this event must take place, and the exception must be logged with Level.SEVERE"

          And that's all - specification is pretty poor in this area.

          ad A) this part expects AbortProcessingException to be queued for exception handler: but it does not make sense: AbortProcessingException is exception for controlling code flow and user can use it in same way as ValidatorException and ConverterException - those exceptions are not published.

          ad B) "exception must be logged with Level.SEVERE" no mention of publish but still weird: again, I think AbortProcessingException is expected exception - why logging it as severe ( funny: specification speaks about java.util.logging API directly)

          So, I completely agree with improved 2a) "exception is instance of APE or any of the causes of the exception are an APE, DON'T publish ExceptionQueuedEvent and terminate processing for current event".

          We should do a logging improvement here; if APE terminates processing for current event, output a log "Processing of .. was terminated with APE ..." but surely log level according to project stage (not severe) - I'll create separate subtask of MYFACES-3053 for it.

          Show
          Martin Kočí added a comment - > 2a) exception is instance of APE or any of the causes of the exception are an APE, don't publish > ExceptionQueuedEvent and terminate processing for current event (is this as such in the spec that an APE has to be queued?) Good question. A) from specification document "6.2.1 Default ExceptionHandler implementation" - " ... upon encountering the first such Exception that is not an instance of javax.faces.event.AbortProcessingException ... " B) from JavaDoc Application.publishEvent : "... invoking the processListener method causes an javax.faces.event.AbortProcessingException to be thrown, processing of the listeners must be aborted, no further processing of the listeners for this event must take place, and the exception must be logged with Level.SEVERE" And that's all - specification is pretty poor in this area. ad A) this part expects AbortProcessingException to be queued for exception handler: but it does not make sense: AbortProcessingException is exception for controlling code flow and user can use it in same way as ValidatorException and ConverterException - those exceptions are not published. ad B) "exception must be logged with Level.SEVERE" no mention of publish but still weird: again, I think AbortProcessingException is expected exception - why logging it as severe ( funny: specification speaks about java.util.logging API directly) So, I completely agree with improved 2a) "exception is instance of APE or any of the causes of the exception are an APE, DON'T publish ExceptionQueuedEvent and terminate processing for current event". We should do a logging improvement here; if APE terminates processing for current event, output a log "Processing of .. was terminated with APE ..." but surely log level according to project stage (not severe) - I'll create separate subtask of MYFACES-3053 for it.
          Hide
          Martin Marinschek added a comment -

          Hi Martin,

          I totally agree with your point about listener isolation, however I see things a little differently (highlighting with _ added):

          1) execute one listener in try catch block and catch (Exception e)
          2) if
          2a) exception is instance of APE or any of the causes of the exception are an APE, don't publish ExceptionQueuedEvent and terminate processing for current event (is this as such in the spec that an APE has to be queued?)
          2b) for any other exception publish ExceptionQueuedEvent and continue broadcast processing
          3) handle queued exception in exception handler
          3a) default exception handler : ignore AbortProcessingException
          3b) custom exception handler: can handle all unexpected exception (for example remove them from queue)
          4) all unhandled exception are rethrow and error page is displayed

          Show
          Martin Marinschek added a comment - Hi Martin, I totally agree with your point about listener isolation, however I see things a little differently (highlighting with _ added): 1) execute one listener in try catch block and catch (Exception e) 2) if 2a) exception is instance of APE or any of the causes of the exception are an APE, don't publish ExceptionQueuedEvent and terminate processing for current event (is this as such in the spec that an APE has to be queued?) 2b) for any other exception publish ExceptionQueuedEvent and continue broadcast processing 3) handle queued exception in exception handler 3a) default exception handler : ignore AbortProcessingException 3b) custom exception handler: can handle all unexpected exception (for example remove them from queue) 4) all unhandled exception are rethrow and error page is displayed
          Hide
          Martin Kočí added a comment -

          see MYFACES-3201 too: if we accept that listener execution is isolated from other listeners, then we should implement is as follows:
          1) execute one listener in try catch block and catch (Exception e)
          2) if
          2a) exception is instance of APE, publish ExceptionQueuedEvent and terminate processing for current event
          2b) for any other exception publish ExceptionQueuedEvent and continue broadcast processing
          3) handle queued exception in exception handler
          3a) default exception handler : ignore AbortProcessingException
          3b) custom exception handler: can handle all unexpected exception (for example remove them from queue)
          4) all unhandled exception are rethrow and error page is displayed

          Show
          Martin Kočí added a comment - see MYFACES-3201 too: if we accept that listener execution is isolated from other listeners, then we should implement is as follows: 1) execute one listener in try catch block and catch (Exception e) 2) if 2a) exception is instance of APE, publish ExceptionQueuedEvent and terminate processing for current event 2b) for any other exception publish ExceptionQueuedEvent and continue broadcast processing 3) handle queued exception in exception handler 3a) default exception handler : ignore AbortProcessingException 3b) custom exception handler: can handle all unexpected exception (for example remove them from queue) 4) all unhandled exception are rethrow and error page is displayed
          Hide
          Martin Kočí added a comment -

          Classes with AbortProcessingException usage:

          1. javax.faces.application.Application.publishEvent(FacesContext, Class<? extends SystemEvent>, Object)
          2. javax.faces.event.MethodExpressionActionListener.processAction(ActionEvent)
          3. javax.faces.event.MethodExpressionValueChangeListener
          4. UIComponentBase.broadcast(FacesEvent)
          5. UIInput.broadcast(FacesEvent)
          6. org.apache.myfaces.view.facelets.tag.jsf.core.ActionListenerHandler.LazyActionListener.processAction(ActionEvent)
          7. org.apache.myfaces.application.ActionListenerImpl.processAction(ActionEvent)
          8. org.apache.myfaces.application.ApplicationImpl.publishEvent(FacesContext, Class<? extends SystemEvent>, Class<?>, Object)
          9. org.apache.myfaces.view.facelets.tag.jsf.core.PhaseListenerHandler.LazyPhaseListener.getInstance()
          10. org.apache.myfaces.taglib.core.PhaseListenerTag.BindingPhaseListener.getPhaseListnerInstance(ValueExpression, ValueExpression)
          11. org.apache.myfaces.view.facelets.tag.jsf.core.ValueChangeListenerHandler.LazyValueChangeListener.processValueChange(ValueChangeEvent)

          rethrowing in code is inconsistent, catch block sometimes contains:
          catch (Exception e) (or catch (FacesException ex) or catch (ELException e))
          throw new AbortProcessingException("....", e);

          Show
          Martin Kočí added a comment - Classes with AbortProcessingException usage: javax.faces.application.Application.publishEvent(FacesContext, Class<? extends SystemEvent>, Object) javax.faces.event.MethodExpressionActionListener.processAction(ActionEvent) javax.faces.event.MethodExpressionValueChangeListener UIComponentBase.broadcast(FacesEvent) UIInput.broadcast(FacesEvent) org.apache.myfaces.view.facelets.tag.jsf.core.ActionListenerHandler.LazyActionListener.processAction(ActionEvent) org.apache.myfaces.application.ActionListenerImpl.processAction(ActionEvent) org.apache.myfaces.application.ApplicationImpl.publishEvent(FacesContext, Class<? extends SystemEvent>, Class<?>, Object) org.apache.myfaces.view.facelets.tag.jsf.core.PhaseListenerHandler.LazyPhaseListener.getInstance() org.apache.myfaces.taglib.core.PhaseListenerTag.BindingPhaseListener.getPhaseListnerInstance(ValueExpression, ValueExpression) org.apache.myfaces.view.facelets.tag.jsf.core.ValueChangeListenerHandler.LazyValueChangeListener.processValueChange(ValueChangeEvent) rethrowing in code is inconsistent, catch block sometimes contains: catch (Exception e) (or catch (FacesException ex) or catch (ELException e)) throw new AbortProcessingException("....", e);
          Hide
          Martin Kočí added a comment -

          What is exactly the purpose of AbortProcessingException compared to normal RuntimeException?

          From JavaDoc: AbortProcessingException "An exception that may be thrown by event listeners to terminate the processing of the current event."

          JSF 2.1 spec contains also clarification:

          3.4.2.6 Event Broadcasting - listener processing an event may ... Throw an AbortProcessingException, to tell the JSF implementation that no further broadcast of this event should take place."
          and "Clarification made: throwing an AbortProcessingException tells an implementation that no further broadcast of the
          current event occurs. Does not affect future events."

          My initial thought was use this in following case:
          <h:commandButton>
          <f:actionListener ...>
          <f:actionListener ...> <!-- This one throws AbortProcessingExpcetion -->
          <f:actionListener ...>
          </h:commandButton>

          Author of view knows that third one listener cannot work if second one fail and therefore throws AbortProcessingException explicitly.
          But this behaviour is same for unexpected exception form listener - it terminates execution of listeners. The only difference is that default ExceptionHandler ignores AbortProcessingException in stack

          Wasn't the original purpose of AbortProcessingException and ExceptionHandler to achieve this:

          <h:commandButton>
          <f:actionListener ...> <!-- may throw a exception - if it does it will be queued for exception handling; but broadcasting continues -->
          <f:actionListener ...> <!-- This one throws AbortProcessingExpcetion -->
          <f:actionListener ...> <!-- not executed if previous AbortProcessingExpcetion; but independent from first one -->
          </h:commandButton>

          That is a classic isolation of listeners in observer pattern.

          Show
          Martin Kočí added a comment - What is exactly the purpose of AbortProcessingException compared to normal RuntimeException? From JavaDoc: AbortProcessingException "An exception that may be thrown by event listeners to terminate the processing of the current event." JSF 2.1 spec contains also clarification: 3.4.2.6 Event Broadcasting - listener processing an event may ... Throw an AbortProcessingException, to tell the JSF implementation that no further broadcast of this event should take place." and "Clarification made: throwing an AbortProcessingException tells an implementation that no further broadcast of the current event occurs. Does not affect future events." My initial thought was use this in following case: <h:commandButton> <f:actionListener ...> <f:actionListener ...> <!-- This one throws AbortProcessingExpcetion --> <f:actionListener ...> </h:commandButton> Author of view knows that third one listener cannot work if second one fail and therefore throws AbortProcessingException explicitly. But this behaviour is same for unexpected exception form listener - it terminates execution of listeners. The only difference is that default ExceptionHandler ignores AbortProcessingException in stack Wasn't the original purpose of AbortProcessingException and ExceptionHandler to achieve this: <h:commandButton> <f:actionListener ...> <!-- may throw a exception - if it does it will be queued for exception handling; but broadcasting continues --> <f:actionListener ...> <!-- This one throws AbortProcessingExpcetion --> <f:actionListener ...> <!-- not executed if previous AbortProcessingExpcetion; but independent from first one --> </h:commandButton> That is a classic isolation of listeners in observer pattern.
          Hide
          Leonardo Uribe added a comment -

          Problem 1: The spec javadoc is clear about this. See MethodExpressionValueChangeListener.processValueChange :

          "... If that fails for any reason, throw an AbortProcessingException, including the cause of the failure. ..."

          Problem 2: Possibily this is a bug, but first we need to check it against the EG.

          Problem 3: It is a bug, so I'll committed a solution for it.

          Show
          Leonardo Uribe added a comment - Problem 1: The spec javadoc is clear about this. See MethodExpressionValueChangeListener.processValueChange : "... If that fails for any reason, throw an AbortProcessingException, including the cause of the failure. ..." Problem 2: Possibily this is a bug, but first we need to check it against the EG. Problem 3: It is a bug, so I'll committed a solution for it.

            People

            • Assignee:
              Leonardo Uribe
              Reporter:
              Martin Kočí
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development