Camel
  1. Camel
  2. CAMEL-5139

Continued(Predicate) does not work when invoked the second time in the Camel flow.

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Incomplete
    • Affects Version/s: 2.9.1
    • Fix Version/s: 2.9.2, 2.10.0
    • Component/s: camel-core
    • Labels:
      None
    • Estimated Complexity:
      Advanced

      Description

      When Camel flow contains error handling as continued(Predicate), it is not invoked when the error occurs for the second time (even in different onException). What is more the exception is not handled even by global onException and Camel flow is interrupted.

      It is similar issue to https://issues.apache.org/jira/browse/CAMEL-4057
      but not for continued(true) but continued(Predicate).

      The same fix also fixes this problem. Simply adding

      exchange.removeProperty(Exchange.FAILURE_HANDLED);

      just before returning true from the Predicate makes the second onException being called and the second time the exception is raised it can be continued again.

        Issue Links

          Activity

          Hide
          Claus Ibsen added a comment -

          Yes the DSL will be made more strict in Camel 3.0 so people cannot mis-configure in the Java DSL.

          Show
          Claus Ibsen added a comment - Yes the DSL will be made more strict in Camel 3.0 so people cannot mis-configure in the Java DSL.
          Hide
          Radoslaw Szymanek added a comment -

          Thank you for the explanation.

          I just thought it was a cool feature of Camel to be able to redefine the onException policy within one route just in case if different processors happen to raise the same recoverable exception for different reasons.

          It is not easy to figure out what is good design and what is bad design in the context of error handling. The current Java DSL is very flexible, so Camel developers views/opinions are not enforced. It will be great if a new version of Java DSL is somewhat more strict to reflect your expert knowledge of what is a good route design.

          I will note in my coding guidelines for Camel that redefining on exception policies is not well supported now and will not be allowed later on.

          Show
          Radoslaw Szymanek added a comment - Thank you for the explanation. I just thought it was a cool feature of Camel to be able to redefine the onException policy within one route just in case if different processors happen to raise the same recoverable exception for different reasons. It is not easy to figure out what is good design and what is bad design in the context of error handling. The current Java DSL is very flexible, so Camel developers views/opinions are not enforced. It will be great if a new version of Java DSL is somewhat more strict to reflect your expert knowledge of what is a good route design. I will note in my coding guidelines for Camel that redefining on exception policies is not well supported now and will not be allowed later on.
          Hide
          Claus Ibsen added a comment -

          You should configure all your onException in the start of the route.
          And not override onException with the same exception, eg you have 2 x onException(IllegalArgumentException.class).

          In Camel 3.0 onwards we are going to tighten up the DSL.

          Show
          Claus Ibsen added a comment - You should configure all your onException in the start of the route. And not override onException with the same exception, eg you have 2 x onException(IllegalArgumentException.class). In Camel 3.0 onwards we are going to tighten up the DSL.
          Hide
          Radoslaw Szymanek added a comment -

          I added an example of my route for which I hit the bug. Hopefully, it will be enough to reproduce the problem.

          Show
          Radoslaw Szymanek added a comment - I added an example of my route for which I hit the bug. Hopefully, it will be enough to reproduce the problem.
          Hide
          Radoslaw Szymanek added a comment - - edited

          Unfortunately, I do not have enough time to do proper bug reporting. I hope the copy&paste from the example I am working with is enough to help you reproduce it.

          I post the route for which I hit the problem with continued.

          The example works like this. There are two beans. Both are raising the same exception but for different reasons. The first time exception is raised, the problem is handled by processor and continued with predicate will return true, so the the second bean is used for processing the exchange. The second bean also raises the exception. Now, if FAILURE_HANDLED remains within the exchange (check CRUCIAL in code) then the second time exception policy is not called and exception propagated directly to the caller. If I remove the property then the exception policy is called the exception is ignored due to the presence of the header (isAdult) and the processing is completed.

          If I remove FAILURE_HANDLED then my test of this route sees no exception, If I keep this property then the test sees exception raised by ValidityCheck even if it was AgeValidityBean that caused the final problem.

          
                  errorHandler(defaultErrorHandler().maximumRedeliveries(1));
          
                  onException(NullPointerException.class).setOnRedelivery(new NullRegistrationDateProcessor());
          
                  onException(IllegalArgumentException.class).handled(new Predicate() {
                      public boolean matches(Exchange exchange) {
                          boolean result = simple("${header.isAdult} == true").matches(exchange);
                          return result;
                      }
                  });
          
          
                  from("direct:toPropertyFileSmart")
                     .errorHandler(defaultErrorHandler().maximumRedeliveries(0))
                     .onException(IllegalArgumentException.class)
                          .process(new NullRegistrationDateProcessor())
                          .continued(new Predicate() {
                              public boolean matches(Exchange exchange) {
                                  Object body = exchange.getIn().getBody();
                                  if (body instanceof Registration) {
                                      Registration registration = (Registration) body;
                                      if (registration.getRegistrationDate() != null
                                          && exchange.getProperty(Exchange.EXCEPTION_CAUGHT, Exception.class).
                                              getMessage().equals("Registration date attribute of the registration is null")) {
          
                                          // if the line below is removed then the error raised by ValidityCheck will remain 
                                          // and cause problems when the second bean raises the exception.
                                          // CRUCIAL to have
                                          exchange.removeProperty(Exchange.FAILURE_HANDLED);
                                          return true;
                                      }
          
                                  }
                                  return false;
                              }
                          }).end()
                     .bean(ValidityCheck.class)
                     .onException(IllegalArgumentException.class)
                          .continued(new Predicate() {
                              public boolean matches(Exchange exchange) {
                                  boolean result = simple("${header.isAdult} == true").matches(exchange);
                                  exchange.removeProperty(Exchange.FAILURE_HANDLED);
                                  return result;
                              }
                          }).end()
                     .bean(AgeValidityBean.class).to("direct:toPropertyFile");
          

          I decided not to remove any part of the code (making it rather poor bug report) but I do not want to run a risk of removing an important clue of how to reproduce this bug in a simpler test.

          Show
          Radoslaw Szymanek added a comment - - edited Unfortunately, I do not have enough time to do proper bug reporting. I hope the copy&paste from the example I am working with is enough to help you reproduce it. I post the route for which I hit the problem with continued. The example works like this. There are two beans. Both are raising the same exception but for different reasons. The first time exception is raised, the problem is handled by processor and continued with predicate will return true, so the the second bean is used for processing the exchange. The second bean also raises the exception. Now, if FAILURE_HANDLED remains within the exchange (check CRUCIAL in code) then the second time exception policy is not called and exception propagated directly to the caller. If I remove the property then the exception policy is called the exception is ignored due to the presence of the header (isAdult) and the processing is completed. If I remove FAILURE_HANDLED then my test of this route sees no exception, If I keep this property then the test sees exception raised by ValidityCheck even if it was AgeValidityBean that caused the final problem. errorHandler(defaultErrorHandler().maximumRedeliveries(1)); onException(NullPointerException.class).setOnRedelivery( new NullRegistrationDateProcessor()); onException(IllegalArgumentException.class).handled( new Predicate() { public boolean matches(Exchange exchange) { boolean result = simple( "${header.isAdult} == true " ).matches(exchange); return result; } }); from( "direct:toPropertyFileSmart" ) .errorHandler(defaultErrorHandler().maximumRedeliveries(0)) .onException(IllegalArgumentException.class) .process( new NullRegistrationDateProcessor()) .continued( new Predicate() { public boolean matches(Exchange exchange) { Object body = exchange.getIn().getBody(); if (body instanceof Registration) { Registration registration = (Registration) body; if (registration.getRegistrationDate() != null && exchange.getProperty(Exchange.EXCEPTION_CAUGHT, Exception.class). getMessage().equals( "Registration date attribute of the registration is null " )) { // if the line below is removed then the error raised by ValidityCheck will remain // and cause problems when the second bean raises the exception. // CRUCIAL to have exchange.removeProperty(Exchange.FAILURE_HANDLED); return true ; } } return false ; } }).end() .bean(ValidityCheck.class) .onException(IllegalArgumentException.class) .continued( new Predicate() { public boolean matches(Exchange exchange) { boolean result = simple( "${header.isAdult} == true " ).matches(exchange); exchange.removeProperty(Exchange.FAILURE_HANDLED); return result; } }).end() .bean(AgeValidityBean.class).to( "direct:toPropertyFile" ); I decided not to remove any part of the code (making it rather poor bug report) but I do not want to run a risk of removing an important clue of how to reproduce this bug in a simpler test.
          Hide
          Claus Ibsen added a comment -

          In Camel using a boolean or predicate for continued/handled becomes the same, as the boolean is wrapped into a predicate. I have added unit tests based on commits of CAMEL-5162 and this works fine on trunk, and 2.9 branch.

          Show
          Claus Ibsen added a comment - In Camel using a boolean or predicate for continued/handled becomes the same, as the boolean is wrapped into a predicate. I have added unit tests based on commits of CAMEL-5162 and this works fine on trunk, and 2.9 branch.
          Hide
          Claus Ibsen added a comment -

          I logged CAMEL-5162 about the 2x evaluation per exception.

          I cannot reproduce this error you report here on trunk.

          Show
          Claus Ibsen added a comment - I logged CAMEL-5162 about the 2x evaluation per exception. I cannot reproduce this error you report here on trunk.
          Hide
          Claus Ibsen added a comment -

          A patch with unit test is much welcome.

          Show
          Claus Ibsen added a comment - A patch with unit test is much welcome.
          Hide
          Radoslaw Szymanek added a comment -

          A side note.

          While debugging I noticed that the predicate in continued(Predicate) is executed twice. I see no obvious reasons why it would happen (no redeliveries allowed in my route, etc). The first execution happens right at the beginning of the onException(...), then processor is executed to "fix" the exchange and then the Predicate is called again with "fixed" exchange so true can be returned.

          The onException part within a route looks like this :
          .onException(IllegalArgumentException.class)
          .process(new NullRegistrationDateProcessor())
          .continued(new Predicate()

          { ... }

          .end()

          Moreover, even if the predicate within continued always returns true (even the first time called for the original exchange that caused exception) it does not prevent the processor to be executed and second time the predicate within continued again.

          The second execution of continued is the deciding one, if rigged to return false, while the first execution of the predicate is rigged to return true this will not continue the exchange.

          I do not create an issue out of this as this may be perfectly reasonable behavior. However, it is suspicious enough to deserve a note in this issue.

          Show
          Radoslaw Szymanek added a comment - A side note. While debugging I noticed that the predicate in continued(Predicate) is executed twice. I see no obvious reasons why it would happen (no redeliveries allowed in my route, etc). The first execution happens right at the beginning of the onException(...), then processor is executed to "fix" the exchange and then the Predicate is called again with "fixed" exchange so true can be returned. The onException part within a route looks like this : .onException(IllegalArgumentException.class) .process(new NullRegistrationDateProcessor()) .continued(new Predicate() { ... } .end() Moreover, even if the predicate within continued always returns true (even the first time called for the original exchange that caused exception) it does not prevent the processor to be executed and second time the predicate within continued again. The second execution of continued is the deciding one, if rigged to return false, while the first execution of the predicate is rigged to return true this will not continue the exchange. I do not create an issue out of this as this may be perfectly reasonable behavior. However, it is suspicious enough to deserve a note in this issue.

            People

            • Assignee:
              Claus Ibsen
              Reporter:
              Radoslaw Szymanek
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development