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