Bug 62637 - Synchronizing Timer set by threads is stuck when adding duration
Summary: Synchronizing Timer set by threads is stuck when adding duration
Status: RESOLVED FIXED
Alias: None
Product: JMeter
Classification: Unclassified
Component: Main (show other bugs)
Version: 4.0
Hardware: All All
: P2 normal (vote)
Target Milestone: JMETER_5.0
Assignee: JMeter issues mailing list
URL:
Keywords: FixedInTrunk
Depends on:
Blocks:
 
Reported: 2018-08-19 12:48 UTC by orimarko
Modified: 2018-09-05 14:21 UTC (History)
1 user (show)



Attachments
jmx reproducer (3.89 KB, application/xml)
2018-08-19 12:48 UTC, orimarko
Details
Add a BlockingTimer interface (8.41 KB, patch)
2018-08-20 11:44 UTC, Felix Schumacher
Details | Diff
Use endTime of JMeterThread for SyncTimer (5.54 KB, patch)
2018-08-20 14:19 UTC, Felix Schumacher
Details | Diff
Use endTime of JMeterThread for SyncTimer (5.50 KB, patch)
2018-08-20 15:35 UTC, Felix Schumacher
Details | Diff
Allow Long.MAX_VALUE as value for initialDelay in TimerService (988 bytes, patch)
2018-08-20 15:37 UTC, Felix Schumacher
Details | Diff
Use endTime of JMeterThread for SyncTimer (5.57 KB, patch)
2018-08-20 16:10 UTC, Felix Schumacher
Details | Diff
Allow Long.MAX_VALUE as value for initialDelay in TimerService (2.76 KB, patch)
2018-08-20 16:11 UTC, Felix Schumacher
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description orimarko 2018-08-19 12:48:23 UTC
Created attachment 36099 [details]
jmx reproducer

When I'm using Synchronizing Timer set by threads (Number of Simultaneous Users to Group by) it works well except when using with Thread group's duration,

When it's used together the test hang, probably because of Synchronizing issue, as documented:

If timeout in milliseconds is set to 0 and number of threads never reaches "Number of Simultaneous Users to Group by" then Test will pause infinitely. Only a forced stop will stop it. Setting Timeout in milliseconds is an option to consider in this case.

Also Runtime Controller isn't a valid replacement for limiting the duration,

Is there other way to limit test duration, but still use some kind of Synchronization of threads?

Can I add a hook using JSR233 Sampler when test duration over and stop all threads?

I'm thinking of using Precise Throughput Timer, but it seems over complicated for this specific requirement.

I can make test no hang if I put a value in Timeout in milliseconds with value higher than expected in a normal flow, as 10 seconds, 10000, and then after 10 seconds the test stops, but I'm not sure it fixes the issue completely.
Comment 1 Felix Schumacher 2018-08-20 11:44:28 UTC
Created attachment 36100 [details]
Add a BlockingTimer interface

The main problem here is, that SyncTimer does not calculate a delay, but IS the delay.

To mitigate this, this patch adds a new interface BlockingTimer, that indicates this behaviour. Anyone implementing this interface has the responsibility to stop "calculating" the delay when max end time has reached.

With this patch I am not able to reproduce the reported hangs anymore.

But to be honest, it will be easy to produce similar hangs with other timers that behave in the same way and block while calculating the delay.
Comment 2 Vladimir Sitnikov 2018-08-20 12:03:56 UTC
Felix, is BlockingTimer really required there?

It looks like SyncTimer fails to respond to Thread#interrupt which is used by JMeter to terminate threads at test finish.

Current action is just "return 0", however it should probably call Thread.currentThread().interrupt(); before returning.
Comment 3 Vladimir Sitnikov 2018-08-20 12:58:28 UTC
Ah, https://bz.apache.org/bugzilla/show_bug.cgi?id=60049 did not add "interrupt", but it just added a cap on maximum delay (to ensure the delay is within the test duration).


However, there's long endTime = getThreadContext().getThread().getEndTime(), so there's still no need in new interface.
Comment 4 Felix Schumacher 2018-08-20 13:03:21 UTC
(In reply to Vladimir Sitnikov from comment #3)
> Ah, https://bz.apache.org/bugzilla/show_bug.cgi?id=60049 did not add
> "interrupt", but it just added a cap on maximum delay (to ensure the delay
> is within the test duration).
> 
> 
> However, there's long endTime = getThreadContext().getThread().getEndTime(),
> so there's still no need in new interface.

Great, I will amend the patch.
Comment 5 Felix Schumacher 2018-08-20 14:19:52 UTC
Created attachment 36101 [details]
Use endTime of JMeterThread for SyncTimer

As noted by Vladimir, we have the endTime already at hand. So use that for the Timer.
Comment 6 Vladimir Sitnikov 2018-08-20 14:46:30 UTC
Felix, it looks like org.apache.jmeter.timers.TimerService#adjustDelay(long) is the way to go here.
Comment 7 Felix Schumacher 2018-08-20 15:35:45 UTC
Created attachment 36102 [details]
Use endTime of JMeterThread for SyncTimer

Use TimerService#adjustDelay as pointed out by Vladimir.

Note that Long.MAX_VALUE will not work without the next patch as value for adjustDelay.
Comment 8 Felix Schumacher 2018-08-20 15:37:07 UTC
Created attachment 36103 [details]
Allow Long.MAX_VALUE as value for initialDelay in TimerService

If initialDelay is too big we get an overflow and the wrong value will be returned.

This is needed for the actual patch for this bug.
Comment 9 Vladimir Sitnikov 2018-08-20 15:45:41 UTC
Thanks for pushing it forward.
The use of adjustDelay makes code cleaner in my point of view (and it confines all the black magic to a single method).


If you change "if (timeoutInMs == 0) {" to "if (timeoutInMs <= 0) {", it makes "Negative value for timeout" branch obsolete.

So I would prefer either remove the else branch or keep "timeoutInMs == 0"


>if (initialDelay > endTime - now) {

It might make sense to add a comment that explains that "now + initialDelay > endTime" does not work for initialDelay==Long.MAX_VALUE (so others won't re-introduce the bug)
Comment 10 Felix Schumacher 2018-08-20 16:10:21 UTC
Created attachment 36105 [details]
Use endTime of JMeterThread for SyncTimer

Only use first branch, if timeoutInMs == 0 (as noted by Vladimir, again :)
Comment 11 Felix Schumacher 2018-08-20 16:11:59 UTC
Created attachment 36106 [details]
Allow Long.MAX_VALUE as value for initialDelay in TimerService

Add a unit test to help future developers from mistakenly re-introducing the overflow. (I think that is better than a comment ;)
Comment 12 Felix Schumacher 2018-08-22 15:30:56 UTC
Date: Wed Aug 22 15:25:41 2018
New Revision: 1838646

URL: http://svn.apache.org/viewvc?rev=1838646&view=rev
Log:
Part of 62637 Avoid Integer overrun when dealing with very large values in TimerService#adjustDelay

Bugzilla 62637

Added:
    jmeter/trunk/test/src/org/apache/jmeter/timers/TimerServiceTest.java   (with props)
Modified:
    jmeter/trunk/src/core/org/apache/jmeter/timers/TimerService.java
    jmeter/trunk/xdocs/changes.xml
Comment 13 Felix Schumacher 2018-08-22 15:32:18 UTC
@orimarko can you test, whether the changes are really fixing your problems?

Date: Wed Aug 22 15:29:50 2018
New Revision: 1838648

URL: http://svn.apache.org/viewvc?rev=1838648&view=rev
Log:
Take scheduler into account when calcuting delay for Synchronizing Timer

Bugzilla Id: 62637

Added:
    jmeter/trunk/test/src/org/apache/jmeter/timers/SyncTimerSpec.groovy   (with props)
Modified:
    jmeter/trunk/src/components/org/apache/jmeter/timers/SyncTimer.java
    jmeter/trunk/xdocs/changes.xml
Comment 14 Felix Schumacher 2018-08-22 16:42:28 UTC
Date: Wed Aug 22 16:16:00 2018
New Revision: 1838655

URL: http://svn.apache.org/viewvc?rev=1838655&view=rev
Log:
Remove unit test, as it interferes with the other tests right now

The Spock mock/stub mechanism seems to play not nicely together with our test runner
that executes all tests in one JVM.

Bugzilla Id: 62637

Date: Wed Aug 22 16:41:12 2018
New Revision: 1838658

URL: http://svn.apache.org/viewvc?rev=1838658&view=rev
Log:
Add a unit test that is hopefully compatible with our test runner

The first unit test with Spock caused havoc on our test runner. This unit test
includes the same tests without using a mock/stub from Spock.

Bugzilla Id: 62637

Added:
    jmeter/trunk/test/src/org/apache/jmeter/timers/SyncTimerTest.java   (with props)