commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dirk Verbeeck <dirk.verbe...@pandora.be>
Subject Re: [pool] faulty change of synchronization? - Re: cvs commit: jakarta-commons/pool/src/java/org/apache/commons/pool/impl GenericObjectPool.java
Date Fri, 15 Aug 2003 12:40:30 GMT
Hi Marc

Very nice link you gave, very interesting.
The changes I'm doing is a work in progress. Before releasing it we will 
do a formal review including a vote,
beta test on release canidate and then the actual release. You will get 
enough time to review.

But the code is not very good yet. There should be a simpler way to 
solve these synchronization issues.
I'll have another go at it...

Dirk


Marc Slemko wrote:

>jOn Wed, 13 Aug 2003, Dirk Verbeeck wrote:
>
>  
>
>>_numActive is an int so the synchronized block wasn't needed.
>>But the extra "synchronized" is in the case where a new object
>>has to be created and not in the critical "get from pool" code path.
>>
>>All the success code paths have now one synchronized block.
>>I'll leave it as is for now.
>>    
>>
>
>No, you need the synchronization (or some other thread safe approach),
>otherwise you risk all sorts of errors.  There are a very limited
>set of cases where you can avoid synchronization when you need concurrent
>access from multiple threads.
>
>Synchronization in java handles both mutual exclusion and visibility.
>There is no guarantee that a change made in one thread to a variable
>is visible to another without synchronization.
>
>See some of the references linked to from:
>
>	http://www.cs.umd.edu/~pugh/java/memoryModel/
>
>There appear to be all sorts of race conditions in borrowObject() now...
>_numActive, accesses to _pool, _maxActive... you can't check the value of
>a counter outside a synchronized block then only synchronize to modify it!
>
>I think you need to step back and reconsider what problems these changes
>are trying to fix.  You need to find the real operation that is
>bottlenecking things (which for connection pools would normally be calling
>into the JDBC driver to open a connection), and move that operation
>outside the synchronized block without doing unsafe things.
>
>Now, ideally that whole thing would be reworked even further to have the
>connection opener in a seperate thread to allow the getConnection() to
>time out after a specified timeout, while it keeps trying to open the
>connection in the background.  In fact, if you do that you may be able to
>just leave the whole method synchronized and wait on the opener thread,
>but there are some extra efforts required to ensure fairness, etc. that
>may make that more complex.
>
>I strongly recommend you roll back the synchronization changes unless
>someone who fully understands what has to be synchronized and why can
>review possible fixes, I don't have time just now, maybe next week; the
>current state is _NOT_ good enough.
>
>---------------------------------------------------------------------
>To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
>For additional commands, e-mail: commons-dev-help@jakarta.apache.org
>
>
>
>
>  
>




Mime
View raw message