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] 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
performance.
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
time.

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

Jacopo


Index: src/main/java/org/apache/commons/pool2/impl/DefaultPooledObject.java
===================================================================
--- src/main/java/org/apache/commons/pool2/impl/DefaultPooledObject.java	(revision 1629013)
+++ src/main/java/org/apache/commons/pool2/impl/DefaultPooledObject.java	(working copy)
@@ -85,7 +85,8 @@
 
     @Override
     public long getIdleTimeMillis() {
-        return System.currentTimeMillis() - lastReturnTime;
+        long lrTime = lastReturnTime;
+        return System.currentTimeMillis() - lrTime;
     }
 
     @Override
@@ -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.
         @Override
-        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 <phil.steitz@gmail.com> wrote:

> On 9/27/14 10:16 AM, Jacopo Cappellato wrote:
>> On Sep 27, 2014, at 6:46 PM, Gary Gregory <garydgregory@gmail.com> 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: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>> 
>> 
> 
> 
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
> 


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


Mime
View raw message