commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sebb <seb...@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 10:28:51 GMT
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.

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


Mime
View raw message