Bug 58506 - JMS Point-to-point sampler should offer an async using temp queue mode and prevent hangs by supporting a reply timeout
Summary: JMS Point-to-point sampler should offer an async using temp queue mode and pr...
Status: NEW
Alias: None
Product: JMeter
Classification: Unclassified
Component: Main (show other bugs)
Version: 2.13
Hardware: PC All
: P2 enhancement with 3 votes (vote)
Target Milestone: ---
Assignee: JMeter issues mailing list
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-10-19 17:55 UTC by Gordon Daugherty
Modified: 2016-08-12 21:14 UTC (History)
3 users (show)



Attachments
Patchfile showing draft of proposed UI changes (6.60 KB, patch)
2016-08-09 13:33 UTC, Gordon Daugherty
Details | Diff
Compiled demo of proposed UI changes. Matches patchfile in attachment 34113. (12.19 KB, application/zip)
2016-08-09 13:35 UTC, Gordon Daugherty
Details
Screenshot of proposed UI - showing settings for existing behavior (56.32 KB, image/png)
2016-08-09 13:40 UTC, Gordon Daugherty
Details
Screenshot of proposed UI - showing settings for new behavior (temp q mode) (46.15 KB, image/png)
2016-08-09 13:40 UTC, Gordon Daugherty
Details
Screenshot of proposed UI - showing settings for new behavior (permanent queue mode) (55.59 KB, image/png)
2016-08-09 13:41 UTC, Gordon Daugherty
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Gordon Daugherty 2015-10-19 17:55:19 UTC
Component: JMS Point-to-Point sampler

Observed from the JMeter GUI on Windows 7 and when run via jmeter-maven-plugin on Redhat 6.6. Observed in JMeter v2.13 most recently and have also noticed it in version 2.6 in the past.

Problematic behavior:

When run in "Request Response" mode and a value is not set for "JNDI name Receive queue" there is no effective way to deal with timeouts. If a reply message never arrives the test thread hangs forever. Per the Stackoverflow article linked below this sampler uses a JMS Temporary Queue to handle the replies and does not set any timeout when waiting for those replies.

http://stackoverflow.com/questions/28609859/timeout-of-jms-point-to-point-requests-in-jmeter-does-not-result-in-an-error

https://github.com/apache/jmeter/blob/dd30d6171d031d3288c7d31da303823dccee03c2/src/protocol/jms/org/apache/jmeter/protocol/jms/sampler/JMSSampler.java#L373

Excerpt from the source linked immediately above:

 if (useTemporyQueue()) {
   executor = new TemporaryQueueExecutor(session, sendQueue); // Timeout setting isn't applied!
 } else {
   producer = session.createSender(sendQueue);
  executor = new FixedQueueExecutor(producer, getTimeoutAsInt(), isUseReqMsgIdAsCorrelId());
 }

Also, the TemporaryQueueExecutor's sendAndReceive method is given a timeout value by the JMSSampler class BUT it doesn't use it per the source linked and excerpted immediately below:

https://github.com/apache/jmeter/blob/5512162ec752b35378275711a9a03f4d003a664a/src/protocol/jms/org/apache/jmeter/protocol/jms/sampler/TemporaryQueueExecutor.java

  @Override
  public Message sendAndReceive(Message request, 
            int deliveryMode, 
            int priority, 
            long expiration) throws JMSException {
        return requestor.request(request);
  }


How to reproduce the behavior:

Create a JMS service that replies to any input message by sending a reply message to the destination found in the request message at "javax.jms.Message.getJMSReplyTo()". Then configure a JMeter job to use the "JMS Point-to-Point" sampler to send messages to the request queue for that "dummy service". Do not set the "JNDI name Receive queue" property on the sampler. Set the timeout to 5 seconds and observe that the test works. Now modify the service provider to not send any responses. Run the test again and observe that it never completes.

Desired behavior:

When a reply message does not arrive within the specified timeout window that sample should be marked as failed and the test should proceed to the next sample. It is acceptable for this to continue to work in a blocking manner where the next request doesn't go out until the reply arrives or (after this fix/enhancement is applied) the timeout is hit.

The implementation of "TemporaryQueueExecutor" reuses the same temporary queue for all requests issued by the thread and doesn't use any form of secondary reply message correlation technique to verify that the received message is a response to the request message. As such, when a timeout occurs the executor needs to handle it by marking this sample as a failure, deleting the temporary queue, creating a new temporary queue, and then allowing the next request to proceed. Deleting the temporary queue is necessary in case the provider sends a reply message after the timeout period; we don't want the next sample to receive that late reply as if it were the reply to some other issued-later request.

To implement this change you'll have to abandon the usage of JMS's QueueRequestor since it doesn't appear to let you set a timeout.
Comment 1 Gordon Daugherty 2015-11-02 22:31:36 UTC
Here's an alternative solution that preserves the configuration interface but significantly changes the test's behavior:

When no "JNDI name Receive queue" is provided create a single temporary queue and internally use that queue name for that field.

This way the execution behavior would always be asynchronous with requests going out at a consistent rate regardless of how fast the replies are coming back. This has the advantage of avoiding the "Coordinated Omission" problem (search "Gil Tene Coordinated Omission" if you're not familiar with that concept). This solution keeps the configuration interface simple. The downside is that people that were using the TemporaryQueueExecutor approach would see new behavior when they upgrade; these more-accurate results are likely to show worse peformance numbers so we'd get some questions when people do that upgrade.
Comment 2 Gordon Daugherty 2016-02-01 23:06:04 UTC
This is a request for feedback to guide our development effort on this one. We're considering taking the approach given in my last comment:

When no "JNDI name Receive queue" is provided create a single temporary queue and act as if that temp queue's name was placed in the "JNDI name Receive queue" field. This will naturally cause receive timeouts to be applied AND will avoid the coordinated omission error in the metrics that JMeter collects.

Please respond by stating whether you have significant concerns about this approach. It'll keep the UI simple but will prevent JMeter from being able to generate a load using a limited number of threads each doing a blocking invocation.

If there are no objections to this approach we'll do it as-proposed to keep it simple. If you believe that users want to be able to generate load using a limited number of threads each doing a blocking invocation we can try to come up with an understandable way to present both options to users in the UI but I think it'll end up making the UI less understandable.
Comment 3 Philippe Mouawad 2016-02-02 21:55:16 UTC
(In reply to Gordon Daugherty from comment #1)
> Here's an alternative solution that preserves the configuration interface
> but significantly changes the test's behavior:
> 
> When no "JNDI name Receive queue" is provided create a single temporary
> queue and internally use that queue name for that field.
> 
> This way the execution behavior would always be asynchronous with requests
> going out at a consistent rate regardless of how fast the replies are coming
> back. This has the advantage of avoiding the "Coordinated Omission" problem
> (search "Gil Tene Coordinated Omission" if you're not familiar with that
> concept). This solution keeps the configuration interface simple. The
> downside is that people that were using the TemporaryQueueExecutor approach
> would see new behavior when they upgrade; these more-accurate results are
> likely to show worse peformance numbers so we'd get some questions when
> people do that upgrade.

(In reply to Gordon Daugherty from comment #0)
> Component: JMS Point-to-Point sampler
> 
> Observed from the JMeter GUI on Windows 7 and when run via
> jmeter-maven-plugin on Redhat 6.6. Observed in JMeter v2.13 most recently
> and have also noticed it in version 2.6 in the past.
> 
> Problematic behavior:
> 
> When run in "Request Response" mode and a value is not set for "JNDI name
> Receive queue" there is no effective way to deal with timeouts. If a reply
> message never arrives the test thread hangs forever. Per the Stackoverflow
> article linked below this sampler uses a JMS Temporary Queue to handle the
> replies and does not set any timeout when waiting for those replies.
> 
> http://stackoverflow.com/questions/28609859/timeout-of-jms-point-to-point-
> requests-in-jmeter-does-not-result-in-an-error
> 
> https://github.com/apache/jmeter/blob/
> dd30d6171d031d3288c7d31da303823dccee03c2/src/protocol/jms/org/apache/jmeter/
> protocol/jms/sampler/JMSSampler.java#L373
> 
> Excerpt from the source linked immediately above:
> 
>  if (useTemporyQueue()) {
>    executor = new TemporaryQueueExecutor(session, sendQueue); // Timeout
> setting isn't applied!
>  } else {
>    producer = session.createSender(sendQueue);
>   executor = new FixedQueueExecutor(producer, getTimeoutAsInt(),
> isUseReqMsgIdAsCorrelId());
>  }
> 
> Also, the TemporaryQueueExecutor's sendAndReceive method is given a timeout
> value by the JMSSampler class BUT it doesn't use it per the source linked
> and excerpted immediately below:
> 
> https://github.com/apache/jmeter/blob/
> 5512162ec752b35378275711a9a03f4d003a664a/src/protocol/jms/org/apache/jmeter/
> protocol/jms/sampler/TemporaryQueueExecutor.java
> 
>   @Override
>   public Message sendAndReceive(Message request, 
>             int deliveryMode, 
>             int priority, 
>             long expiration) throws JMSException {
>         return requestor.request(request);
>   }

Just to be sure there is no confusion, expiration here is not a timeout in processing of message. It's an expiration of the message. 

> 
> 
> How to reproduce the behavior:
> 
> Create a JMS service that replies to any input message by sending a reply
> message to the destination found in the request message at
> "javax.jms.Message.getJMSReplyTo()". Then configure a JMeter job to use the
> "JMS Point-to-Point" sampler to send messages to the request queue for that
> "dummy service". Do not set the "JNDI name Receive queue" property on the
> sampler. Set the timeout to 5 seconds and observe that the test works. Now
> modify the service provider to not send any responses. Run the test again
> and observe that it never completes.
> 
> Desired behavior:
> 
> When a reply message does not arrive within the specified timeout window
> that sample should be marked as failed and the test should proceed to the
> next sample. It is acceptable for this to continue to work in a blocking
> manner where the next request doesn't go out until the reply arrives or
> (after this fix/enhancement is applied) the timeout is hit.
> 
> The implementation of "TemporaryQueueExecutor" reuses the same temporary
> queue for all requests issued by the thread and doesn't use any form of
> secondary reply message correlation technique to verify that the received
> message is a response to the request message. As such, when a timeout occurs
> the executor needs to handle it by marking this sample as a failure,
> deleting the temporary queue, creating a new temporary queue, and then
> allowing the next request to proceed. Deleting the temporary queue is
> necessary in case the provider sends a reply message after the timeout
> period; we don't want the next sample to receive that late reply as if it
> were the reply to some other issued-later request.
> 
> To implement this change you'll have to abandon the usage of JMS's
> QueueRequestor since it doesn't appear to let you set a timeout.
Comment 4 Philippe Mouawad 2016-02-02 22:04:39 UTC
(In reply to Gordon Daugherty from comment #2)
> This is a request for feedback to guide our development effort on this one.
> We're considering taking the approach given in my last comment:
> 
> When no "JNDI name Receive queue" is provided create a single temporary
> queue and act as if that temp queue's name was placed in the "JNDI name
> Receive queue" field. This will naturally cause receive timeouts to be
> applied AND will avoid the coordinated omission error in the metrics that
> JMeter collects.
> 
> Please respond by stating whether you have significant concerns about this
> approach. It'll keep the UI simple but will prevent JMeter from being able
> to generate a load using a limited number of threads each doing a blocking
> invocation.
> 
> If there are no objections to this approach we'll do it as-proposed to keep
> it simple. If you believe that users want to be able to generate load using
> a limited number of threads each doing a blocking invocation we can try to
> come up with an understandable way to present both options to users in the
> UI but I think it'll end up making the UI less understandable.

First thanks for your contribution.
Second, if possible, I think you should keep the current possibility to generate load using a limited number of threads each doing a blocking invocation, unless this is absolutely stupid, which for now is not sure in my understanding.

What would be the impact on UI of this ?
Comment 5 Gordon Daugherty 2016-08-09 13:26:53 UTC
You asked about the impact to the UI. I'll added attachments in a moment that demonstrate how it might look if we preserve the availability of the old functionality, offer the new, and make the new the default. I'll attach screenshots and the source code used to mock this up.
Comment 6 Gordon Daugherty 2016-08-09 13:33:22 UTC
Created attachment 34113 [details]
Patchfile showing draft of proposed UI changes
Comment 7 Gordon Daugherty 2016-08-09 13:35:37 UTC
Created attachment 34114 [details]
Compiled demo of proposed UI changes. Matches patchfile in attachment 34113 [details].
Comment 8 Gordon Daugherty 2016-08-09 13:40:09 UTC
Created attachment 34115 [details]
Screenshot of proposed UI - showing settings for existing behavior
Comment 9 Gordon Daugherty 2016-08-09 13:40:34 UTC
Created attachment 34116 [details]
Screenshot of proposed UI - showing settings for new behavior (temp q mode)
Comment 10 Gordon Daugherty 2016-08-09 13:41:02 UTC
Created attachment 34117 [details]
Screenshot of proposed UI - showing settings for new behavior (permanent queue mode)