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 14:18:44 GMT
On 20/04/2010, Phil Steitz <phil.steitz@gmail.com> wrote:
> 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.
>

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


Mime
View raw message