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