sebb wrote: > On 20/04/2010, Phil Steitz <[email protected]> wrote: >> sebb wrote: >> > On 18/04/2010, Phil Steitz <[email protected]> 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.
Actually not true any more. Pre-caffeine statement above. I now remember that the reason that I was worried about threadsafety in the first place is the test that I later added in r935532 (testMakeConcurrentWithReturn) that changes the validateLatency on an in-use factory. >> > > 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. See comment above on changing settings on an in-use factory. Might be better in retrospect to do as you say (make factories immutable) and build in the variable behavior otherwise. Phil > >> Phil >> >> >> Phil >> >> >> >> >> >> [email protected] 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: [email protected] >> >> For additional commands, e-mail: [email protected] >> >> >> >> >> > >> > --------------------------------------------------------------------- >> > 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] >> >> > > --------------------------------------------------------------------- > 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]
