cocoon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Berin Loritsch <blorit...@apache.org>
Subject Re: [PATCH] Re: [C2][Xalan2] Xalan2J problems under heavy load using Apache JMeter
Date Fri, 16 Mar 2001 13:51:03 GMT
Santiago Gala wrote:
> 
> Berin Loritsch wrote:
> 
> >>>> The synchronization in org.apache.avalon.util.JdbcConnectionPool is
> >>>> broken. I was about to patch it, but a major code reorganization is
> >>>> required, so I preferred to ask first.
> >>>>
> >>>
> >> In this connection pool there are serious race conditions (I have a SMP
> >> box to see these things happen before my customers :-).
> >
> >
> > Do you know where exactly?  I was thinking about changing the logic to a
> > Semaphore based type of pool.  That will fix allot of the race conditions.
> 
> First, init() should be synchronized unless you can ensure that no
> get()/put() activity will go on before init() returns. This is not
> important (I hope).

That is part of the contract.  It is not fully ready until that method is called.
It is part of the Avalon Lifecycle of Components.  Check out
http://jakarta.apache.org/avalon for more information on that.

> The big issues come in get(). Between the moment you close the first
> synchronized( m_ready ) and the  moment you open it again, m_ready can
> change! Ant the same goes on. The operation should be completely atomic.
> Also, there is a type synchronized( this ) which would do nothing, as it
> is not guarding against any other thing in the same class.

Ok.  I see your point.

> I would reimplement it completely, locking the whole set of operations.
> A good way to start is to change public methods init(), get(), put(),
> dispose() to synchronized methods and wrapping the while in run() in a
> synchronized( this ).

The init() does not need to be synchronized because no other method will
be called before that method.  It is an execute once method, so it will
never be called again after the Component is initialized.

We could use two boolean values m_initialized and m_disposed to flag the
get() and put() methods that the pool is or is not valid any longer.

That way, if m_initialized is false, calls to get() and put() would throw
an IllegalStateException.  Once dispose() is called, calls to get() throw
an IllegalStateException, and calls to put() destroy the pooled object.

> Later, the performance can be tuned, by ordering operations and using
> synchronized( this ) instead of synchronizing the whole method.

That sounds good.

> But please, start always with a safe implementation and tune later on.
> Also, think of your code as a highway, where drunk threads are running
> into. Unless you have proper traffic lights, they will bump into the
> same places (variables).

Cool.  I have limited time and limitted budget, but if you can give me
some good resources on this subject I would really appreciate it.

> This kind of thinking is useful:
> (Murphy's law). I have
> 
>         synchronized (m_ready)
>         {
>             size = m_ready.size();
>         }
> 
>         if( 0 == size )
>         { ...
> 
> Well, what if a second thread is just doing
> 
> (down delow)
>                      synchronized (m_ready)
>                      {
> /*
>                          m_ready.wait((end - start)/2);
> */
>                          if (m_ready.size() > 0)
>                          {
>                              obj = (Poolable)m_ready.remove(0);
> 
>                              synchronized (m_active)
>                              {
>                                  m_active.add(obj);
>                              }
>                          }
>                      }
> and the first thread exits the first synchronized, and then sleeps. When
> the first thread resumes, size was 1, but the second thread has just
> stealed the remaining object (remove)...
> 
> So, every use of a shared resource MUST be serialized atomically. Having
> serialized acces to the size, and then releasing the lock and using the
> size is not valid, as the size can change in between.
> 
> Thinking of your code as a highway where threads run is very useful in
> multithreaded programming. Think of variables as lanes or parking space,
> or tickets that a driver picks up and leaves later. Two cars cannot have
> the same ticket at the same time!

That is what I was trying to avoid (poorly I might add).  Alright.  There
is a Lock object in Avalon, so I might try to use that.

> > I am the one who originally wrote the code--and I don't have an SMP box
> > to test on.  Please, by all means, if you can do it better, do it!
> 
> I'll try to have time to do a basic runnable implementation, but full
> optimization should be left for the future (first get it working)
> 
> Also, i code like
> 
> if( true ) {
>      //body
>      }
> 
> not like
> 
> if(true)
> {
>      //body
> }
> 
> I hear something around here... :)

Same here.  However the coding standards on Avalon are to do next line
bracing (using Turbine as precident).  The code I placed in there was
formatted the way we normally code, and was subsequently reformatted
by someone else.

---------------------------------------------------------------------
To unsubscribe, e-mail: cocoon-dev-unsubscribe@xml.apache.org
For additional commands, email: cocoon-dev-help@xml.apache.org


Mime
View raw message