On 20/04/2010, Phil Steitz <phil.ste...@gmail.com> wrote: > sebb wrote: > > On 18/04/2010, Phil Steitz <phil.ste...@gmail.com> wrote: > >> I will revert this if there are objections or failures on other > >> JDKs. If there are no objections or failures, I will do the same > >> for GKOP tests. All current tests pass for me, but the added > >> synchronization may make them less sensitive, so we may wish > >> document lack of thread safety instead and revert. I can see both > >> sides of this. > >> > >> I did not fully synchronize any of the factory methods, even though > >> for activate and validate in their current form, the effect would be > >> the same. The reason for this is that we may want to add > >> configurable latency to these methods and we don't want the waits to > >> lock the factory. > >> > >> Thanks in advance for review / comments or even vetos ;) > > > > I'm not sure that the patch adds anything - only the set() methods are > > synchronised, so the new version is not thread-safe under all > > conditions: > > > > If set() and get() (implicit in this case, as the fields are read > > directly) can be called from different *active* threads, then synch. > > will also be needed for read accesses to ensure safe publication of > > the value. > > > My intent was to ensure that the factory methods got consistent data > on activation. The direct reads should all be in synch blocks. >
True. Sorry, should have spotted that. > > > > On the other hand, if the set() methods are only ever called before > > the threads that use the instance are created, then there is no need > > to synch. the set() methods as thread start acts as a synch. lock and > > ensures safe publication. > > > That is in fact the case in the tests now, so could be the change is > pointless. Thanks for reviewing. > If the settings are only changed immediately after construction, it might be worth considering using final fields instead and not have to worry about safe publication. This could certainly be done for the "valid" fields. > > Phil > > > > >> Phil > >> > >> > >> pste...@apache.org wrote: > >> > Author: psteitz > >> > Date: Sun Apr 18 15:59:27 2010 > >> > New Revision: 935362 > >> > > >> > URL: http://svn.apache.org/viewvc?rev=935362&view=rev > >> > Log: > >> > Made SimpleFactory threadsafe. > >> > > >> > Modified: > >> > > commons/proper/pool/trunk/src/test/org/apache/commons/pool/impl/TestGenericObjectPool.java > >> > > >> > Modified: > commons/proper/pool/trunk/src/test/org/apache/commons/pool/impl/TestGenericObjectPool.java > >> > URL: > http://svn.apache.org/viewvc/commons/proper/pool/trunk/src/test/org/apache/commons/pool/impl/TestGenericObjectPool.java?rev=935362&r1=935361&r2=935362&view=diff > >> > > ============================================================================== > >> > --- > commons/proper/pool/trunk/src/test/org/apache/commons/pool/impl/TestGenericObjectPool.java > (original) > >> > +++ > commons/proper/pool/trunk/src/test/org/apache/commons/pool/impl/TestGenericObjectPool.java > Sun Apr 18 15:59:27 2010 > >> > @@ -1332,26 +1332,26 @@ public class TestGenericObjectPool exten > >> > evenValid = evalid; > >> > oddValid = ovalid; > >> > } > >> > - void setValid(boolean valid) { > >> > + public synchronized void setValid(boolean valid) { > >> > setEvenValid(valid); > >> > setOddValid(valid); > >> > } > >> > - void setEvenValid(boolean valid) { > >> > + public synchronized void setEvenValid(boolean valid) { > >> > evenValid = valid; > >> > } > >> > - void setOddValid(boolean valid) { > >> > + public synchronized void setOddValid(boolean valid) { > >> > oddValid = valid; > >> > } > >> > - public void setThrowExceptionOnPassivate(boolean bool) { > >> > + public synchronized void setThrowExceptionOnPassivate(boolean > bool) { > >> > exceptionOnPassivate = bool; > >> > } > >> > - public void setMaxActive(int maxActive) { > >> > + public synchronized void setMaxActive(int maxActive) { > >> > this.maxActive = maxActive; > >> > } > >> > - public void setDestroyLatency(long destroyLatency) { > >> > + public synchronized void setDestroyLatency(long > destroyLatency) { > >> > this.destroyLatency = destroyLatency; > >> > } > >> > - public void setMakeLatency(long makeLatency) { > >> > + public synchronized void setMakeLatency(long makeLatency) { > >> > this.makeLatency = makeLatency; > >> > } > >> > public Object makeObject() { > >> > @@ -1365,36 +1365,70 @@ public class TestGenericObjectPool exten > >> > if (makeLatency > 0) { > >> > doWait(makeLatency); > >> > } > >> > - return String.valueOf(makeCounter++); > >> > + int counter; > >> > + synchronized(this) { > >> > + counter = makeCounter++; > >> > + } > >> > + return String.valueOf(counter); > >> > } > >> > public void destroyObject(Object obj) throws Exception { > >> > - if (destroyLatency > 0) { > >> > + boolean wait; > >> > + boolean hurl; > >> > + synchronized(this) { > >> > + wait = destroyLatency > 0; > >> > + hurl = exceptionOnDestroy; > >> > + } > >> > + if (wait) { > >> > doWait(destroyLatency); > >> > } > >> > synchronized(this) { > >> > activeCount--; > >> > } > >> > - if (exceptionOnDestroy) { > >> > + if (hurl) { > >> > throw new Exception(); > >> > } > >> > } > >> > public boolean validateObject(Object obj) { > >> > - if (enableValidation) { > >> > - return validateCounter++%2 == 0 ? evenValid : > oddValid; > >> > + boolean validate; > >> > + boolean evenTest; > >> > + boolean oddTest; > >> > + int counter; > >> > + synchronized(this) { > >> > + validate = enableValidation; > >> > + evenTest = evenValid; > >> > + oddTest = oddValid; > >> > + counter = validateCounter++; > >> > + } > >> > + if (validate) { > >> > + return counter%2 == 0 ? evenTest : oddTest; > >> > } > >> > else { > >> > return true; > >> > } > >> > } > >> > public void activateObject(Object obj) throws Exception { > >> > - if (exceptionOnActivate) { > >> > - if (!(validateCounter++%2 == 0 ? evenValid : > oddValid)) { > >> > + boolean hurl; > >> > + boolean evenTest; > >> > + boolean oddTest; > >> > + int counter; > >> > + synchronized(this) { > >> > + hurl = exceptionOnActivate; > >> > + evenTest = evenValid; > >> > + oddTest = oddValid; > >> > + counter = validateCounter++; > >> > + } > >> > + if (hurl) { > >> > + if (!(counter%2 == 0 ? evenTest : oddTest)) { > >> > throw new Exception(); > >> > } > >> > } > >> > } > >> > public void passivateObject(Object obj) throws Exception { > >> > - if(exceptionOnPassivate) { > >> > + boolean hurl; > >> > + synchronized(this) { > >> > + hurl = exceptionOnPassivate; > >> > + } > >> > + if (hurl) { > >> > throw new Exception(); > >> > } > >> > } > >> > @@ -1411,23 +1445,23 @@ public class TestGenericObjectPool exten > >> > long makeLatency = 0; > >> > int maxActive = Integer.MAX_VALUE; > >> > > >> > - public boolean isThrowExceptionOnActivate() { > >> > + public synchronized boolean isThrowExceptionOnActivate() { > >> > return exceptionOnActivate; > >> > } > >> > > >> > - public void setThrowExceptionOnActivate(boolean b) { > >> > + public synchronized void setThrowExceptionOnActivate(boolean > b) { > >> > exceptionOnActivate = b; > >> > } > >> > > >> > - public void setThrowExceptionOnDestroy(boolean b) { > >> > + public synchronized void setThrowExceptionOnDestroy(boolean > b) { > >> > exceptionOnDestroy = b; > >> > } > >> > > >> > - public boolean isValidationEnabled() { > >> > + public synchronized boolean isValidationEnabled() { > >> > return enableValidation; > >> > } > >> > > >> > - public void setValidationEnabled(boolean b) { > >> > + public synchronized void setValidationEnabled(boolean b) { > >> > enableValidation = b; > >> > } > >> > > >> > > >> > > >> > >> > >> > >> --------------------------------------------------------------------- > >> 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
