Re: svn commit: r575397 - /felix/trunk/framework/src/main/java/org/apache/felix/framework/util/EventDispatcher.java

[Karl Pauls <ka...@gmail.com>]

Rob,

I tried to simplify the code somewhat and do the clean-up in a finally
block. It now uses the m_threadLock instead of the m_shutdownLock. I
do think that should work correctly.

Could you retry this with your set-up? In case something is broken
again I rollback to your version. Thanks!

regards,

Karl

On 9/13/07, walkerr@apache.org <wa...@apache.org> wrote:
> Author: walkerr
> Date: Thu Sep 13 11:36:07 2007
> New Revision: 575397
>
> URL: http://svn.apache.org/viewvc?rev=575397&view=rev
> Log:
> Put back in "wait" step on event shutdown, but used dedicated shutdown lock outside of other synchronosed blocks to help avoid deadlocks (FELIX-363).
>
> Modified:
>     felix/trunk/framework/src/main/java/org/apache/felix/framework/util/EventDispatcher.java
>
> Modified: felix/trunk/framework/src/main/java/org/apache/felix/framework/util/EventDispatcher.java
> URL: http://svn.apache.org/viewvc/felix/trunk/framework/src/main/java/org/apache/felix/framework/util/EventDispatcher.java?rev=575397&r1=575396&r2=575397&view=diff
> ==============================================================================
> --- felix/trunk/framework/src/main/java/org/apache/felix/framework/util/EventDispatcher.java (original)
> +++ felix/trunk/framework/src/main/java/org/apache/felix/framework/util/EventDispatcher.java Thu Sep 13 11:36:07 2007
> @@ -62,9 +62,10 @@
>      // A single thread is used to deliver events for all dispatchers.
>      private static Thread m_thread = null;
>      private static String m_threadLock = "thread lock";
> +    private static String m_shutdownLock = "thread shutdown lock";
>      private static int m_references = 0;
>      private static boolean m_stopping = false;
> -    private static boolean m_stopped = false;
> +
>      // List of requests.
>      private static final ArrayList m_requestList = new ArrayList();
>      // Pooled requests to avoid memory allocation.
> @@ -84,24 +85,32 @@
>              // Start event dispatching thread if necessary.
>              if (m_thread == null || !m_thread.isAlive())
>              {
> +                m_stopping = false;
> +
>                  m_thread = new Thread(new Runnable() {
>                      public void run()
>                      {
>                          EventDispatcher.run();
>                          // Ensure we update state even if stopped by external cause
>                          // e.g. an Applet VM forceably killing threads
> -                        m_thread = null;
> -                        m_stopped = true;
> -                        m_stopping = false;
> +                        synchronized (m_threadLock)
> +                        {
> +                            m_thread = null;
> +                            m_stopping = false;
> +                            m_references = 0;
> +                        }
> +
> +                        synchronized (m_shutdownLock)
> +                        {
> +                            m_shutdownLock.notifyAll();
> +                        }
>                      }
>                  }, "FelixDispatchQueue");
>                  m_thread.start();
>              }
> -
> +
>              // reference counting and flags
>              m_references++;
> -            m_stopping = false;
> -            m_stopped = false;
>          }
>
>          return eventDispatcher;
> @@ -111,8 +120,8 @@
>      {
>          synchronized (m_threadLock)
>          {
> -            // Return if already stopped.
> -            if (m_stopped)
> +            // Return if already dead or stopping.
> +            if (m_thread == null || m_stopping)
>              {
>                  return;
>              }
> @@ -123,12 +132,28 @@
>              {
>                  return;
>              }
> -
> -            // Signal dispatch thread.
> +
>              m_stopping = true;
> -            synchronized (m_requestList)
> +        }
> +
> +        // Signal dispatch thread.
> +        synchronized (m_requestList)
> +        {
> +            m_requestList.notify();
> +        }
> +
> +        // Use separate lock for shutdown to prevent any chance of nested lock deadlock
> +        synchronized (m_shutdownLock)
> +        {
> +            while (m_thread != null)
>              {
> -                m_requestList.notify();
> +                try
> +                {
> +                    m_shutdownLock.wait();
> +                }
> +                catch (InterruptedException ex)
> +                {
> +                }
>              }
>          }
>      }
> @@ -531,8 +556,9 @@
>      private void fireEventAsynchronously(
>          Logger logger, int type, Object[] listeners, EventObject event)
>      {
> +        //TODO: should possibly check this within thread lock, seems to be ok though without
>          // If dispatch thread is stopped, then ignore dispatch request.
> -        if (m_stopped || m_stopping)
> +        if (m_stopping || m_thread == null)
>          {
>              return;
>          }
>
>
>


-- 
Karl Pauls
karlpauls@gmail.com