Mailing-List: contact dev-help@jmeter.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@jmeter.apache.org
MIME-Version: 1.0
In-Reply-To: <20170301222242.841693A0112@svn01-us-west.apache.org>
References: <20170301222242.841693A0112@svn01-us-west.apache.org>
From: sebb <sebbaz@gmail.com>
Date: Thu, 2 Mar 2017 01:14:04 +0000
Message-ID: <CAOGo0VYsZFVjFUfCxHPKb4YzFwcqoeNzWTnhP2LVewoveCuhtw@mail.gmail.com>
Subject: Re: svn commit: r1785057 - in /jmeter/trunk: src/components/org/apache/jmeter/sampler/TestAction.java
 src/components/org/apache/jmeter/timers/TimerService.java src/core/org/apache/jmeter/threads/JMeterThread.java
 xdocs/changes.xml
To: dev@jmeter.apache.org
Content-Type: text/plain; charset=UTF-8
archived-at: Thu, 02 Mar 2017 01:14:15 -0000

On 1 March 2017 at 22:22,  <pmouawad@apache.org> wrote:
> Author: pmouawad
> Date: Wed Mar  1 22:22:42 2017
> New Revision: 1785057
>
> URL: http://svn.apache.org/viewvc?rev=1785057&view=rev
> Log:
> Bug 60797 - TestAction in pause mode can last beyond configured duration of test
> Bugzilla Id: 60797
>
> Added:
>     jmeter/trunk/src/components/org/apache/jmeter/timers/TimerService.java   (with props)
> Modified:
>     jmeter/trunk/src/components/org/apache/jmeter/sampler/TestAction.java
>     jmeter/trunk/src/core/org/apache/jmeter/threads/JMeterThread.java
>     jmeter/trunk/xdocs/changes.xml
>
> Modified: jmeter/trunk/src/components/org/apache/jmeter/sampler/TestAction.java
> URL: http://svn.apache.org/viewvc/jmeter/trunk/src/components/org/apache/jmeter/sampler/TestAction.java?rev=1785057&r1=1785056&r2=1785057&view=diff
> ==============================================================================
> --- jmeter/trunk/src/components/org/apache/jmeter/sampler/TestAction.java (original)
> +++ jmeter/trunk/src/components/org/apache/jmeter/sampler/TestAction.java Wed Mar  1 22:22:42 2017
> @@ -33,6 +33,7 @@ import org.apache.jmeter.testelement.pro
>  import org.apache.jmeter.testelement.property.StringProperty;
>  import org.apache.jmeter.threads.JMeterContext;
>  import org.apache.jmeter.threads.JMeterContextService;
> +import org.apache.jmeter.timers.TimerService;
>  import org.slf4j.Logger;
>  import org.slf4j.LoggerFactory;
>
> @@ -45,6 +46,8 @@ public class TestAction extends Abstract
>
>      private static final Logger log = LoggerFactory.getLogger(TestAction.class);
>
> +    private static final TimerService TIMER_SERVICE = TimerService.getInstance();
> +
>      private static final long serialVersionUID = 241L;
>
>      private static final Set<String> APPLIABLE_CONFIG_CLASSES = new HashSet<>(
> @@ -121,7 +124,7 @@ public class TestAction extends Abstract
>          try {
>              pauseThread = Thread.currentThread();
>              if(millis>0) {
> -                TimeUnit.MILLISECONDS.sleep(millis);
> +                TimeUnit.MILLISECONDS.sleep(TIMER_SERVICE.adjustDelay(millis));
>              } else if(millis<0) {
>                  throw new IllegalArgumentException("Configured sleep is negative:"+millis);
>              } // else == 0 we do nothing
>
> Added: jmeter/trunk/src/components/org/apache/jmeter/timers/TimerService.java
> URL: http://svn.apache.org/viewvc/jmeter/trunk/src/components/org/apache/jmeter/timers/TimerService.java?rev=1785057&view=auto
> ==============================================================================
> --- jmeter/trunk/src/components/org/apache/jmeter/timers/TimerService.java (added)
> +++ jmeter/trunk/src/components/org/apache/jmeter/timers/TimerService.java Wed Mar  1 22:22:42 2017
> @@ -0,0 +1,74 @@
> +/*
> + * Licensed to the Apache Software Foundation (ASF) under one or more
> + * contributor license agreements.  See the NOTICE file distributed with
> + * this work for additional information regarding copyright ownership.
> + * The ASF licenses this file to You under the Apache License, Version 2.0
> + * (the "License"); you may not use this file except in compliance with
> + * the License.  You may obtain a copy of the License at
> + *
> + *   http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + *
> + */
> +
> +package org.apache.jmeter.timers;
> +
> +import org.apache.jmeter.threads.JMeterContextService;
> +import org.apache.jmeter.threads.JMeterThread;
> +
> +/**
> + * Manages logic related to timers and pauses
> + * @since 3.2
> + */
> +public class TimerService {
> +
> +    private TimerService() {
> +        super();
> +    }
> +
> +    /**
> +     * Initialization On Demand Holder pattern
> +     */
> +    private static class TimerServiceHolder {
> +        public static final TimerService INSTANCE = new TimerService();

This should be package protected; no point in being public.

> +    }
> +
> +    /**
> +     * @return ScriptEngineManager singleton
> +     */
> +    public static TimerService getInstance() {
> +        return TimerServiceHolder.INSTANCE;
> +    }

But why is the IODH idiom used here?

It looks just like a utility class.

The methods could just be static.

> +
> +    /**
> +     * Adjust delay so that initialDelay does not exceed end of test
> +     * @param delay initial delay in millis
> +     * @return initialDelay or adjusted delay
> +     */
> +    public long adjustDelay(final long initialDelay) {
> +        JMeterThread thread = JMeterContextService.getContext().getThread();
> +        long endTime = thread != null ? thread.getEndTime() : 0;
> +        return adjustDelay(initialDelay, endTime);
> +    }
> +
> +    /**
> +     * Adjust delay so that initialDelay does not exceed end of test
> +     * @param initialDelay initial delay in millis
> +     * @param endTime End time of JMeterThread
> +     * @return initialDelay or adjusted delay
> +     */
> +    public long adjustDelay(final long initialDelay, long endTime) {
> +        if (endTime > 0) {
> +            long now = System.currentTimeMillis();
> +            if(now + initialDelay > endTime) {
> +                return endTime - now;
> +            }
> +        }
> +        return initialDelay;
> +    }
> +}
>
> Propchange: jmeter/trunk/src/components/org/apache/jmeter/timers/TimerService.java
> ------------------------------------------------------------------------------
>     svn:eol-style = native
>
> Propchange: jmeter/trunk/src/components/org/apache/jmeter/timers/TimerService.java
> ------------------------------------------------------------------------------
>     svn:mime-type = text/plain
>
> Modified: jmeter/trunk/src/core/org/apache/jmeter/threads/JMeterThread.java
> URL: http://svn.apache.org/viewvc/jmeter/trunk/src/core/org/apache/jmeter/threads/JMeterThread.java?rev=1785057&r1=1785056&r2=1785057&view=diff
> ==============================================================================
> --- jmeter/trunk/src/core/org/apache/jmeter/threads/JMeterThread.java (original)
> +++ jmeter/trunk/src/core/org/apache/jmeter/threads/JMeterThread.java Wed Mar  1 22:22:42 2017
> @@ -47,6 +47,7 @@ import org.apache.jmeter.testelement.Tes
>  import org.apache.jmeter.testelement.TestIterationListener;
>  import org.apache.jmeter.testelement.ThreadListener;
>  import org.apache.jmeter.timers.Timer;
> +import org.apache.jmeter.timers.TimerService;
>  import org.apache.jmeter.util.JMeterUtils;
>  import org.apache.jorphan.collections.HashTree;
>  import org.apache.jorphan.collections.HashTreeTraverser;
> @@ -79,6 +80,7 @@ public class JMeterThread implements Run
>
>      private static final float TIMER_FACTOR = JMeterUtils.getPropDefault("timer.factor", 1.0f);
>
> +    private static final TimerService TIMER_SERVICE = TimerService.getInstance();
>      /**
>       * 1 as float
>       */
> @@ -842,10 +844,7 @@ public class JMeterThread implements Run
>                  if(scheduler) {
>                      // We reduce pause to ensure end of test is not delayed by a sleep ending after test scheduled end
>                      // See Bug 60049
> -                    long now = System.currentTimeMillis();
> -                    if(now + totalDelay > endTime) {
> -                        totalDelay = endTime - now;
> -                    }
> +                    totalDelay = TIMER_SERVICE.adjustDelay(totalDelay, endTime);
>                  }
>                  TimeUnit.MILLISECONDS.sleep(totalDelay);
>              } catch (InterruptedException e) {
>
> Modified: jmeter/trunk/xdocs/changes.xml
> URL: http://svn.apache.org/viewvc/jmeter/trunk/xdocs/changes.xml?rev=1785057&r1=1785056&r2=1785057&view=diff
> ==============================================================================
> --- jmeter/trunk/xdocs/changes.xml [utf-8] (original)
> +++ jmeter/trunk/xdocs/changes.xml [utf-8] Wed Mar  1 22:22:42 2017
> @@ -276,6 +276,7 @@ JMeter now requires Java 8. Ensure you u
>      <li><bug>60730</bug>The JSON PostProcessor should set the <code>_ALL</code> variable even if the JSON path matches only once.</li>
>      <li><bug>60747</bug>Response Assertion : Add Request Headers to <code>Field to Test</code></li>
>      <li><bug>60763</bug>XMLAssertion should not leak errors to console</li>
> +    <li><bug>60797</bug>TestAction in pause mode can last beyond configured duration of test</li>
>  </ul>
>
>  <h3>Functions</h3>
>
>