On Oct 2, 2014, at 7:24 PM, Mark Thomas <ma...@apache.org> wrote: > 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 :-)
An update: I was finally able to break the method; I have created a ticket and attached to it the fix (same as #1 below) and the Junit test that should fail most of the times without the fix. Here is the ticket: https://issues.apache.org/jira/browse/POOL-279 I hope I set the proper fields in it. Thanks, Jacopo >> 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.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 >> > > > --------------------------------------------------------------------- > 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
