You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@commons.apache.org by ps...@apache.org on 2010/04/18 17:59:27 UTC
svn commit: r935362 -
/commons/proper/pool/trunk/src/test/org/apache/commons/pool/impl/TestGenericObjectPool.java
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;
}
Re: svn commit: r935362 - /commons/proper/pool/trunk/src/test/org/apache/commons/pool/impl/TestGenericObjectPool.java
Posted by sebb <se...@gmail.com>.
On 20/04/2010, Phil Steitz <ph...@gmail.com> wrote:
> sebb wrote:
> > On 20/04/2010, Phil Steitz <ph...@gmail.com> wrote:
> >> sebb wrote:
> >> > On 18/04/2010, Phil Steitz <ph...@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.
>
>
> 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
> >> >>
> >> >>
> >> >> psteitz@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-unsubscribe@commons.apache.org
> >> >> For additional commands, e-mail: dev-help@commons.apache.org
> >> >>
> >> >>
> >> >
> >> > ---------------------------------------------------------------------
> >> > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> >> > For additional commands, e-mail: dev-help@commons.apache.org
> >> >
> >>
> >>
> >> ---------------------------------------------------------------------
> >> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> >> For additional commands, e-mail: dev-help@commons.apache.org
> >>
> >>
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > For additional commands, e-mail: dev-help@commons.apache.org
> >
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r935362 - /commons/proper/pool/trunk/src/test/org/apache/commons/pool/impl/TestGenericObjectPool.java
Posted by Phil Steitz <ph...@gmail.com>.
sebb wrote:
> On 20/04/2010, Phil Steitz <ph...@gmail.com> wrote:
>> sebb wrote:
>> > On 18/04/2010, Phil Steitz <ph...@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.
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
>> >>
>> >>
>> >> psteitz@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-unsubscribe@commons.apache.org
>> >> For additional commands, e-mail: dev-help@commons.apache.org
>> >>
>> >>
>> >
>> > ---------------------------------------------------------------------
>> > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> > For additional commands, e-mail: dev-help@commons.apache.org
>> >
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r935362 - /commons/proper/pool/trunk/src/test/org/apache/commons/pool/impl/TestGenericObjectPool.java
Posted by sebb <se...@gmail.com>.
On 20/04/2010, Phil Steitz <ph...@gmail.com> wrote:
> sebb wrote:
> > On 18/04/2010, Phil Steitz <ph...@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
> >>
> >>
> >> psteitz@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-unsubscribe@commons.apache.org
> >> For additional commands, e-mail: dev-help@commons.apache.org
> >>
> >>
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > For additional commands, e-mail: dev-help@commons.apache.org
> >
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r935362 - /commons/proper/pool/trunk/src/test/org/apache/commons/pool/impl/TestGenericObjectPool.java
Posted by Phil Steitz <ph...@gmail.com>.
sebb wrote:
> On 18/04/2010, Phil Steitz <ph...@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
>>
>>
>> psteitz@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-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r935362 - /commons/proper/pool/trunk/src/test/org/apache/commons/pool/impl/TestGenericObjectPool.java
Posted by sebb <se...@gmail.com>.
On 18/04/2010, Phil Steitz <ph...@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.
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.
> Phil
>
>
> psteitz@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-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r935362 - /commons/proper/pool/trunk/src/test/org/apache/commons/pool/impl/TestGenericObjectPool.java
Posted by Phil Steitz <ph...@gmail.com>.
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 ;)
Phil
psteitz@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-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org