commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mark Thomas <ma...@apache.org>
Subject Re: [pool] time to cut 2.3
Date Thu, 02 Oct 2014 17:24:18 GMT
On 02/10/2014 18:00, Jacopo Cappellato wrote:
> 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;
>      }

+0

>      @Override
> @@ -216,9 +217,7 @@
>                  state == PooledObjectState.RETURNING) {
>              state = PooledObjectState.IDLE;
>              lastReturnTime = System.currentTimeMillis();
> -            if (borrowedBy != null) {
> -                borrowedBy = null;
> -            }
> +            borrowedBy = null;
>              return true;
>          }

+1

> @@ -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));
>          }
>      }
>  }

-1. The sync should be as narrow as possible and only on the object that
needs to be sync'd. Move the sync to the method changes the lock object
to the instance and will increase the possibility of contention for
those actions that require the lock to be on the instance.

Mark


> 
> 
> 
> 
> 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
> 


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


Mime
View raw message