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: <CAH9fUpZBz7w40UfdhyJs=csRn-xNJev-KdOvcs1+B7x-xUB=xQ@mail.gmail.com>
References: <20170301222242.841693A0112@svn01-us-west.apache.org>
 <CAOGo0VYsZFVjFUfCxHPKb4YzFwcqoeNzWTnhP2LVewoveCuhtw@mail.gmail.com> <CAH9fUpZBz7w40UfdhyJs=csRn-xNJev-KdOvcs1+B7x-xUB=xQ@mail.gmail.com>
From: sebb <sebbaz@gmail.com>
Date: Thu, 2 Mar 2017 11:06:20 +0000
Message-ID: <CAOGo0VaAvhEcJbHpOeWwi_4UwkQZ4_3tLL-qH8ODV_C6ye7MHg@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 11:06:58 -0000

On 2 March 2017 at 06:52, Philippe Mouawad <philippe.mouawad@gmail.com> wrote:
> On Thursday, March 2, 2017, sebb <sebbaz@gmail.com> wrote:
>
>> On 1 March 2017 at 22:22,  <pmouawad@apache.org <javascript:;>> 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.
>
> Yes, I'll fix it
>
>>
>> > +    }
>> > +
>> > +    /**
>> > +     * @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.
>
> Inthe future the service might have state.

Or it might not.

But if it does acquire state, then it's likely that it cannot be a singleton.

In the meantime, it's harder to use.

Besides, the IODH idiom is intended for classes that include expensive
initialisation.

> Also

??

>
>>
>> > +
>> > +    /**
>> > +     * 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>
>> >
>> >
>>
>
>
> --
> Cordialement.
> Philippe Mouawad.