Mule
  1. Mule
  2. MULE-4955

Refactor Exception Strategy invocation so we don't get exception strategies invoked twice

    Details

    • User impact:
      High
    • Migration Impact:
      People need to know that exceptions will only be handled by the listener closer to where the exception happens.
    • Similar Issues:
      MULE-6490Exception strategy triggered twice
      MULE-4924Exception strategy invoked twice on TransactionTemplate
      MULE-2313Exception Strategy logs error twice
      MULE-5808Mule not invoking exception strategy when an exception is thrown in asynchronous flow
      MULE-6249Custom Exception Strategy class not getting being invoked in a Jetty Inbound Endpoint
      MULE-4927Exception strategy invoked both for connector *and* service exception strategies when exception on sync outbound endpoint
      MULE-6140Custom Exception Strategy class not getting invoked
      MULE-6575There is no way to turn off logging in exception strategy
      MULE-4233Quartz connector ignores exception strategy
      MULE-5438default-exception-strategy within a flow doesn't work as expected

      Description

      There are several cases in which exceptions are being handled twice by exception listeners. I worked on MULE-4924 and MULE-4927, but the issue is more general. Issue EE-1968 shows this.

      For example:
      We have this when the exception occurs inside a transformer on the outbound endpoint and we are inside an xa transaction.

      In that use case these two calls are in the stack when the exception is thrown:

      • WebSphereMQMessageDispatcher(AbstractMessageDispatcher).send(MuleEvent) line: 165
        • Exception gets handled and rethrown here at:
          • WebSphereMQMessageDispatcher(AbstractMessageDispatcher).send(MuleEvent) line: 200
      • TransactionTemplate.execute(TransactionCallback) line: 114
        • Exception gets handled and not rethrown here:
          • TransactionTemplate.execute(TransactionCallback) line: 138

      The problem is that we cannot avoid the throwing of the exception inside the AbstractMessageDispatcher because we need that exception for the TransactionTemplate to know there was an exception (for example, to rollback a transaction).
      And in the TransactionTemplate we have no way of telling it the exception was handled or not.

      I think we need some marker on the Exceptions that would tell us when they have been handled.

      The proposed solution:

      The idea is to have some marker on the MuleException and MuleRuntimeException that would let us know if an exception was already handled or not. Probably by making both of them implement a new interface called something like MuleHandleStatusException that would have a isAlreadyHandled() and a setAlreadyHandled(boolean handled).

      We would also have a MuleExceptionHandlingUtil with a method that could be called boolean handledExceptionIfNeeded(ExceptionListener listener, Exception e) that would take care of detecting if somewhere in the causes of that exception there is a MuleHandleStatusException and check its status. It could return true if the exception is handled and false if it was already handled.

      We can then change all the lines similar to this:
      exceptionListener.exceptionThrown(e);
      to
      MuleExceptionHandlingUtil.handledExceptionIfNeeded(exceptionListener, e);

      Potential Problems:

      Some decisions made:

      • What should we do if we mark an exception as handled and later someone else in the stack encapsulates it in another exception?
        • We will only handle it once. It won't be rehandled even if encapsulated
      • What do we do with exceptions that are neither MuleException nor MuleRuntimeException (ie, that don't implement MuleHandleStatusException)?
        • This case should be very rare (I've seen a lot of places where we catch Exception and we encapsulate it with some MuleException or another). As we don't have a way of know if it was already handled or not, we will handle it everytime.

        Issue Links

          Activity

          Hide
          Edu Pereda added a comment - - edited

          I committed some changes for this bug (on 2.2.x):

          http://fisheye.codehaus.org/changelog/mule/?cs=18198

          The commit has changes for 2 issues:
          MULE-4955: Refactor Exception Strategy invocation so we don't get exception strategies invoked twice
          MULE-4927: Exception strategy invoked both for connector and service exception strategies when exception on sync outbound endpoint

          This fix reverts some of the changes from MULE-4927 because it solves the problem in a more general way.
          With this change the MuleExceptions and MuleRuntimeExceptions can be marked as handled. They both implement a new interface called MuleExceptionHandleStatus and there is an utility class called MuleExceptionHandlingUtil that takes care of checking if an exception was already handled or not.

          Show
          Edu Pereda added a comment - - edited I committed some changes for this bug (on 2.2.x): http://fisheye.codehaus.org/changelog/mule/?cs=18198 The commit has changes for 2 issues: MULE-4955 : Refactor Exception Strategy invocation so we don't get exception strategies invoked twice MULE-4927 : Exception strategy invoked both for connector and service exception strategies when exception on sync outbound endpoint This fix reverts some of the changes from MULE-4927 because it solves the problem in a more general way. With this change the MuleExceptions and MuleRuntimeExceptions can be marked as handled. They both implement a new interface called MuleExceptionHandleStatus and there is an utility class called MuleExceptionHandlingUtil that takes care of checking if an exception was already handled or not.
          Hide
          Edu Pereda added a comment -

          This issue is fixed for 2.2.x (it will be out on 2.2.6).

          I am not closing it until we merge it with 3.x

          Show
          Edu Pereda added a comment - This issue is fixed for 2.2.x (it will be out on 2.2.6). I am not closing it until we merge it with 3.x
          Hide
          Edu Pereda added a comment -

          Reopening because there are many exceptions that are not wrapped by neither a MuleException nor a MuleRuntimeException (and so, they are handled twice).

          Show
          Edu Pereda added a comment - Reopening because there are many exceptions that are not wrapped by neither a MuleException nor a MuleRuntimeException (and so, they are handled twice).
          Hide
          Edu Pereda added a comment -

          Changed the strategy for marking an exception as handled. Instead of using an instance variable on the exception (which only work for the exceptions wrapped by some of the ones that we control: instances of MuleException and MuleRuntimeException), we now use a ThreadLocal variable that contains a reference to the exception being handled on the current thread. So if the same thread tries to handle it again, it will be able to avoid doing so.

          http://fisheye.codehaus.org/changelog/mule/?cs=18641
          http://fisheye.codehaus.org/changelog/mule/?cs=18684

          Show
          Edu Pereda added a comment - Changed the strategy for marking an exception as handled. Instead of using an instance variable on the exception (which only work for the exceptions wrapped by some of the ones that we control: instances of MuleException and MuleRuntimeException), we now use a ThreadLocal variable that contains a reference to the exception being handled on the current thread. So if the same thread tries to handle it again, it will be able to avoid doing so. http://fisheye.codehaus.org/changelog/mule/?cs=18641 http://fisheye.codehaus.org/changelog/mule/?cs=18684
          Hide
          Mateo Almenta Reca added a comment -

          We need to make sure all this cases are addressed by the new exception handling model in 3.0

          Show
          Mateo Almenta Reca added a comment - We need to make sure all this cases are addressed by the new exception handling model in 3.0
          Hide
          Travis Carlson added a comment -

          Reopening to set Fix Version correctly

          Show
          Travis Carlson added a comment - Reopening to set Fix Version correctly

            People

            • Assignee:
              Edu Pereda
              Reporter:
              Edu Pereda
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development