Bug 63490 - At end of scheduler duration lots of Samplers gets executed at the same time
Summary: At end of scheduler duration lots of Samplers gets executed at the same time
Status: RESOLVED FIXED
Alias: None
Product: JMeter
Classification: Unclassified
Component: Main (show other bugs)
Version: 5.1.1
Hardware: All All
: P2 normal (vote)
Target Milestone: JMETER_5.2
Assignee: JMeter issues mailing list
URL:
Keywords: FixedInTrunk
Depends on:
Blocks:
 
Reported: 2019-06-07 17:57 UTC by Felix Schumacher
Modified: 2019-09-21 16:14 UTC (History)
1 user (show)



Attachments
Check for end of scheduler duration before the real sampling occurs (571 bytes, patch)
2019-06-07 17:57 UTC, Felix Schumacher
Details | Diff
Check for end of scheduler duration before the real sampling occurs (1.03 KB, patch)
2019-06-07 19:23 UTC, Felix Schumacher
Details | Diff
Simple test plan to reproduce bug (4.06 KB, application/xml)
2019-06-07 19:34 UTC, Felix Schumacher
Details
Check for end of scheduler duration before the real sampling occurs (7.99 KB, patch)
2019-06-09 11:53 UTC, Felix Schumacher
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Felix Schumacher 2019-06-07 17:57:03 UTC
Created attachment 36610 [details]
Check for end of scheduler duration before the real sampling occurs

Samplers might get started at the end of the scheduled duration, when they are configured with a delay. That delay has already been shortened to end at the end of duration with Bug 60049.

That might lead to a stampede of samplers getting started at the end of a load test. When the tested system is too weak to handle this, those samples will take a lot of time. This will result in a strange looking report with high latency at the end of the test.

To mitigate this, I propose to check for end of scheduler duration before right after the delaying logic inside the sampling setup.
Comment 1 Vladimir Sitnikov 2019-06-07 18:18:48 UTC
@Felix, I think this fix should be put right into delay method, because it has extra delay that "prevents" delays that exceed scheduled timeframe.

In other words, dealay method should stop scheduling rather than calling "adjustDelay".
Comment 2 Felix Schumacher 2019-06-07 19:23:34 UTC
Created attachment 36611 [details]
Check for end of scheduler duration before the real sampling occurs

End test in delay method, if the computed delay would result in waiting till the end of the scheduler duration.
Comment 3 Felix Schumacher 2019-06-07 19:34:08 UTC
Created attachment 36612 [details]
Simple test plan to reproduce bug

The attached test plan will not execute any samplers with this fix and end the scheduler directly.

Without it, it will run for the scheduled period of time and execute the sampler afterwards.
Comment 4 Philippe Mouawad 2019-06-08 13:14:24 UTC
Hi Felix,
Can't we find a way to put this code in TimerService and add junit testing ?

For example, make it return Pair<Boolean, Long> (exit or not/sleep time).

As you noticed it, it's the 3rd bug around this feature.

Thanks
Comment 5 Felix Schumacher 2019-06-08 15:46:37 UTC
(In reply to Philippe Mouawad from comment #4)
> Hi Felix,
> Can't we find a way to put this code in TimerService and add junit testing ?
> 
> For example, make it return Pair<Boolean, Long> (exit or not/sleep time).

I would probably go for an Optional<Long> and define an empty Optional to mean, we should end the test, but I am not sure, whether it is a good idea to create another object instance for this. Same could be expressed by returning a negative long.

> 
> As you noticed it, it's the 3rd bug around this feature.

The missing test case is one of the reasons, why I haven't committed it yet.

> 
> Thanks
Comment 6 Philippe Mouawad 2019-06-08 17:20:08 UTC
(In reply to Felix Schumacher from comment #5)
> (In reply to Philippe Mouawad from comment #4)
> > Hi Felix,
> > Can't we find a way to put this code in TimerService and add junit testing ?
> > 
> > For example, make it return Pair<Boolean, Long> (exit or not/sleep time).
> 
> I would probably go for an Optional<Long> and define an empty Optional to
> mean, we should end the test, but I am not sure, whether it is a good idea
> to create another object instance for this. Same could be expressed by
> returning a negative long.
> 

+1 for this idea
> > 
> > As you noticed it, it's the 3rd bug around this feature.
> 
> The missing test case is one of the reasons, why I haven't committed it yet.

Great, thanks 
> 
> > 
> > Thanks
Comment 7 Vladimir Sitnikov 2019-06-08 18:22:20 UTC
> Optional<Long> and define an empty Optional to mean, we should end the test

As for me, it would look cryptic.
Comment 8 Felix Schumacher 2019-06-08 18:42:00 UTC
(In reply to Vladimir Sitnikov from comment #7)
> > Optional<Long> and define an empty Optional to mean, we should end the test
> 
> As for me, it would look cryptic.

Which one do you prefer? Or any other alternative?
Comment 9 Felix Schumacher 2019-06-09 11:53:05 UTC
Created attachment 36615 [details]
Check for end of scheduler duration before the real sampling occurs

The current logic of TimerService has now been changed to return -1, when delay would be too long for the end time.

JMeterThread#delay stops the current thread, when delay is -1 (which could be surprising, when a timer uses -1 to indicate no waiting).

A test case has been added to test the current behaviour (and an old one has been adapted to check for -1 in TimerService) and two tests have been added to complete all possible cases in adjustDelay.
Comment 10 Philippe Mouawad 2019-06-16 13:13:37 UTC
(In reply to Felix Schumacher from comment #9)
> Created attachment 36615 [details]
> Check for end of scheduler duration before the real sampling occurs
> 
> The current logic of TimerService has now been changed to return -1, when
> delay would be too long for the end time.
> 
> JMeterThread#delay stops the current thread, when delay is -1 (which could
> be surprising, when a timer uses -1 to indicate no waiting).
> 
> A test case has been added to test the current behaviour (and an old one has
> been adapted to check for -1 in TimerService) and two tests have been added
> to complete all possible cases in adjustDelay.

+1 for merging
Comment 11 Felix Schumacher 2019-06-16 18:24:19 UTC
commit a72e68ca69e874be020b1bda5ccf3a5756e14f21
AuthorDate: Sun Jun 16 19:55:54 2019 +0200

    At end of scheduler duration lots of Samplers gets executed at the same time
    
    The old behaviour lead to strange long sampling times at the end of scheduled
    test runs.
    
    Bugzilla Id: 63490
---
 .../org/apache/jmeter/threads/JMeterThread.java    |  14 +++
 .../org/apache/jmeter/timers/TimerService.java     |   4 +-
 .../apache/jmeter/threads/TestJMeterThread.java    | 104 ++++++++++++++++++++-
 .../org/apache/jmeter/timers/TimerServiceTest.java |  17 +++-
 xdocs/changes.xml                                  |   1 +
 5 files changed, 135 insertions(+), 5 deletions(-)

commit 2dc8f5b511dfb814c47402b348bc1aebe2991880
AuthorDate: Sun Jun 16 20:05:25 2019 +0200

    Make checkstyle happy
    
    Use braces with if statements and use uppercase L for long literals.
    A bit of whitespace police while we are here anyway.
    
    Bugzilla Id: 63490
Comment 12 Philippe Mouawad 2019-09-21 16:14:42 UTC
Closing after fix of Bug 63711 on nightly introduced by fix of this