On 20/04/2010, Phil Steitz <[email protected]> wrote: > 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. >
Or at least make as many fields final as possible; other fields could perhaps be volatile, rather than using synch. blocks. > 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] > > --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
