commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jacopo Cappellato <jacopo.cappell...@gmail.com>
Subject Re: [POOL-279] Thread concurrency issue in DefaultPooledObject.getIdleTimeMillis()
Date Mon, 06 Oct 2014 05:21:40 GMT

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?

Jacopo


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


Mime
View raw message