On Oct 6, 2014, at 2:31 AM, sebb <seb...@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-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
