commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Phil Steitz <phil.ste...@gmail.com>
Subject Re: svn commit: r935362 - /commons/proper/pool/trunk/src/test/org/apache/commons/pool/impl/TestGenericObjectPool.java
Date Tue, 20 Apr 2010 12:16:38 GMT
sebb wrote:
> On 20/04/2010, Phil Steitz <phil.steitz@gmail.com> wrote:
>> sebb wrote:
>>  > On 18/04/2010, Phil Steitz <phil.steitz@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


Mime
View raw message