Re: [pool] pool: the future [WAS Re: [pool] synchronization issues in Pool]
On Sat, 2005-10-29 at 12:49 -0400, Sandy McArthur wrote: > Since you mentioned not breaking backwards compatibility I started > working on a fresh implementation which I think is coming along very > well and I intend to contribute back to the commons. sounds good moving to 2.0 gives a little more freedom but even so it makes sense to preserve as much compatibility as possible. > I've uploaded JavaDocs of what progress I've made so far. I figure I'm > about 45% done not including unit tests. See the org.mcarthur. > package: > > JavaDocs for only the public interface: > http://sandy.mcarthur.org/pool/Pool-Protected/ > > JavaDocs including private members: > http://sandy.mcarthur.org/pool/Pool-Private/ since there's substantial code and it's being developed elsewhere (rather than on list here), they'll be some paperwork required. the incubator projects acts as the contact point for this kind of thing. i'll make some enquiries about the process. (the ASF is currently trying to grow the new processes required to scale. things are a little chaotic, under-development and under-documented at the moment so patience may be required.) ASF code is used extensively and it's very important that we can demonstrate it's origins. > I am interested in becoming a commiter someday. cool the ASF can be a bit mysterious (when viewed from the outside) at times. few rules but lots of social conventions. if you get a minute or two, browse the foundation sites (http://www.apache.org and http://www.apache.org/dev) as well as jakarta. - robert - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [pool] pool: the future [WAS Re: [pool] synchronization issues in Pool]
Since you mentioned not breaking backwards compatibility I started working on a fresh implementation which I think is coming along very well and I intend to contribute back to the commons. I've uploaded JavaDocs of what progress I've made so far. I figure I'm about 45% done not including unit tests. See the org.mcarthur. package: JavaDocs for only the public interface: http://sandy.mcarthur.org/pool/Pool-Protected/ JavaDocs including private members: http://sandy.mcarthur.org/pool/Pool-Private/ I am interested in becoming a commiter someday. On 10/29/05, robert burrell donkin <[EMAIL PROTECTED]> wrote: > > re: 33264, 36719, 36904, 37153 > > > > these all suffer from issues with backward compatibility. i'd like to > > have a little think and then move the discussion to a new thread. > > there are a couple of issues which i needed a bit of time to think > about. > > IMHO these changes will improve pool (though some details need more > discussion) but are not backwards compatible with the existing 1.x > releases. so, if these are applied pool would need to be moved forward > to 2.0. moving to 2.0 open doors for other revisions. > > the recent synchronization fixes are important so one more 1.x release > would also be needed. > > i created a new branch from trunk (before committing the above patches). > the trunk version is now 2.0-dev. all of these patches (save the > collections one) have now been committed. i've haven't committed the > collections patch since i wonder whether it might be better to break the > dependency entirely. > > as always, opinions encouraged :) > > a more significant issue is that pool really seems short of an active > development community. there are patches from developers out there and > the existing committers haven't been very active for a while. so, > perhaps all that's needed is an effort to restart active development. > > i'm stretched pretty thin already. so, i'd need some help from > developers to kick start pool developmen. this might lead to 1.3 and 2.0 > releases. any volunteers interested? -- Sandy McArthur "Government big enough to supply everything you need is big enough to take everything you have ... The course of history shows that as a government grows, liberty decreases." -- Thomas Jefferson - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
[pool] pool: the future [WAS Re: [pool] synchronization issues in Pool]
On Thu, 2005-10-27 at 21:05 +0100, robert burrell donkin wrote: > On Mon, 2005-10-24 at 17:42 -0400, Sandy McArthur wrote: > > > > While I'm at it would it be desirable to transition to the privately > > > > head lock idiom > > > it might (however) stop a user doing something equivalent through > > > carelessness or naivity. so probably worth doing. would need to add a > > > note to the documentation since this would change the semantics. > > > opinions? > > > > I have a number of pending patches: http://tinyurl.com/cauwd that I'd > > like to see commited or rejected as I'm starting to feel no one cares > > about them and it's becoming a pain to maintain my own patched pool > > version. > > most of the committers listed in the project.xml aren't that active in > the commons any more :-/ > > re: 33264, 36719, 36904, 37153 > > these all suffer from issues with backward compatibility. i'd like to > have a little think and then move the discussion to a new thread. there are a couple of issues which i needed a bit of time to think about. IMHO these changes will improve pool (though some details need more discussion) but are not backwards compatible with the existing 1.x releases. so, if these are applied pool would need to be moved forward to 2.0. moving to 2.0 open doors for other revisions. the recent synchronization fixes are important so one more 1.x release would also be needed. i created a new branch from trunk (before committing the above patches). the trunk version is now 2.0-dev. all of these patches (save the collections one) have now been committed. i've haven't committed the collections patch since i wonder whether it might be better to break the dependency entirely. as always, opinions encouraged :) a more significant issue is that pool really seems short of an active development community. there are patches from developers out there and the existing committers haven't been very active for a while. so, perhaps all that's needed is an effort to restart active development. i'm stretched pretty thin already. so, i'd need some help from developers to kick start pool developmen. this might lead to 1.3 and 2.0 releases. any volunteers interested? - robert - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [pool] synchronization issues in Pool
Cool, looking forward to both the release of you tool as well as your report for commons transaction :) Cheers and thanks Oliver 2005/10/25, Mayur Naik <[EMAIL PROTECTED]>: > Hi Oliver, > > The tool is still a research prototype but I intend to release it in > the near future. I took a quick look and it seems like my tool might > not be very useful on the commons transaction code because that code > uses sophisticated locking primitives which the current version of my > tool does not handle and hence will produce lots of false alarms > (though we have plans to extend it to handle those primitives). > Nevertheless, I will run my tool on that code in the near future and > get back to you if I find any issues. > > Best, > -- Mayur > > On 10/23/05, Oliver Zeigermann <[EMAIL PROTECTED]> wrote: > > Hi Mayur, > > > > that really sounds like a cool tool. You results are also quite > > impressive. I actually can not comment on them, but would like to know > > if your tool is available anywhere? > > > > If not - and I suppose so - could you also run it on the commons > > transaction code where any race condition really would be very > > harmful. I already know of one that has been fixed in CVS, but not in > > a final release. > > > > Thanks in advance > > > > Oliver > > - > To unsubscribe, e-mail: [EMAIL PROTECTED] > For additional commands, e-mail: [EMAIL PROTECTED] > > - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [pool] synchronization issues in Pool
On Mon, 2005-10-24 at 17:42 -0400, Sandy McArthur wrote: > On 10/24/05, robert burrell donkin <[EMAIL PROTECTED]> wrote: > > unless some one beats me to it, once the patch is available i'll add > > myself to the dev list and review (as time permits). > > Patches submitted. committed. many thanks :) > > > While I'm at it would it be desirable to transition to the privately > > > head lock idiom > > it might (however) stop a user doing something equivalent through > > carelessness or naivity. so probably worth doing. would need to add a > > note to the documentation since this would change the semantics. > > opinions? > > I have a number of pending patches: http://tinyurl.com/cauwd that I'd > like to see commited or rejected as I'm starting to feel no one cares > about them and it's becoming a pain to maintain my own patched pool > version. most of the committers listed in the project.xml aren't that active in the commons any more :-/ re: 33264, 36719, 36904, 37153 these all suffer from issues with backward compatibility. i'd like to have a little think and then move the discussion to a new thread. > After that I'd be willing to implement the transition to the > private lock idiom. as sebb noted, this needs a little more consideration. i'd like to move it to a new thread. - robert - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [pool] synchronization issues in Pool
On Mon, 2005-10-24 at 17:56 -0700, Mayur Naik wrote: > Hi Oliver, > > The tool is still a research prototype but I intend to release it in > the near future. cool please drop us a note here when you do :) - robert - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [pool] synchronization issues in Pool
Hi Oliver, The tool is still a research prototype but I intend to release it in the near future. I took a quick look and it seems like my tool might not be very useful on the commons transaction code because that code uses sophisticated locking primitives which the current version of my tool does not handle and hence will produce lots of false alarms (though we have plans to extend it to handle those primitives). Nevertheless, I will run my tool on that code in the near future and get back to you if I find any issues. Best, -- Mayur On 10/23/05, Oliver Zeigermann <[EMAIL PROTECTED]> wrote: > Hi Mayur, > > that really sounds like a cool tool. You results are also quite > impressive. I actually can not comment on them, but would like to know > if your tool is available anywhere? > > If not - and I suppose so - could you also run it on the commons > transaction code where any race condition really would be very > harmful. I already know of one that has been fixed in CVS, but not in > a final release. > > Thanks in advance > > Oliver - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [pool] synchronization issues in Pool
On 10/24/05, robert burrell donkin <[EMAIL PROTECTED]> wrote: > unless some one beats me to it, once the patch is available i'll add > myself to the dev list and review (as time permits). Patches submitted. > > While I'm at it would it be desirable to transition to the privately > > head lock idiom > it might (however) stop a user doing something equivalent through > carelessness or naivity. so probably worth doing. would need to add a > note to the documentation since this would change the semantics. > opinions? I have a number of pending patches: http://tinyurl.com/cauwd that I'd like to see commited or rejected as I'm starting to feel no one cares about them and it's becoming a pain to maintain my own patched pool version. After that I'd be willing to implement the transition to the private lock idiom. -- Sandy McArthur "Government big enough to supply everything you need is big enough to take everything you have ... The course of history shows that as a government grows, liberty decreases." -- Thomas Jefferson - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [pool] synchronization issues in Pool
On 24/10/05, Sandy McArthur <[EMAIL PROTECTED]> wrote: > On 10/23/05, robert burrell donkin <[EMAIL PROTECTED]> wrote: > > are there any pool developers out there with time to pick this up? > > > > otherwise, we could probably do with a volunteer to go through and > > analyse these issues. anyone fancy taking a crack at this? > > I'm not a pool dev but I'll make some time to implement fixes for the > problems Mayur Naik found and submit patches. > > While I'm at it would it be desirable to transition to the privately > head lock idiom to defend against intentionally malicious code? eg to > defend against: > > synchronized (aPool) { > Thread.sleep(Integer.Max_Value); > } > > by making anything that synchronized on this synchronize on a private field. > +1 If possible, but one cannot always do this. If a class is conditionally thread-safe, i.e. it needs external synchronisation - then the appropriate lock object must be made available to clients. E.g. HashTable needs external synch for its Iterator [Bloch, Item 52] S. - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [pool] synchronization issues in Pool
On Sun, 2005-10-23 at 22:11 +0100, sebb wrote: > On 23/10/05, robert burrell donkin <[EMAIL PROTECTED]> wrote: > > On Sun, 2005-10-23 at 14:56 +0100, Stephen Colebourne wrote: > > > This looks interesting. I'll leave the pool comments to a pool > > > developer. However, could adding a lot more synchronoization could cause > > > other issues with locking and performance? > > > > probably. however, IMHO an object pool really needs to be thread safe. > > i've taken a quick look and think that perhaps it'd be better to fix any > > race conditions as the performance price that has to be paid for the > > design chosen. opinions? > > As Knuth put it "... premature optimization is the root of all evil". > And Jackson: > Rule 1. Don't do it > Rule 2. (for experts only). Don't do it yet - that is, not until you > have a perfectly clear and unoptimized solution. > > [From Bloch, Effective Java, Item 37] > > It's not only race conditions that might need to be fixed - > "Synchronization is required for reliable communication between > threads as well as for mutual exclusion." [Ibid, Item 48] > > He says that modern JVMs have much lower overheads for synchronisation > than the early releases, especially if the synchronisation is > uncontended, as would be the case for single-threaded access. > > So I'd suggest doing enough synch to ensure that the user can use all > the methods individually without needing to synchronise. Iterators > would likely still need external synch (except when running > single-threaded). +1 BTW if any existing apache committers feel like helping out on any commons components then i'd be glad to propose karma - robert - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [pool] synchronization issues in Pool
On Mon, 2005-10-24 at 14:32 -0400, Sandy McArthur wrote: > On 10/23/05, robert burrell donkin <[EMAIL PROTECTED]> wrote: > > are there any pool developers out there with time to pick this up? > > > > otherwise, we could probably do with a volunteer to go through and > > analyse these issues. anyone fancy taking a crack at this? > > I'm not a pool dev but I'll make some time to implement fixes for the > problems Mayur Naik found and submit patches. cool (in case you don't know the drill you might find some useful stuff in http://www.apache.org/dev/, http://jakarta.apache.org/site/getinvolved.html and http://jakarta.apache.org/commons/patches.html. please create a bugzilla for the patchs) unless some one beats me to it, once the patch is available i'll add myself to the dev list and review (as time permits). > While I'm at it would it be desirable to transition to the privately > head lock idiom to defend against intentionally malicious code? eg to > defend against: > > synchronized (aPool) { > Thread.sleep(Integer.Max_Value); > } > > by making anything that synchronized on this synchronize on a private field. i'm not too bothered about (that kind of) malicious code: there are too many ways the user of a library could subvert it. it might (however) stop a user doing something equivalent through carelessness or naivity. so probably worth doing. would need to add a note to the documentation since this would change the semantics. opinions? - robert - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [pool] synchronization issues in Pool
On 10/23/05, robert burrell donkin <[EMAIL PROTECTED]> wrote: > are there any pool developers out there with time to pick this up? > > otherwise, we could probably do with a volunteer to go through and > analyse these issues. anyone fancy taking a crack at this? I'm not a pool dev but I'll make some time to implement fixes for the problems Mayur Naik found and submit patches. While I'm at it would it be desirable to transition to the privately head lock idiom to defend against intentionally malicious code? eg to defend against: synchronized (aPool) { Thread.sleep(Integer.Max_Value); } by making anything that synchronized on this synchronize on a private field. -- Sandy McArthur "Government big enough to supply everything you need is big enough to take everything you have ... The course of history shows that as a government grows, liberty decreases." -- Thomas Jefferson - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [pool] synchronization issues in Pool
On 23/10/05, robert burrell donkin <[EMAIL PROTECTED]> wrote: > On Sun, 2005-10-23 at 14:56 +0100, Stephen Colebourne wrote: > > This looks interesting. I'll leave the pool comments to a pool > > developer. However, could adding a lot more synchronoization could cause > > other issues with locking and performance? > > probably. however, IMHO an object pool really needs to be thread safe. > i've taken a quick look and think that perhaps it'd be better to fix any > race conditions as the performance price that has to be paid for the > design chosen. opinions? As Knuth put it "... premature optimization is the root of all evil". And Jackson: Rule 1. Don't do it Rule 2. (for experts only). Don't do it yet - that is, not until you have a perfectly clear and unoptimized solution. [From Bloch, Effective Java, Item 37] It's not only race conditions that might need to be fixed - "Synchronization is required for reliable communication between threads as well as for mutual exclusion." [Ibid, Item 48] He says that modern JVMs have much lower overheads for synchronisation than the early releases, especially if the synchronisation is uncontended, as would be the case for single-threaded access. So I'd suggest doing enough synch to ensure that the user can use all the methods individually without needing to synchronise. Iterators would likely still need external synch (except when running single-threaded). > are there any pool developers out there with time to pick this up? > > otherwise, we could probably do with a volunteer to go through and > analyse these issues. anyone fancy taking a crack at this? > > - robert > > > - > To unsubscribe, e-mail: [EMAIL PROTECTED] > For additional commands, e-mail: [EMAIL PROTECTED] > > - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [pool] synchronization issues in Pool
On Sun, 2005-10-23 at 14:56 +0100, Stephen Colebourne wrote: > This looks interesting. I'll leave the pool comments to a pool > developer. However, could adding a lot more synchronoization could cause > other issues with locking and performance? probably. however, IMHO an object pool really needs to be thread safe. i've taken a quick look and think that perhaps it'd be better to fix any race conditions as the performance price that has to be paid for the design chosen. opinions? are there any pool developers out there with time to pick this up? otherwise, we could probably do with a volunteer to go through and analyse these issues. anyone fancy taking a crack at this? - robert - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [pool] synchronization issues in Pool
Hi Mayur, that really sounds like a cool tool. You results are also quite impressive. I actually can not comment on them, but would like to know if your tool is available anywhere? If not - and I suppose so - could you also run it on the commons transaction code where any race condition really would be very harmful. I already know of one that has been fixed in CVS, but not in a final release. Thanks in advance Oliver 2005/10/23, Mayur Naik <[EMAIL PROTECTED]>: > > Hello, > > I'm a PhD student in Computer Science at Stanford University, evaluating a > static race detection tool I'm developing on open source Java programs. > I ran it on the 5 implementations of pools in Apache Commons Pool, and > found some bugs. The output of the tool is here: > > http://suif.stanford.edu/~mhn/commons-pool-1.2/build/classes/PASS1/generic/index.html > http://suif.stanford.edu/~mhn/commons-pool-1.2/build/classes/PASS1/stack/index.html > http://suif.stanford.edu/~mhn/commons-pool-1.2/build/classes/PASS1/softref/index.html > http://suif.stanford.edu/~mhn/commons-pool-1.2/build/classes/PASS1/generic_keyed/index.html > http://suif.stanford.edu/~mhn/commons-pool-1.2/build/classes/PASS1/stack_keyed/index.html > > I will explain one race in detail. Following the [grouped by field] link > on the first link above and then the [details] link under the heading > "Races on field org.apache.commons.pool.BaseObjectPool: boolean closed" > leads you to: > > http://suif.stanford.edu/~mhn/commons-pool-1.2/build/classes/PASS1/generic/R8_trace.html > > Traversing the [up] links on this page yields at least one pair of paths > in the call graph along which a common lock is not held and hence a race > can occur if that pair of paths is executed by different threads: > > PATH 1: > > GenericHarness:27 > org.apache.commons.pool.impl.GenericObjectPool:853 > org.apache.commons.pool.BaseObjectPool:77 > > leading to a read access of the field 'closed' at > org.apache.commons.pool.BaseObjectPool:73 > > PATH 2: > > GenericHarness:33 > org.apache.commons.pool.impl.GenericObjectPool:901 > > leading to a write access of the field 'closed' at > org.apache.commons.pool.BaseObjectPool:62 > > [GenericHarness is a test harness that my tool automatically generates.] > > The above race is due to missing synchronization in method returnObject of > class GenericObjectPool. In fact, it can cause a null pointer exception: > > thread 1 calls the returnObject method which executes the assertOpen > method and finds the pool to be open. > > thread 2 calls the close method which sets _factory to null. > > thread 1 executes the addObjectToPool method which deferences _factory. > > Many other races are because parts of some methods implementing the pool > interfaces (ex. invalidateObject below) that access _factory are not > synchronized: > > public void invalidateObject(Object obj) throws Exception { > assertOpen(); > try { > _factory.destroyObject(obj); > } > finally { > synchronized(this) { > _numActive--; > notifyAll(); > } > } > } > > Perhaps, the developer expects the client methods implementing the factory > interface to be synchronized. If this is indeed the case, it should > perhaps be documented somewhere so that unwitting users don't implement > the factory interface without synchronization. Even if users implement > the factory interface with synchronization, there is a problem: if one > thread executes the invalidateObject method and another executes the close > method (which sets _factory to null), then there can be a null pointer > exception similar to the scenario outlined above. > > Most of the races I found are harmful (ex. causing null pointer > exceptions) if the close method is called concurrently with some other > pool interface method. Perhaps, it is highly unlikely that a client would > do that. However, since the pool interface is intended to be thread-safe, > I think it's implementations should handle this scenario gracefully. > > I have summarized below all possible bugs I found by inspecting the above > reports generated by my race detection tool. I would be happy if you can > confirm/refute these bugs. > > Thanks! > -- Mayur > > org.apache.commons.pool.impl.GenericObjectPool > == > > The returnObject(Object) and addObject() methods must be synchronized. > The synchronization inside the body of the method addObjectToPool is then > unnecessary since all callers of this method are synchronized. > > The entire invalidateObject(Object) method must be synchronized, not just > parts of it. > > The entire borrowObject() method must be synchronized, not just parts of > it. Note that there are dereferences of _factory in the unsynchronized > portions of this method, so there will be a null pointer dereference if it > is called concurrently with the close method. Once the entire > borrowObject method is synchronize
Re: [pool] synchronization issues in Pool
On Sun, 2005-10-23 at 04:12 -0700, Mayur Naik wrote: > Hello, > > I'm a PhD student in Computer Science at Stanford University, > evaluating a static race detection tool I'm developing on open source > Java programs. an open source static race detector? if so, then it sounds very cool :) if you have an long term URL for your project then please post it. - robert - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [pool] synchronization issues in Pool
On 23/10/05, Stephen Colebourne <[EMAIL PROTECTED]> wrote: > My main question is can these tests be run against any class? We're > particularly stuck with > http://issues.apache.org/bugzilla/show_bug.cgi?id=32573 collections > LRUMap at present where a bug can easily be reproduced when you don't > sync, but not at all when you do sync. Yet we have bugs reported! So, is The issue is not clear to me on the exact circumstances when the reported bugs do occur: - do problems ever occur in single-threaded use? - if problems only occur in multi-threaded use, is access synchronized? and, just as important: - are all uses of Iterators synchronized on the map? See for example: http://java.sun.com/j2se/1.4.2/docs/api/java/util/Collections.html#synchronizedMap(java.util.Map) HTH. S. - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [pool] synchronization issues in Pool
This looks interesting. I'll leave the pool comments to a pool developer. However, could adding a lot more synchronoization could cause other issues with locking and performance? My main question is can these tests be run against any class? We're particularly stuck with http://issues.apache.org/bugzilla/show_bug.cgi?id=32573 collections LRUMap at present where a bug can easily be reproduced when you don't sync, but not at all when you do sync. Yet we have bugs reported! So, is your tool an internal project for your PhD, or intended to be for general open source usage. Stephen Mayur Naik wrote: Hello, I'm a PhD student in Computer Science at Stanford University, evaluating a static race detection tool I'm developing on open source Java programs. I ran it on the 5 implementations of pools in Apache Commons Pool, and found some bugs. The output of the tool is here: http://suif.stanford.edu/~mhn/commons-pool-1.2/build/classes/PASS1/generic/index.html http://suif.stanford.edu/~mhn/commons-pool-1.2/build/classes/PASS1/stack/index.html http://suif.stanford.edu/~mhn/commons-pool-1.2/build/classes/PASS1/softref/index.html http://suif.stanford.edu/~mhn/commons-pool-1.2/build/classes/PASS1/generic_keyed/index.html http://suif.stanford.edu/~mhn/commons-pool-1.2/build/classes/PASS1/stack_keyed/index.html I will explain one race in detail. Following the [grouped by field] link on the first link above and then the [details] link under the heading "Races on field org.apache.commons.pool.BaseObjectPool: boolean closed" leads you to: http://suif.stanford.edu/~mhn/commons-pool-1.2/build/classes/PASS1/generic/R8_trace.html Traversing the [up] links on this page yields at least one pair of paths in the call graph along which a common lock is not held and hence a race can occur if that pair of paths is executed by different threads: PATH 1: GenericHarness:27 org.apache.commons.pool.impl.GenericObjectPool:853 org.apache.commons.pool.BaseObjectPool:77 leading to a read access of the field 'closed' at org.apache.commons.pool.BaseObjectPool:73 PATH 2: GenericHarness:33 org.apache.commons.pool.impl.GenericObjectPool:901 leading to a write access of the field 'closed' at org.apache.commons.pool.BaseObjectPool:62 [GenericHarness is a test harness that my tool automatically generates.] The above race is due to missing synchronization in method returnObject of class GenericObjectPool. In fact, it can cause a null pointer exception: thread 1 calls the returnObject method which executes the assertOpen method and finds the pool to be open. thread 2 calls the close method which sets _factory to null. thread 1 executes the addObjectToPool method which deferences _factory. Many other races are because parts of some methods implementing the pool interfaces (ex. invalidateObject below) that access _factory are not synchronized: public void invalidateObject(Object obj) throws Exception { assertOpen(); try { _factory.destroyObject(obj); } finally { synchronized(this) { _numActive--; notifyAll(); } } } Perhaps, the developer expects the client methods implementing the factory interface to be synchronized. If this is indeed the case, it should perhaps be documented somewhere so that unwitting users don't implement the factory interface without synchronization. Even if users implement the factory interface with synchronization, there is a problem: if one thread executes the invalidateObject method and another executes the close method (which sets _factory to null), then there can be a null pointer exception similar to the scenario outlined above. Most of the races I found are harmful (ex. causing null pointer exceptions) if the close method is called concurrently with some other pool interface method. Perhaps, it is highly unlikely that a client would do that. However, since the pool interface is intended to be thread-safe, I think it's implementations should handle this scenario gracefully. I have summarized below all possible bugs I found by inspecting the above reports generated by my race detection tool. I would be happy if you can confirm/refute these bugs. Thanks! -- Mayur org.apache.commons.pool.impl.GenericObjectPool == The returnObject(Object) and addObject() methods must be synchronized. The synchronization inside the body of the method addObjectToPool is then unnecessary since all callers of this method are synchronized. The entire invalidateObject(Object) method must be synchronized, not just parts of it. The entire borrowObject() method must be synchronized, not just parts of it. Note that there are dereferences of _factory in the unsynchronized portions of this method, so there will be a null pointer dereference if it is called concurrently with the close method. Once the entire borrowObject method is synchronized, you can also move the assertOpen() call at the st