Tapestry
  1. Tapestry
  2. TAPESTRY-230

Ignored interruptedException prevents Janitorthread termination

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 3.0, 3.0.1
    • Fix Version/s: 3.0.5
    • Component/s: Framework
    • Labels:
      None
    • Environment:
      Tomcat 4 & 5 (on Windows platform)

      Description

      The org.apache.tapestry.util.JanitorThread uses a waitForNextPass() function that makes it sleep for 30 seconds and then do another pass.
      In this method an InterruptedException is caught and ignored. This prevents the thread from terminating normally when, for instance, a context is reloaded. See also http://java.sun.com/j2se/1.5.0/docs/guide/misc/threadPrimitiveDeprecation.html.

      I think it would be better if the while-loop in the run() method exited when the wait() in waitfornextpass() method was interrupted.

        Activity

        Hide
        Nicholas Stuart added a comment -

        I've just tried version 3.0.2 on Windows Server 2003 and Tomcat 5.5.4 and the issue is still there. I am unable to reload the context because the Tapestry and Tapestry-Contrib jars are still locked, and can not be deleted until I manually stop tomcat and delete the files by hand.

        Show
        Nicholas Stuart added a comment - I've just tried version 3.0.2 on Windows Server 2003 and Tomcat 5.5.4 and the issue is still there. I am unable to reload the context because the Tapestry and Tapestry-Contrib jars are still locked, and can not be deleted until I manually stop tomcat and delete the files by hand.
        Hide
        Martijn Stellinga added a comment -

        I did some checking; if I understand correctly Tomcat doesn't close any Threads that were spawned by a servlet when that servlet is destroyed. In other words, the JanitorThread remains active.
        One way to stop it is to override the Tapestry servlet (and declaring your subclassed servlet in the web.xml) and overriding the destroy() method:
        public void destroy() {
        try

        { // Close Tapestry JanitorThread JanitorThread.getSharedJanitorThread().interrupt(); }

        finally

        { super.destroy(); }

        }
        Even though the Thread is static, because of the way classloading works for servlets, each servlet will have its own JanitorThread.
        Maybe the above code should be a part of the org.apache.tapestry.ApplicationServlet?

        Show
        Martijn Stellinga added a comment - I did some checking; if I understand correctly Tomcat doesn't close any Threads that were spawned by a servlet when that servlet is destroyed. In other words, the JanitorThread remains active. One way to stop it is to override the Tapestry servlet (and declaring your subclassed servlet in the web.xml) and overriding the destroy() method: public void destroy() { try { // Close Tapestry JanitorThread JanitorThread.getSharedJanitorThread().interrupt(); } finally { super.destroy(); } } Even though the Thread is static, because of the way classloading works for servlets, each servlet will have its own JanitorThread. Maybe the above code should be a part of the org.apache.tapestry.ApplicationServlet?
        Hide
        Nicholas Stuart added a comment -

        Hi Martijn, I just tried your suggestion and that doesn't appear to work either. I know that its using my servlet and calling destroy just fine:
        2005-02-17 13:19:08,909 [http-8080-Processor4] DEBUG com.vort.help.servlets.MyApplicationServlet - Init MyApplication Servlet.
        2005-02-17 13:19:16,219 [http-8080-Processor3] DEBUG com.vort.help.servlets.MyApplicationServlet - Destroying MyApplication Servlet.

        But, it never goes back and inits again, because the files are never deleted. In theory there should be another Init after that destroy, but thats all I got in my logs. So I dont know know if its the janitor thread or not. Is there anyway I can look to see whats still being used by tomcat? Would like to help with this, but really not sure where to begin.

        Show
        Nicholas Stuart added a comment - Hi Martijn, I just tried your suggestion and that doesn't appear to work either. I know that its using my servlet and calling destroy just fine: 2005-02-17 13:19:08,909 [http-8080-Processor4] DEBUG com.vort.help.servlets.MyApplicationServlet - Init MyApplication Servlet. 2005-02-17 13:19:16,219 [http-8080-Processor3] DEBUG com.vort.help.servlets.MyApplicationServlet - Destroying MyApplication Servlet. But, it never goes back and inits again, because the files are never deleted. In theory there should be another Init after that destroy, but thats all I got in my logs. So I dont know know if its the janitor thread or not. Is there anyway I can look to see whats still being used by tomcat? Would like to help with this, but really not sure where to begin.
        Hide
        Paul Ferraro added a comment -

        There are 2 issues at play here...
        1.) In order to function properly in every servlet engine implementation, the shared JanitorThread needs to be explicitly interrupted upon servlet context destruction.
        2.) The fix implemented in 3.0.2 is not complete. With the fix in 3.0.2, the JanitorThread only exists properly if interrupted during a sleep() operation. If the thread is interrupted during a sweep() operation the interrupted status of the thread is set, but never checked - so the run() method never exits.

        The fix should look something like this...

        protected void waitForNextPass()
        {
        try

        { sleep(interval); }

        catch (InterruptedException e)

        { interrupt(); }

        }

        public void run()
        {
        while (!isInterrupted())

        { waitForNextPass(); sweep(); }

        }

        Show
        Paul Ferraro added a comment - There are 2 issues at play here... 1.) In order to function properly in every servlet engine implementation, the shared JanitorThread needs to be explicitly interrupted upon servlet context destruction. 2.) The fix implemented in 3.0.2 is not complete. With the fix in 3.0.2, the JanitorThread only exists properly if interrupted during a sleep() operation. If the thread is interrupted during a sweep() operation the interrupted status of the thread is set, but never checked - so the run() method never exits. The fix should look something like this... protected void waitForNextPass() { try { sleep(interval); } catch (InterruptedException e) { interrupt(); } } public void run() { while (!isInterrupted()) { waitForNextPass(); sweep(); } }
        Hide
        David Taylor added a comment -

        Here is my attempt at a fix based on the previous comments. This seems to fix the severe locking issue I was seeing when deploying to Tomcat 5.5.7 using MyEclipse. Independent verification would be appreciated since I have not done extensive testing.

        A key observation not previously mentioned is that the thread interrupt() method has no effect when the thread is not sleeping. The use of a boolean flag solves this problem. The tricky part is that synchronization needs to be done properly to ensure the JMM forces memory cache flushing across the threads. Note the use of the volatile and synchronized keywords. The Java threading and memory management models have some very surprising wrinkles if you are used to threading on other platforms. IBM published a great article on this a few years back as I recall.

        Anyway, if I haven't bored you to death, here are my simple code changes:

        Changes to JanitorThread.java (v3.0.2)

        Additions:

        /**

        • Flag used to signal the thread to stop. Note: The volatile
        • keyword may not be fully implemented on some JVMs
          */
          private volatile boolean terminated = false;

        /**

        • Signals the thread to terminate
          */

        public synchronized void terminate()

        { terminated = true; // Always do this first! interrupt(); }

        Modifications:

        public void run()
        {
        while (true)
        {
        synchronized (this) // Must synchronize even when reading!

        { if (terminated) return; }

        if (!waitForNextPass())
        return;

        sweep();
        }
        }

        Changes to ApplicationServlet.java (v3.0.2)

        Additions:

        /**

        • Signals the shared janitor thread to terminate when the
        • servlet is shutdown.
          */

        public void destroy() {
        try

        { // Close Tapestry JanitorThread JanitorThread janitorThread = JanitorThread.getSharedJanitorThread(); if (janitorThread != null) // Just being a little paranoid janitorThread.terminate(); }

        finally

        { super.destroy(); }

        }

        Show
        David Taylor added a comment - Here is my attempt at a fix based on the previous comments. This seems to fix the severe locking issue I was seeing when deploying to Tomcat 5.5.7 using MyEclipse. Independent verification would be appreciated since I have not done extensive testing. A key observation not previously mentioned is that the thread interrupt() method has no effect when the thread is not sleeping. The use of a boolean flag solves this problem. The tricky part is that synchronization needs to be done properly to ensure the JMM forces memory cache flushing across the threads. Note the use of the volatile and synchronized keywords. The Java threading and memory management models have some very surprising wrinkles if you are used to threading on other platforms. IBM published a great article on this a few years back as I recall. Anyway, if I haven't bored you to death, here are my simple code changes: Changes to JanitorThread.java (v3.0.2) Additions: /** Flag used to signal the thread to stop. Note: The volatile keyword may not be fully implemented on some JVMs */ private volatile boolean terminated = false; /** Signals the thread to terminate */ public synchronized void terminate() { terminated = true; // Always do this first! interrupt(); } Modifications: public void run() { while (true) { synchronized (this) // Must synchronize even when reading! { if (terminated) return; } if (!waitForNextPass()) return; sweep(); } } Changes to ApplicationServlet.java (v3.0.2) Additions: /** Signals the shared janitor thread to terminate when the servlet is shutdown. */ public void destroy() { try { // Close Tapestry JanitorThread JanitorThread janitorThread = JanitorThread.getSharedJanitorThread(); if (janitorThread != null) // Just being a little paranoid janitorThread.terminate(); } finally { super.destroy(); } }
        Hide
        Paul Ferraro added a comment -

        Issue reopened since bug persists after fix in 3.0.2.

        Show
        Paul Ferraro added a comment - Issue reopened since bug persists after fix in 3.0.2.
        Hide
        Paul Ferraro added a comment -

        Added explicit JanitorThread interruption in ApplicationServlet.destroy() method.
        Fixed case where JanitorThread interruption happens during sweep() option.

        Show
        Paul Ferraro added a comment - Added explicit JanitorThread interruption in ApplicationServlet.destroy() method. Fixed case where JanitorThread interruption happens during sweep() option.

          People

          • Assignee:
            Paul Ferraro
            Reporter:
            Martijn Stellinga
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development