commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jacopo Cappellato <>
Subject Re: [pool] time to cut 2.3
Date Thu, 02 Oct 2014 17:00:44 GMT
Thank you Phil and Gary for the responses.

Phil, the approach you described makes sense to me and I agree: even if part of the fields
of DefaultPooledObject are not formally thread-safe, the state transitions are preserved and
this is the most important function of this class.
I understand that it doesn't make sense to make it formally thread safe if this may penalize
I have to admit that I have implemented a Junit test to try to break the  getIdleTimeMillis()
method but I was unable to make it fail :-) I am still convinced that it could happen but
it is very difficult to recreate and it may be just a theoretical issue that will never happen
in the real world.
However, while studying this class I did some minor cleanups that may be irrelevant at this
point; but I am sharing them with you just in case (see patch below):
1) minor change to getIdleTimeMillis() to make even more rare the possibility of a negative
number being returned by the method
2) removed an if block that was unnecessary
3) improved the synchronization of the date formatting: no need to use the "format" field
as the lock; my change will not modify the behavior of the class but, in my opinion, it makes
the code a bit more readable and concise

This is all about this class, let me know if you want me to create a ticket; I will try to
analyze, as you have recommended, GOP / GKOP for thread-safety as soon as I have some free

Thanks for your time and good luck for the new release.


Index: src/main/java/org/apache/commons/pool2/impl/
--- src/main/java/org/apache/commons/pool2/impl/	(revision 1629013)
+++ src/main/java/org/apache/commons/pool2/impl/	(working copy)
@@ -85,7 +85,8 @@
     public long getIdleTimeMillis() {
-        return System.currentTimeMillis() - lastReturnTime;
+        long lrTime = lastReturnTime;
+        return System.currentTimeMillis() - lrTime;
@@ -216,9 +217,7 @@
                 state == PooledObjectState.RETURNING) {
             state = PooledObjectState.IDLE;
             lastReturnTime = System.currentTimeMillis();
-            if (borrowedBy != null) {
-                borrowedBy = null;
-            }
+            borrowedBy = null;
             return true;
@@ -311,12 +310,8 @@
         // Override getMessage to avoid creating objects and formatting
         // dates unless the log message will actually be used.
-        public String getMessage() {
-            String msg;
-            synchronized(format) {
-                msg = format.format(new Date(_createdTime));
-            }
-            return msg;
+        public synchronized String getMessage() {
+            return format.format(new Date(_createdTime));

On Sep 27, 2014, at 8:25 PM, Phil Steitz <> wrote:

> On 9/27/14 10:16 AM, Jacopo Cappellato wrote:
>> On Sep 27, 2014, at 6:46 PM, Gary Gregory <> wrote:
>>> It is not enough to set them volatile because the class has some invariants that
include more than one field.
>> For example, under unlucky thread concurrency the method getIdleTimeMillis() could
return a negative value.
> Thanks for jumping in!
> See last response re DefaultPooledObject#getIdleTimeMillis.  One
> thing to keep in mind when digging in to this code is that
> PooledObjectState is used internally by the pool implementations to
> effectively gate access to PooledObject instance data.  Clients do
> not in general have direct access to these objects.  While monitor
> contention would not be a practical issue, the overhead of adding
> sync locking to make DefaultPooledObject strictly threadsafe might
> have performance impacts.  I would personally favor adding a
> disclaimer to the javadoc rather than adding more sync to this
> object.  Alternatively, we could add a synchronized version of the
> class that factories could use, or just point out that if your
> factories require (for reasons not obvious to me) more sync
> protection on these objects you can subclass.  The PooledObject
> interface was introduced so that factories could do exactly this
> kind of thing.
> If on the other hand you can identify thread-safety issues that
> could impact GOP / GKOP usage, we certainly want to address those. 
> The issue above is an example.  If my analysis is incorrect and you
> can engineer a test case showing incorrect stats being reported by
> the pool, then please do open a ticket.
> Thanks again for looking at the code.
> Phil
>> Jacopo
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail:
>> For additional commands, e-mail:
> ---------------------------------------------------------------------
> To unsubscribe, e-mail:
> For additional commands, e-mail:

To unsubscribe, e-mail:
For additional commands, e-mail:

View raw message