Mailing-List: contact dev-help@commons.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "Commons Developers List" <dev@commons.apache.org>
Received-SPF: pass (nike.apache.org: domain of jak-commons-dev@m.gmane.org
 designates 80.91.229.3 as permitted sender)
To: dev@commons.apache.org
From: =?UTF-8?B?SsO2cmc=?= Schaible <joerg.schaible@swisspost.com>
Subject: Re: [POOL-279] Thread concurrency issue in
 DefaultPooledObject.getIdleTimeMillis()
Date: Mon, 06 Oct 2014 09:34:14 +0200
Organization: Swiss Post Solutions
Lines: 83
Message-ID: <m0tglm$p28$1@ger.gmane.org>
References: 
 <CABOR3+yZ8iewEREcUegQmAdQ8+CrJGmu2wOa0CGPBveYmbBa9A@mail.gmail.com>
 <CAOGo0VaDUeU9WL7Cj5A8H1fhdwKc71ymr63Lhodm0bsLW+UHXA@mail.gmail.com>
 <CAOfDJ6sqJ4Ku7VCB5eAZFKv3QGQwf7rqsXzaXK=tOsiisrDrvw@mail.gmail.com>
 <CAOGo0Va+ksvqWJCUdObaS5=PeNqD15ktts8oAfGjBEx3jpik8A@mail.gmail.com>
 <172959CF-CD39-474D-ADA5-A81F314719B4@gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8Bit
User-Agent: KNode/4.12.5

Hi,

Jacopo Cappellato wrote:

> 
> On Oct 6, 2014, at 2:31 AM, sebb <sebbaz@gmail.com> wrote:
> 
>>> The only way to do it is to make
>>> lastReturnTime field thread-safe using locks.
>> 
>> Volatile does make the field thread-safe.
>> It's just a question of whether the JVM can re-order the statements
>> when volatile is used.
> 
> I think Xavier's comments are spot on.
> From what I know, the only JMM guarantee that applies in this context is
> the  following: "A write to a volatile field happens-before every
> subsequent read of that same field".
> 
> On the other hand the "program order rule", as correctly pointed out by
> Xavier, doesn't guarantee that the method will always return a positive
> value because the JVM.
> 
> I am aware of this issue in my fix. I have proposed this fix after I
> learned that, by design (to avoid synchronization), in this class it was
> relaxed the thread safe guarantees of a few (non critical) fields.
> 
> However, the original issue I have identified and exposed in my unit test
> could only happen under unlucky conditions (I don't know if they are
> theoretical or real because I don't know enough about how it is used); the
> "incomplete fix" that I have proposed makes the possibility of an error
> even more remote (I was not able to recreate it with my unit test).
> 
> In my opinion, if we do not want to implement a fix that involves thread
> synchronization, we could enhance the fix by including the suggestion from
> Bernd (returning null if the value is negative):
> 
> Index:
> src/main/java/org/apache/commons/pool2/impl/DefaultPooledObject.java
> ===================================================================
> --- src/main/java/org/apache/commons/pool2/impl/DefaultPooledObject.java
> (revision 1629471)
> +++ src/main/java/org/apache/commons/pool2/impl/DefaultPooledObject.java
> (working copy) @@ -85,7 +85,14 @@
>  
>      @Override
>      public long getIdleTimeMillis() {
> -        return System.currentTimeMillis() - lastReturnTime;
> +        // Take a copy of lastReturnTime to prevent the risk that
> +        // a concurrent thread update the value of lastReturnTime
> +        // between the time that System.currentTimeMillis()
> +        // is called and before the return value is computed: in
> +        // this case a negative value could be returned
> +        long lrTime = lastReturnTime;
> +        long idleTimeMillis = System.currentTimeMillis() - lrTime;
> +        return idleTimeMillis > 0? idleTimeMillis: 0;
>      }
>  
>      @Override
> 
> 
> What do you think?

looking at this,I suppose the situation is even worse. 
System.currentTimeMillis will return the elapsed seconds since 1. January 
1970 calculated based on the system time. Now, if the *system* time respects 
daylight saving, System.currentTimeMillis will also - so the value can jump 
back for an hour. System time is also affected by corrections from time 
daemons and might therefore decrease also.

The current recommendation is to use System.nanoTime which calculates the 
elapsed time based on a value calculated at VM startup and that is not 
affected by changes in system time (well, it should not, but some JDK 
versions have errors here). Nevertheless, even in this situation you're not 
safe from negative values in the method above. If you run multi-threaded on 
several cores, the individual cores are not always synchronized by 100% 
(especially if they run at different clock-speeds due power management). 
Depending on the system timer used by the Java VM you are affected.

So be always prepared to get a negative value for such time measuring 
methods.

- Jörg


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org