Struts 2
  1. Struts 2
  2. WW-2523

NPE in dispatcher cleanup when FilterDispatcher is used in a portlet

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Won't Fix
    • Affects Version/s: 2.0.11, 2.1.0
    • Fix Version/s: Future
    • Labels:
      None
    • Environment:

      JBoss Portal 2.6.4, Struts 2.1.1

      Description

      When the FilterDispatcher is configured in web.xml, it does cleanup first, then JSR168Dispatcher does cleanup, and for some reason, the ThreadLocal variable (the actual ThreadLocal variable, not the value it contain) is null. I don't know if this is an issue only in JBoss.

      1. stackTrace_ok.txt
        4 kB
        Torsten Krah
      2. stackTrace_failure.txt
        13 kB
        Torsten Krah
      3. FilterDispatcher.diff
        2 kB
        Torsten Krah

        Activity

        Hide
        Torsten Krah added a comment - - edited

        This is a issue to with Pluto 1.1.5 and e.g. Tomcat 6.0.13 and Pluto 1.1.6 with Tomcat 6.0.18.
        I am facing this problem too on JBoss (4.0.5, Tomcat 5.5.20, Liferay 5.x).

        Version 2.0.11.2 (latest stable Release) is affected too.
        I think this is a critical flaw as you cannot undeploy or redeploy the webapp and are forced to restart the container every time.
        Imho this should be fixed in 2.0 branch too.

        Show
        Torsten Krah added a comment - - edited This is a issue to with Pluto 1.1.5 and e.g. Tomcat 6.0.13 and Pluto 1.1.6 with Tomcat 6.0.18. I am facing this problem too on JBoss (4.0.5, Tomcat 5.5.20, Liferay 5.x). Version 2.0.11.2 (latest stable Release) is affected too. I think this is a critical flaw as you cannot undeploy or redeploy the webapp and are forced to restart the container every time. Imho this should be fixed in 2.0 branch too.
        Hide
        Torsten Krah added a comment -

        Could this be raised to blocker priority please?
        Fixing this in future versions is really a no go because portlet spec v 1.0 (struts 2.0.11.2 implements) does require to use servlets to deliver resources like images, css or other resource stuff.
        You can use FilterDispatcher and Jsr168Dispatcher and its working - but only until you want to stop - restart or redeploy your webapp - the container get stuck because the app cannot be undeployed.
        This is more than a major problem because the complete container (and in a portlet environment there you got many other apps than your own providing portlets) needs to be killed and "restarted".

        Show
        Torsten Krah added a comment - Could this be raised to blocker priority please? Fixing this in future versions is really a no go because portlet spec v 1.0 (struts 2.0.11.2 implements) does require to use servlets to deliver resources like images, css or other resource stuff. You can use FilterDispatcher and Jsr168Dispatcher and its working - but only until you want to stop - restart or redeploy your webapp - the container get stuck because the app cannot be undeployed. This is more than a major problem because the complete container (and in a portlet environment there you got many other apps than your own providing portlets) needs to be killed and "restarted".
        Hide
        Torsten Krah added a comment -

        Some more details:

        At init level all is fine.
        FilterDispatcher and Jsr168 got its own dispatcher Instance.

        But at the first Request it fails here:

        protected HttpServletRequest prepareDispatcherAndWrapRequest(HttpServletRequest request, HttpServletResponse response) throws ServletException {
        Dispatcher du = Dispatcher.getInstance();

        // Prepare and wrap the request if the cleanup filter hasn't already, cleanup filter should be
        // configured first before struts2 dispatcher filter, hence when its cleanup filter's turn,
        // static instance of Dispatcher should be null.
        if (du == null)

        { Dispatcher.setInstance(dispatcher); // prepare the request no matter what - this ensures that the proper character encoding // is used before invoking the mapper (see WW-9127) dispatcher.prepare(request, response); }

        else

        { dispatcher = du; }

        At init time Dispatcher du = Dispatcher.getInstance() is null, so the dispatcher instance is used which was created before and gets set to the instance var.
        But at first request time Dispatcher.getInstance(), called in FilterDispatcher not the Jsr168 one, is returning the actual instance which was created by the Jsr168 Dispatcher - i don't know why but thats what the debugger told.
        Both Dispatchers are working with the same instance and all is working fine - but only until destroy is called, because both modify the same instance, which must fail if it is done a second time.
        The other instance which is dangeling around gets not cleaned up and the app fails do undeploy.
        The only thing i am missing is - hm i need to read more about those ThreadLocals - why getInstance does deliver the same instance ...

        Show
        Torsten Krah added a comment - Some more details: At init level all is fine. FilterDispatcher and Jsr168 got its own dispatcher Instance. But at the first Request it fails here: protected HttpServletRequest prepareDispatcherAndWrapRequest(HttpServletRequest request, HttpServletResponse response) throws ServletException { Dispatcher du = Dispatcher.getInstance(); // Prepare and wrap the request if the cleanup filter hasn't already, cleanup filter should be // configured first before struts2 dispatcher filter, hence when its cleanup filter's turn, // static instance of Dispatcher should be null. if (du == null) { Dispatcher.setInstance(dispatcher); // prepare the request no matter what - this ensures that the proper character encoding // is used before invoking the mapper (see WW-9127) dispatcher.prepare(request, response); } else { dispatcher = du; } At init time Dispatcher du = Dispatcher.getInstance() is null, so the dispatcher instance is used which was created before and gets set to the instance var. But at first request time Dispatcher.getInstance(), called in FilterDispatcher not the Jsr168 one, is returning the actual instance which was created by the Jsr168 Dispatcher - i don't know why but thats what the debugger told. Both Dispatchers are working with the same instance and all is working fine - but only until destroy is called, because both modify the same instance, which must fail if it is done a second time. The other instance which is dangeling around gets not cleaned up and the app fails do undeploy. The only thing i am missing is - hm i need to read more about those ThreadLocals - why getInstance does deliver the same instance ...
        Hide
        Torsten Krah added a comment -

        StackTrace ok.

        Show
        Torsten Krah added a comment - StackTrace ok.
        Hide
        Torsten Krah added a comment -

        Failure Trace.

        Show
        Torsten Krah added a comment - Failure Trace.
        Hide
        Torsten Krah added a comment -

        Look at the failure stackTrace.
        serviceAction on the Jsr168Dispatchter gots invoked.
        Looking at this method it reads like this:

        Dispatcher.setInstance(dispatcherUtils);

        So the actual thread local gets overwritten with the local value of the Jsr168Dispatcher everytime.
        Later at the StackTrace look at prepareDispatcherAndWrapRequest of FilterDispatcher.

        There Dispatcher.getInstance() is used to get the actual ThreadLocal. Doing this the first time this return null and the local FilterDispatcher dispatcher value is used and ThreadLocal is set.
        The snd time this is done, Jsr168 overwrites the actual ThreadLocal value and the reference to the one created by the FilterDispatcher is lost.

        Maybe someone can confirm this? Maybe i am wrong but thats what it looks to me.

        Show
        Torsten Krah added a comment - Look at the failure stackTrace. serviceAction on the Jsr168Dispatchter gots invoked. Looking at this method it reads like this: Dispatcher.setInstance(dispatcherUtils); So the actual thread local gets overwritten with the local value of the Jsr168Dispatcher everytime. Later at the StackTrace look at prepareDispatcherAndWrapRequest of FilterDispatcher. There Dispatcher.getInstance() is used to get the actual ThreadLocal. Doing this the first time this return null and the local FilterDispatcher dispatcher value is used and ThreadLocal is set. The snd time this is done, Jsr168 overwrites the actual ThreadLocal value and the reference to the one created by the FilterDispatcher is lost. Maybe someone can confirm this? Maybe i am wrong but thats what it looks to me.
        Hide
        Torsten Krah added a comment -

        Workaround.
        If an IllegalStateException is thrown, catch it because the Jsr168 Dispatcher has already cleaned things up (but log it).
        In this case the private variable initialDispatcher holding a reference to the dispatcher which was created through init() method needs to be cleaned up.

        More than a hack than a real fix, but i am not a struts2 source code hacker. It solves the container problem and i can redeploy, restart and stop my webapp / container without those annoying need to "kill" the whole thing.
        It may have side effects, but i did not have found anyone yet.
        Comments appreciated.

        Show
        Torsten Krah added a comment - Workaround. If an IllegalStateException is thrown, catch it because the Jsr168 Dispatcher has already cleaned things up (but log it). In this case the private variable initialDispatcher holding a reference to the dispatcher which was created through init() method needs to be cleaned up. More than a hack than a real fix, but i am not a struts2 source code hacker. It solves the container problem and i can redeploy, restart and stop my webapp / container without those annoying need to "kill" the whole thing. It may have side effects, but i did not have found anyone yet. Comments appreciated.
        Hide
        Nils-Helge Garli added a comment -

        I think the best would be if the FilterDispatcher and the Jsr168Dispatcher had it's own actual instance of the Dispatcher. I'm not sure how easy that is to achieve though.

        Show
        Nils-Helge Garli added a comment - I think the best would be if the FilterDispatcher and the Jsr168Dispatcher had it's own actual instance of the Dispatcher. I'm not sure how easy that is to achieve though.
        Hide
        Torsten Krah added a comment -

        Yes this would be the best idea. JBoss is doing undeployment first for FilterDispatcher, so you have to patch Jsr168 one too ... that can't be the right solution.
        However i don't know how to patch both so everyone got its own actual instance, if someone can provide me with some more details how it might be done, i might be able to write a patch.

        Show
        Torsten Krah added a comment - Yes this would be the best idea. JBoss is doing undeployment first for FilterDispatcher, so you have to patch Jsr168 one too ... that can't be the right solution. However i don't know how to patch both so everyone got its own actual instance, if someone can provide me with some more details how it might be done, i might be able to write a patch.
        Hide
        Nils-Helge Garli added a comment -

        I can only reproduce the NPE in JBoss portal. Pluto seems to undeploy without problems.

        Show
        Nils-Helge Garli added a comment - I can only reproduce the NPE in JBoss portal. Pluto seems to undeploy without problems.
        Hide
        Nils-Helge Garli added a comment -

        It's still beyond my understanding how the 'instance' thread-local variable suddenly turns null... I have tried doing a null check, but aparently the 'dispatcherListeners' static variable is also suddenly null. So for some reason, the static fields of the class are set to null somewhere... I'm guessing there's some sort of cleanup happening in the container that causes this.

        Show
        Nils-Helge Garli added a comment - It's still beyond my understanding how the 'instance' thread-local variable suddenly turns null... I have tried doing a null check, but aparently the 'dispatcherListeners' static variable is also suddenly null. So for some reason, the static fields of the class are set to null somewhere... I'm guessing there's some sort of cleanup happening in the container that causes this.
        Hide
        Nils-Helge Garli added a comment -

        This might be related to https://issues.apache.org/bugzilla/show_bug.cgi?id=37956

        From reading that, it seems there's a system property 'org.apache.catalina.loader.WebappClassLoader.ENABLE_CLEAR_REFERENCES' that controls this behaviour. Try setting it to "false" and see if that makes any difference.

        Show
        Nils-Helge Garli added a comment - This might be related to https://issues.apache.org/bugzilla/show_bug.cgi?id=37956 From reading that, it seems there's a system property 'org.apache.catalina.loader.WebappClassLoader.ENABLE_CLEAR_REFERENCES' that controls this behaviour. Try setting it to "false" and see if that makes any difference.
        Hide
        Nils-Helge Garli added a comment -

        I downloaded Jboss Portal 2.7.2 and tested the system property there. Without setting it, I experienced the same error. But when setting it to false, I no longer got the NPE.

        Show
        Nils-Helge Garli added a comment - I downloaded Jboss Portal 2.7.2 and tested the system property there. Without setting it, I experienced the same error. But when setting it to false, I no longer got the NPE.
        Hide
        Nils-Helge Garli added a comment -

        Aparently, this is caused by Tomcat aggressively setting static fields to null when the context is stopped. This behaviour can be configured with the org.apache.catalina.loader.WebappClassLoader.ENABLE_CLEAR_REFERENCES system property. Setting it to 'false' prevents this error (-Dorg.apache.catalina.loader.WebappClassLoader.ENABLE_CLEAR_REFERENCES=false).

        Show
        Nils-Helge Garli added a comment - Aparently, this is caused by Tomcat aggressively setting static fields to null when the context is stopped. This behaviour can be configured with the org.apache.catalina.loader.WebappClassLoader.ENABLE_CLEAR_REFERENCES system property. Setting it to 'false' prevents this error (-Dorg.apache.catalina.loader.WebappClassLoader.ENABLE_CLEAR_REFERENCES=false).

          People

          • Assignee:
            Unassigned
            Reporter:
            Nils-Helge Garli
          • Votes:
            1 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development