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.
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.
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.
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.
(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.
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.
Felix, it looks like org.apache.jmeter.timers.TimerService#adjustDelay(long) is the way to go here.
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.
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.
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)
Created attachment 36105 [details] Use endTime of JMeterThread for SyncTimer Only use first branch, if timeoutInMs == 0 (as noted by Vladimir, again :)
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 ;)
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
@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
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)
This issue has been migrated to GitHub: https://github.com/apache/jmeter/issues/4843