[
https://issues.apache.org/jira/browse/AMQ-4674?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13745438#comment-13745438
]
Paul Gale edited comment on AMQ-4674 at 8/20/13 9:21 PM:
---------------------------------------------------------
I have no idea how to write unit tests for ActiveMQ and this section of code in
particular.
Unfortunately I could not find any unit tests for this feature which is
surprising given that it was added just the other day. How was it tested? It's
a tad galling to be asked for some tests now.
I don't mind modifying existing tests though. However, my personal experience
with the unit test codebase is that they're rather flaky; they almost never
pass whenever I've tried to run them which doesn't exactly entice me into
wanting to try now.
Nonetheless, from a quick analysis of the code it would appear that the
offending code is in
activemq-stomp/src/main/java/org/apache/activemq/transport/stomp/ProtocolConverter.java
at line 929:
{code:java|borderStyle=solid}
hbReadInterval = (long) (Long.parseLong(keepAliveOpts[0]) *
hbGracePeriodMultiplier); // Wrong!
{code}
should be:
{code:java|borderStyle=solid}
hbReadInterval = (long) Long.parseLong(keepAliveOpts[0]); // Honor the
client's read interval
{code}
where keepAliveOpts[0] is the client specified heartbeat read-interval.
When the inactivity monitor's read check time is calculated it's done correctly:
{code:java|borderStyle=solid}
StompInactivityMonitor monitor = this.stompTransport.getInactivityMonitor();
monitor.setReadCheckTime((long) (hbReadInterval * hbGracePeriodMultiplier));
// Correct
monitor.setInitialDelayTime(Math.min(hbReadInterval, hbWriteInterval));
monitor.setWriteCheckTime(hbWriteInterval);
monitor.startMonitoring();
{code}
Setup: keepAliveOpts[0] = 5000, hbGracePeriodMultiplier = 1.5
Expected: hbReadInterval == 5000, monitor.getReadCheckTime() == 7500
Actual: hbReadInterval == 7500, monitor.getReadCheckTime() == 11250
was (Author: paulgale):
I have no idea how to write unit tests for ActiveMQ and this section of
code in particular.
Unfortunately I could not find any unit tests for this feature which is
surprising given that it was added just the other day. How was it tested? It's
a tad galling to be asked for some tests now.
I don't mind modifying existing tests though. However, my personal experience
with the unit test codebase is that they're rather flaky; they almost never
pass whenever I've tried to run them which doesn't exactly entice me into
wanting to try now.
Nonetheless, from a quick analysis of the code it would appear that the
offending code is in
activemq-stomp/src/main/java/org/apache/activemq/transport/stomp/ProtocolConverter.java
at line 929:
hbReadInterval = (long) (Long.parseLong(keepAliveOpts[0]) *
hbGracePeriodMultiplier); // Wrong!
should be:
hbReadInterval = (long) Long.parseLong(keepAliveOpts[0]); // Honor the
client's read interval
where keepAliveOpts[0] is the client specified heartbeat read-interval.
When the inactivity monitor's read check time is calculated it's done correctly:
StompInactivityMonitor monitor = this.stompTransport.getInactivityMonitor();
monitor.setReadCheckTime((long) (hbReadInterval * hbGracePeriodMultiplier));
// Correct
monitor.setInitialDelayTime(Math.min(hbReadInterval, hbWriteInterval));
monitor.setWriteCheckTime(hbWriteInterval);
monitor.startMonitoring();
Setup: keepAliveOpts[0] = 5000, hbGracePeriodMultiplier = 1.5
Expected: hbReadInterval == 5000, monitor.getReadCheckTime() == 7500
Actual: hbReadInterval == 7500, monitor.getReadCheckTime() == 11250
> ActiveMQ 5.x does not support the notion of a grace-period for heart beats as
> supported by the STOMP protocol
> -------------------------------------------------------------------------------------------------------------
>
> Key: AMQ-4674
> URL: https://issues.apache.org/jira/browse/AMQ-4674
> Project: ActiveMQ
> Issue Type: Bug
> Affects Versions: 5.8.0
> Reporter: Paul Gale
> Assignee: Timothy Bish
> Labels: easyfix
> Fix For: 5.9.0
>
>
> Regarding the configuration of heart beating the STOMP protocol spec states:
> "- because of timing inaccuracies, the receiver SHOULD be tolerant and
> take into account an error margin"
> However, it appears that ActiveMQ 5.x is not tolerant of any error margin.
> Despite the fact that the spec says SHOULD rather than MUST it would make the
> implementation of STOMP clients easier if the error margin was published.
> As the broker aggressively enforces the heart beat timeouts false failover
> attempts can result.
> Apparently Apollo supports an error margin of 1.5x the configured heart beat.
> If it could be made configurable that would be even better!
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
