Camel
  1. Camel
  2. CAMEL-1366

EndpointMessageListener should respect ExchangePattern

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.6.0
    • Fix Version/s: 2.0-M1
    • Component/s: camel-jms
    • Labels:
      None
    • Environment:

      ActiveMQ/Camel

      Description

      In all current releases, org.apache.camel.component.jms.EndpointMessageListener.onMessage() has the following logic (line 90 in 1.6.0 code):

      // send the reply
      if (rce == null && body != null && !disableReplyTo) {
          sendReply(replyDestination, message, exchange, body);
      }
      

      This logic should also respect ExchangePattern of the exchange, so I propose a change to:

      // send the reply
      if (rce == null && body != null && exchange.isOutCapable()) {
          sendReply(replyDestination, message, exchange, body);
      }
      

      This change allows a processing pattern where the route may change the ExchangePattern using methods like RouteBuilder.inOnly() to switch the MEP at will so that the reply is send at a later time (true asynchronous exchange). This processing pattern is particularly useful for integrating long running services. For example,

      // Java DSL
      from("activemq:my_queue?exchangePattern=InOnly").to("predict_weather://?reply_later=true");
      // or
      from("activemq:my_queue2").inOnly().to("predict_weather://?reply_later=true");
      

      The flaw of the current logic makes it impossible to do true asynchronous exchange, because 1) it does not respect the ExchangePattern; 2) if property "disableReplyTo" is used, the "org.apache.camel.jms.replyDestination" property will not be set (see method createExchange in the same file), thus downstream cannot find the reply destination.

      The proposed change can also deprecate the disableReplyTo property and put the MEP concept into good use.

        Issue Links

          Activity

          Michael Chen created issue -
          Michael Chen made changes -
          Field Original Value New Value
          Description In all current releases, org.apache.camel.component.jms.EndpointMessageListener.onMessage() has the following logic (line 90 in 1.6.0 code):
          {code}
          // send the reply
          if (rce == null && body != null && !disableReplyTo) {
              sendReply(replyDestination, message, exchange, body);
          }
          {code}
          This logic should also respect ExchangePattern of the exchange, so I propose a change to:
          {code}
          // send the reply
          if (rce == null && body != null && exchange.isOutCapable()) {
              sendReply(replyDestination, message, exchange, body);
          }
          {code}
          This change allows a processing pattern where the route may change the ExchangePattern using methods like RouteBuilder.inOnly() to switch the MEP at will so that the reply is send at a later time (true asynchronous exchange). This processing pattern is particularly useful for integrating long running services. For example,
          {code}
          // Java DSL
          from("activemq:my_queue").InOnly().to("predict_weather://?reply_later=true");
          {code}
          The flaw of the current logic makes it impossible to do true asynchronous exchange, because 1) it does not respect the ExchangePattern; 2) if property "disableReplyTo" is used, the "org.apache.camel.jms.replyDestination" property will not be set (see method createExchange in the same file), thus downstream cannot find the reply destination.

          The proposed change can also deprecate the disableReplyTo property and put the MEP concept into good use.
          In all current releases, org.apache.camel.component.jms.EndpointMessageListener.onMessage() has the following logic (line 90 in 1.6.0 code):
          {code}
          // send the reply
          if (rce == null && body != null && !disableReplyTo) {
              sendReply(replyDestination, message, exchange, body);
          }
          {code}
          This logic should also respect ExchangePattern of the exchange, so I propose a change to:
          {code}
          // send the reply
          if (rce == null && body != null && exchange.isOutCapable()) {
              sendReply(replyDestination, message, exchange, body);
          }
          {code}
          This change allows a processing pattern where the route may change the ExchangePattern using methods like RouteBuilder.inOnly() to switch the MEP at will so that the reply is send at a later time (true asynchronous exchange). This processing pattern is particularly useful for integrating long running services. For example,
          {code}
          // Java DSL
          from("activemq:my_queue?exchangePattern=InOnly").to("predict_weather://?reply_later=true");
          {code}
          The flaw of the current logic makes it impossible to do true asynchronous exchange, because 1) it does not respect the ExchangePattern; 2) if property "disableReplyTo" is used, the "org.apache.camel.jms.replyDestination" property will not be set (see method createExchange in the same file), thus downstream cannot find the reply destination.

          The proposed change can also deprecate the disableReplyTo property and put the MEP concept into good use.
          Michael Chen made changes -
          Description In all current releases, org.apache.camel.component.jms.EndpointMessageListener.onMessage() has the following logic (line 90 in 1.6.0 code):
          {code}
          // send the reply
          if (rce == null && body != null && !disableReplyTo) {
              sendReply(replyDestination, message, exchange, body);
          }
          {code}
          This logic should also respect ExchangePattern of the exchange, so I propose a change to:
          {code}
          // send the reply
          if (rce == null && body != null && exchange.isOutCapable()) {
              sendReply(replyDestination, message, exchange, body);
          }
          {code}
          This change allows a processing pattern where the route may change the ExchangePattern using methods like RouteBuilder.inOnly() to switch the MEP at will so that the reply is send at a later time (true asynchronous exchange). This processing pattern is particularly useful for integrating long running services. For example,
          {code}
          // Java DSL
          from("activemq:my_queue?exchangePattern=InOnly").to("predict_weather://?reply_later=true");
          {code}
          The flaw of the current logic makes it impossible to do true asynchronous exchange, because 1) it does not respect the ExchangePattern; 2) if property "disableReplyTo" is used, the "org.apache.camel.jms.replyDestination" property will not be set (see method createExchange in the same file), thus downstream cannot find the reply destination.

          The proposed change can also deprecate the disableReplyTo property and put the MEP concept into good use.
          In all current releases, org.apache.camel.component.jms.EndpointMessageListener.onMessage() has the following logic (line 90 in 1.6.0 code):
          {code}
          // send the reply
          if (rce == null && body != null && !disableReplyTo) {
              sendReply(replyDestination, message, exchange, body);
          }
          {code}
          This logic should also respect ExchangePattern of the exchange, so I propose a change to:
          {code}
          // send the reply
          if (rce == null && body != null && exchange.isOutCapable()) {
              sendReply(replyDestination, message, exchange, body);
          }
          {code}
          This change allows a processing pattern where the route may change the ExchangePattern using methods like RouteBuilder.inOnly() to switch the MEP at will so that the reply is send at a later time (true asynchronous exchange). This processing pattern is particularly useful for integrating long running services. For example,
          {code}
          // Java DSL
          from("activemq:my_queue?exchangePattern=InOnly").to("predict_weather://?reply_later=true");
          // or
          from("activemq:my_queue2").inOnly().to("predict_weather://?reply_later=true");
          {code}
          The flaw of the current logic makes it impossible to do true asynchronous exchange, because 1) it does not respect the ExchangePattern; 2) if property "disableReplyTo" is used, the "org.apache.camel.jms.replyDestination" property will not be set (see method createExchange in the same file), thus downstream cannot find the reply destination.

          The proposed change can also deprecate the disableReplyTo property and put the MEP concept into good use.
          Hide
          Claus Ibsen added a comment -

          What if the JMSMessage have a replyTo desintation set?

          Then the original caller expects a response. Then we can not just disregard this.
          Could you dig into the code and check how this is handled.

          Show
          Claus Ibsen added a comment - What if the JMSMessage have a replyTo desintation set? Then the original caller expects a response. Then we can not just disregard this. Could you dig into the code and check how this is handled.
          Claus Ibsen made changes -
          Component/s camel-jms [ 11642 ]
          Hide
          Michael Chen added a comment -

          As I stated above, I want to end the current process chain without sending back a reply. When the long running services is completed, the reply will be sent then and only then. This free up the original processing thread immediately. The original caller still gets the response it expect.

          I now realize that the RouteBuilder methods like inOnly(), outOnly(), etc. are useless. They only effects downstream processors in the route. It does NOT change the MEP of the endpoint and its behavior. I will submit a separate bug on this.

          Show
          Michael Chen added a comment - As I stated above, I want to end the current process chain without sending back a reply. When the long running services is completed, the reply will be sent then and only then. This free up the original processing thread immediately. The original caller still gets the response it expect. I now realize that the RouteBuilder methods like inOnly(), outOnly(), etc. are useless. They only effects downstream processors in the route. It does NOT change the MEP of the endpoint and its behavior. I will submit a separate bug on this.
          Hide
          Claus Ibsen added a comment -

          @Michael

          The last one is not a bug. inOnly, inOut is used to affect/change the MEP on route.

          What you are asking for is on the consumer side (jms consumer, the from node) to set it to request only, or request-reply.

          However the jms spec has this JMSReplyTo that we should honor. So if someone sends a JMS message to a queue with a JMSReplyTo header then that caller would expect a reply on that destination and thus Camel should honor this and return a reply.

          So today you can do what you want:

          from("activemq:my_queue").to("predict_weather://?reply_later=true");
          

          If the message sent to my_queue does NOT contain a JMSReplyTo then its a request only.

          However what we could consider is to also test for exchange.isOutCapable() and send a "null" body if the test fails to signal no/empty reply.

          Show
          Claus Ibsen added a comment - @Michael The last one is not a bug. inOnly, inOut is used to affect/change the MEP on route. What you are asking for is on the consumer side (jms consumer, the from node) to set it to request only, or request-reply. However the jms spec has this JMSReplyTo that we should honor. So if someone sends a JMS message to a queue with a JMSReplyTo header then that caller would expect a reply on that destination and thus Camel should honor this and return a reply. So today you can do what you want: from( "activemq:my_queue" ).to( "predict_weather: //?reply_later= true " ); If the message sent to my_queue does NOT contain a JMSReplyTo then its a request only. However what we could consider is to also test for exchange.isOutCapable() and send a "null" body if the test fails to signal no/empty reply.
          Hide
          Michael Chen added a comment -

          exchange.isOutCapable() is always true downstream due to the logic in method EndpointMessageListener.createExchange(). That method forces the MEP to be InOut if JMSReplyTo is present in the original request.

          If your reason for forcing a reply is to honor the JMS spec, I can't argue otherwise. Please close this bug.

          However, I believe the camel.component.jms implementation should offer the option of not replying the original request and give that job to other components or processors downstream. This does not break the JMS spec, but just a matter of which Camel component is replying.

          Show
          Michael Chen added a comment - exchange.isOutCapable() is always true downstream due to the logic in method EndpointMessageListener.createExchange(). That method forces the MEP to be InOut if JMSReplyTo is present in the original request. If your reason for forcing a reply is to honor the JMS spec, I can't argue otherwise. Please close this bug. However, I believe the camel.component.jms implementation should offer the option of not replying the original request and give that job to other components or processors downstream. This does not break the JMS spec, but just a matter of which Camel component is replying.
          Hide
          Claus Ibsen added a comment -

          @Michael

          That is actually a good idea. The reply can be done later as you say.
          But isnt that supported already with the disableReplyTo option?

          from("activemq:my_queue?disableReplyTo=true").to("predict_weather://?reply_later=true");
          
          Show
          Claus Ibsen added a comment - @Michael That is actually a good idea. The reply can be done later as you say. But isnt that supported already with the disableReplyTo option? from( "activemq:my_queue?disableReplyTo= true " ).to( "predict_weather: //?reply_later= true " );
          Hide
          Michael Chen added a comment -

          Actually, the logic in EndpointMessageListener.createExchange() made it impossible. If disableReplyTo is set to true, the "org.apache.camel.jms.replyDestination" will not be set. Downstream processor will have no way to know how to reply.

          Since we have talked about it, I want to reinstate the original change suggestion plus the following. In the createExchange() method, these lines:

          //
                  if (replyDestination != null && !disableReplyTo) {
                      exchange.setProperty("org.apache.camel.jms.replyDestination", replyDestination);
                      exchange.setPattern(ExchangePattern.InOut);
                  }
          

          should be changed to:

          //
                  if (replyDestination != null && !disableReplyTo) {
                      exchange.setProperty("org.apache.camel.jms.replyDestination", replyDestination);
                      if (!exchange.getPattern().isOutCapable())
                          exchange.setPattern(ExchangePattern.InOut);
                  }
          

          This change will account for ExchangePattern.InOptionalOut. However, it will also require the fix for bug CAMEL-1384 I submitted yesterday for everything to work. In summary, a route could be defined as:

          from("activemq:my_queue?exchangePattern=InOptionalOut").to("predict_weather://?reply_later=true");
          

          Then if and only if the last processor does NOT set a out message, camel.jms will not sent a reply. It is the downstream processors' responsibility to use property "org.apache.camel.jms.replyDestination" to construct and sent the JMS reply message.

          The two changes in this bug and CAMEL-1384 are 100% backward compatible, since InOptionalOut has not been considered for camel.jms before.

          Show
          Michael Chen added a comment - Actually, the logic in EndpointMessageListener.createExchange() made it impossible. If disableReplyTo is set to true, the "org.apache.camel.jms.replyDestination" will not be set. Downstream processor will have no way to know how to reply. Since we have talked about it, I want to reinstate the original change suggestion plus the following. In the createExchange() method, these lines: // if (replyDestination != null && !disableReplyTo) { exchange.setProperty( "org.apache.camel.jms.replyDestination" , replyDestination); exchange.setPattern(ExchangePattern.InOut); } should be changed to: // if (replyDestination != null && !disableReplyTo) { exchange.setProperty( "org.apache.camel.jms.replyDestination" , replyDestination); if (!exchange.getPattern().isOutCapable()) exchange.setPattern(ExchangePattern.InOut); } This change will account for ExchangePattern.InOptionalOut. However, it will also require the fix for bug CAMEL-1384 I submitted yesterday for everything to work. In summary, a route could be defined as: from( "activemq:my_queue?exchangePattern=InOptionalOut" ).to( "predict_weather: //?reply_later= true " ); Then if and only if the last processor does NOT set a out message, camel.jms will not sent a reply. It is the downstream processors' responsibility to use property "org.apache.camel.jms.replyDestination" to construct and sent the JMS reply message. The two changes in this bug and CAMEL-1384 are 100% backward compatible, since InOptionalOut has not been considered for camel.jms before.
          Claus Ibsen made changes -
          Link This issue depends upon CAMEL-1384 [ CAMEL-1384 ]
          Hide
          Claus Ibsen added a comment -

          Good idea.

          I was planning to look into a few tickets for camel-jms related to Camel 2.0. Will look into this one as well, so we can support your use case.

          Show
          Claus Ibsen added a comment - Good idea. I was planning to look into a few tickets for camel-jms related to Camel 2.0. Will look into this one as well, so we can support your use case.
          Claus Ibsen made changes -
          Assignee Claus Ibsen [ davsclaus ]
          Fix Version/s 2.0.0 [ 11900 ]
          Hide
          Claus Ibsen added a comment -

          trunk: Committed revision 748476.

          Show
          Claus Ibsen added a comment - trunk: Committed revision 748476.
          Claus Ibsen made changes -
          Link This issue depends upon CAMEL-1405 [ CAMEL-1405 ]
          Hide
          Claus Ibsen added a comment -

          James created the stuff in camel-jms to support this

          Show
          Claus Ibsen added a comment - James created the stuff in camel-jms to support this
          Hide
          Claus Ibsen added a comment -

          The reply destination is set as a property on the exchange:

          Destination replyDestination = in.getHeader(JmsConstants.JMS_REPLY_DESTINATION, Destination.class);
          

          So you have a Destination object you can use to send the reply later.

          See unit test JmsSimpleRequestLateReplyTest in camel-jms.

          Show
          Claus Ibsen added a comment - The reply destination is set as a property on the exchange: Destination replyDestination = in.getHeader(JmsConstants.JMS_REPLY_DESTINATION, Destination.class); So you have a Destination object you can use to send the reply later. See unit test JmsSimpleRequestLateReplyTest in camel-jms.
          Claus Ibsen made changes -
          Resolution Fixed [ 1 ]
          Status Open [ 1 ] Resolved [ 5 ]
          Hadrian Zbarcea made changes -
          Fix Version/s 2.0.0 [ 11900 ]
          Fix Version/s 2.0-M1 [ 12061 ]
          Hide
          Claus Ibsen added a comment -

          Closing 2.0m1 tickets

          Show
          Claus Ibsen added a comment - Closing 2.0m1 tickets
          Claus Ibsen made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Jeff Turner made changes -
          Project Import Sat Nov 27 00:14:50 EST 2010 [ 1290834890113 ]
          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Resolved Resolved
          6d 14h 48m 1 Claus Ibsen 27/Feb/09 13:16
          Resolved Resolved Closed Closed
          153d 17h 17m 1 Claus Ibsen 31/Jul/09 06:34

            People

            • Assignee:
              Claus Ibsen
              Reporter:
              Michael Chen
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development