Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.9
    • Fix Version/s: 2.1.8.1
    • Component/s: None
    • Labels:
      None
    • Environment:

      Description

      Struts 2 somehow prevents the app's classes from being garbage collected when the application is stopped or undeployed.

      I created a barebones Struts 2 app with an action with the following code:
      private static final Object x = new Object() {

      { System.out.println("================== Object created: " + hashCode() + " ==================="); }

      protected void finalize() throws Throwable

      { System.out.println("**************** Object finalized: " + hashCode() + " *********************"); }

      ;
      };

      Because of this static field, a message should be printed when the class is initialized and when it is garbage collected. "Object created" would be printed out whenever I went to the action for the first time, but restarting the app never printed "Object finalized." This is not an issue with garbage collection in my web container because doing the same thing with a servlet resulted in both messages being printed.

      One problem is that the FilterDispatcher.init() method sets a ThreadLocal but never clears it. I fixed that by adding ActionContext.setContext(null); to the end of the init() method, but that didn't solve the larger problem.

      1. ActionContextReleasingFilterDispatcher.java
        1.0 kB
        Sean Kleinjung
      2. FilterDispatcher_leak-fix.java
        21 kB
        Sean Kleinjung
      3. log_leak.png
        52 kB
        Nicolas Raynaud

        Activity

        Hide
        James Holmes added a comment -

        Can you give us more information about your environment? Which app server? Whick JDK? Which OS?

        Show
        James Holmes added a comment - Can you give us more information about your environment? Which app server? Whick JDK? Which OS?
        Hide
        Adam Crume added a comment -

        Has this been looked at? I added all the information you asked for to the Environment field.

        Show
        Adam Crume added a comment - Has this been looked at? I added all the information you asked for to the Environment field.
        Hide
        Rob Leland added a comment -

        I thought that it was entirely up to the JVM wheather finalize() is ever called.

        Do you have class unloading enabled for a sun JVM it is :-XX:+CMSClassUnloadingEnabled

        Show
        Rob Leland added a comment - I thought that it was entirely up to the JVM wheather finalize() is ever called. Do you have class unloading enabled for a sun JVM it is :-XX:+CMSClassUnloadingEnabled
        Hide
        Adam Crume added a comment -

        My understanding is that finalize() will be called eventually (unless the JVM shuts down), although it is not guaranteed to be called immediately. In either case, as I said I tried the same experiment with a project containing an action and a project containing a servlet, and the finalize() method was being called for the static object in the servlet but not the action. I tried this several times and the results were quite repeatable.

        Class unloading is enabled. Personally, I can't imagine why on earth it would be disabled for an application server. Every time an app was restarted, memory would leak.

        Besides, if class unloading was disabled in my JVM, the static object in the servlet would never be garbage collected and the finalize() method would never be called, but this was clearly not the case.

        Show
        Adam Crume added a comment - My understanding is that finalize() will be called eventually (unless the JVM shuts down), although it is not guaranteed to be called immediately. In either case, as I said I tried the same experiment with a project containing an action and a project containing a servlet, and the finalize() method was being called for the static object in the servlet but not the action. I tried this several times and the results were quite repeatable. Class unloading is enabled. Personally, I can't imagine why on earth it would be disabled for an application server. Every time an app was restarted, memory would leak. Besides, if class unloading was disabled in my JVM, the static object in the servlet would never be garbage collected and the finalize() method would never be called, but this was clearly not the case.
        Hide
        Adam Crume added a comment -

        As further proof, I added a static field:
        private static final byte[] data = new byte[1024 * 1024 * 10]; // 10 megabytes

        This was added to the servlet and to the action. If I visited the servlet, restarted its project, visited the servlet, restarted its project, etc., memory usage remained stable. If I did the same thing with the action, memory usage went up by 10 megabytes every time I restarted the project and visited the action.

        Show
        Adam Crume added a comment - As further proof, I added a static field: private static final byte[] data = new byte [1024 * 1024 * 10] ; // 10 megabytes This was added to the servlet and to the action. If I visited the servlet, restarted its project, visited the servlet, restarted its project, etc., memory usage remained stable. If I did the same thing with the action, memory usage went up by 10 megabytes every time I restarted the project and visited the action.
        Hide
        Tim Stavenger added a comment -

        I've experienced a similar memory leak while in the following environment:

        WebLogic 9.2 MP1
        Sun Java 1.5.0_12 and BEA JRockit 90_150_06 (a Sun Java 1.5 equivalent)
        Windows XP and Linux (RedHat)

        I've noticed Struts and XWork classes remaining in the class loader after undeployment. This is noticed by both general out of memory exceptions in the heap and perm gen space. I can also see the memory leak by viewing the classes loaded while using BEA JRockit and JRockit's memory leak detection tool (similar to many memory profiling applications). Struts and XWork classes remain in the class loader after undeployment and several garbage collection processes. Upon redeployment, a new set of the same classes are loaded again (so now there are multiple copies of the same classes in memory).

        The memory leak not only with the application my company is deploying while using Struts 2.0.9, but also with the Struts 2.0.9 showcase demo application. I believe this shows that my memory leak issues are not entirely our code's fault, but rather at least in part due to Struts.

        Unfortunately I have not been able to pinpoint any particular set of classes as the culprit for the memory leak, mainly due to my lack of familiarity with the Struts and XWork code base. However, since I see the same problem with the Struts showcase sample app and a memory profiler, I would expect similar results to be reproducible most anywhere.

        Show
        Tim Stavenger added a comment - I've experienced a similar memory leak while in the following environment: WebLogic 9.2 MP1 Sun Java 1.5.0_12 and BEA JRockit 90_150_06 (a Sun Java 1.5 equivalent) Windows XP and Linux (RedHat) I've noticed Struts and XWork classes remaining in the class loader after undeployment. This is noticed by both general out of memory exceptions in the heap and perm gen space. I can also see the memory leak by viewing the classes loaded while using BEA JRockit and JRockit's memory leak detection tool (similar to many memory profiling applications). Struts and XWork classes remain in the class loader after undeployment and several garbage collection processes. Upon redeployment, a new set of the same classes are loaded again (so now there are multiple copies of the same classes in memory). The memory leak not only with the application my company is deploying while using Struts 2.0.9, but also with the Struts 2.0.9 showcase demo application. I believe this shows that my memory leak issues are not entirely our code's fault, but rather at least in part due to Struts. Unfortunately I have not been able to pinpoint any particular set of classes as the culprit for the memory leak, mainly due to my lack of familiarity with the Struts and XWork code base. However, since I see the same problem with the Struts showcase sample app and a memory profiler, I would expect similar results to be reproducible most anywhere.
        Hide
        Nicolas Raynaud added a comment - - edited

        I've spent a few hours on this issue, looking at memory snapshots. My finding seems to be an ActionContext tied to a ThreadLocal (every route I checked in memory lead to an ActionContext in a ThreadLocal).

        The problem is that I can't find any code path leading to an exit of the request leaving an ActionContext in the threadlocal to pin a bug.

        As a side note : you HAVE TO unregister manually the JDBC driver, spring doesn't do it.

        edit : found a suspect @ StrutsXmlConfigurationProvider line109 (on loading) and line 164 (on unloading) those lines are called in init and shutdown thread which are not worker thread and no cleanup is performed.

        Show
        Nicolas Raynaud added a comment - - edited I've spent a few hours on this issue, looking at memory snapshots. My finding seems to be an ActionContext tied to a ThreadLocal (every route I checked in memory lead to an ActionContext in a ThreadLocal). The problem is that I can't find any code path leading to an exit of the request leaving an ActionContext in the threadlocal to pin a bug. As a side note : you HAVE TO unregister manually the JDBC driver, spring doesn't do it. edit : found a suspect @ StrutsXmlConfigurationProvider line109 (on loading) and line 164 (on unloading) those lines are called in init and shutdown thread which are not worker thread and no cleanup is performed.
        Hide
        Tim Stavenger added a comment -

        While I see the potential issue in the StrutsXmlConfigurationProvider class that Nicolas points out, I'm not certain that this class is really the entire reason for the memory leak (at least in my case).

        While using the JRockit memory leak detector (a memory profiler), I do see the StrutsXmlConfigurationProvider class remaining in memory, but I also see XWork classes holding references to the StrutsXmlConfigurationProvider.

        In particular I see instances of com.opensymphony.xwork2.inject.ContainerImpl and its inner classes as well as com.opensymphony.xwork2.inject.ContainerBuilder. I haven't looked much into the code, so I'm not familiar with it. Perhaps this portion of the memory leak should be filed as a bug/issue to the XWork project?

        Show
        Tim Stavenger added a comment - While I see the potential issue in the StrutsXmlConfigurationProvider class that Nicolas points out, I'm not certain that this class is really the entire reason for the memory leak (at least in my case). While using the JRockit memory leak detector (a memory profiler), I do see the StrutsXmlConfigurationProvider class remaining in memory, but I also see XWork classes holding references to the StrutsXmlConfigurationProvider. In particular I see instances of com.opensymphony.xwork2.inject.ContainerImpl and its inner classes as well as com.opensymphony.xwork2.inject.ContainerBuilder. I haven't looked much into the code, so I'm not familiar with it. Perhaps this portion of the memory leak should be filed as a bug/issue to the XWork project?
        Hide
        Nicolas Raynaud added a comment -

        Tim > I've not even yet resolved the issue on MY case yet (its now a log4j problem I'm investigating), and looking at the code practice of the project, I suppose that opportunities for memory leak are quite numerous.

        This is a self-reinforcing worst case : one reference is enough to create the leak, but the probability to have many leaks is high if you don't specifically test against it.

        I far as I know, only commiters can really solve this problem, but for this they have to feel concerned about it, because it is a really hard problem. Finding a path between a GC root and an object is easy with appropriate tool, but connecting the memory map to the code path that led to it is hard.

        Show
        Nicolas Raynaud added a comment - Tim > I've not even yet resolved the issue on MY case yet (its now a log4j problem I'm investigating), and looking at the code practice of the project, I suppose that opportunities for memory leak are quite numerous. This is a self-reinforcing worst case : one reference is enough to create the leak, but the probability to have many leaks is high if you don't specifically test against it. I far as I know, only commiters can really solve this problem, but for this they have to feel concerned about it, because it is a really hard problem. Finding a path between a GC root and an object is easy with appropriate tool, but connecting the memory map to the code path that led to it is hard.
        Hide
        Nicolas Raynaud added a comment - - edited

        Log4JLogger class seems to be loaded in the wrong classloader.

        I used tomcat 5.5.25 for my debug session, and I put common-logging and log4j in common/lib and an log4.properties in common/classes as I found on the web. I used the LogFactory.release(Thread.currentThread().getContextClassLoader()); from http://wiki.apache.org/jakarta-commons/Logging/UndeployMemoryLeak?action=print in my cleanup listener as well, But no success (well actually, I don't how this stuff would change a static variable in such a remote place)

        But I don't understand much to this Log stuff, that's why I need some help.

        BTW, nothing tells me this is the last leak in my case, but I cannot calculate all the routes from the classloader to the GC roots.

        Show
        Nicolas Raynaud added a comment - - edited Log4JLogger class seems to be loaded in the wrong classloader. I used tomcat 5.5.25 for my debug session, and I put common-logging and log4j in common/lib and an log4.properties in common/classes as I found on the web. I used the LogFactory.release(Thread.currentThread().getContextClassLoader()); from http://wiki.apache.org/jakarta-commons/Logging/UndeployMemoryLeak?action=print in my cleanup listener as well, But no success (well actually, I don't how this stuff would change a static variable in such a remote place) But I don't understand much to this Log stuff, that's why I need some help. BTW, nothing tells me this is the last leak in my case, but I cannot calculate all the routes from the classloader to the GC roots.
        Hide
        Sean Kleinjung added a comment -

        There is a memory leak in XWork 2.0.4 documented as Jira issue XW-560 (http://jira.opensymphony.com/browse/XW-560). This issue is fixed in XWork 2.1.0. However, several classes that Struts uses were removed in this version, so it is not possible to deploy Struts 2.0.11 with XWork 2.1.0. I have fixed this issue temporarily by patching my XWork 2.0.4 jar with the fix for this bug.

        In order to fix this leak in Struts, a future version will need to be released that uses XWork 2.1.0 or later.

        Show
        Sean Kleinjung added a comment - There is a memory leak in XWork 2.0.4 documented as Jira issue XW-560 ( http://jira.opensymphony.com/browse/XW-560 ). This issue is fixed in XWork 2.1.0. However, several classes that Struts uses were removed in this version, so it is not possible to deploy Struts 2.0.11 with XWork 2.1.0. I have fixed this issue temporarily by patching my XWork 2.0.4 jar with the fix for this bug. In order to fix this leak in Struts, a future version will need to be released that uses XWork 2.1.0 or later.
        Hide
        Sean Kleinjung added a comment -

        Ok.. My previous comment was made a bit prematurely. Call me an optimist. It looks like there are actually three places where references are being kept and causing classloader leaks – all related to XWork ThreadLocals.

        1. The InternalContext leak referenced by http://jira.opensymphony.com/browse/XW-560. I previously stated this was resolved in version 2.1.0 of XWork, but upon closer inspection the patch was not fully applied. Applying the patch attached to that Jira issue WILL resolve this leak, but it is not yet fixed in the xwork trunk.

        2. The Struts FilterDispatcher's init method results in an ActionContext being stored in a ThreadLocal that is never cleaned up. This is because it calls "Dispatcher.init()" which eventually results in a call to org.apache.struts2.config.StrutsXmlConfigurationProvider.loadPackages. This method calls ActionContext.getContext(), which attempts to access the ActionContext stored in a ThreadLocal. If there is none, one is created via the com.opensymphony.xwork2.ActionContext.ActionContextThreadLocal.initialValue method. This instance is not cleaned up anywhere by Struts or xwork. I am not certain who is responsible for cleaning up this 'default' ActionContext, so I cannot say for certain if this is a bug in Struts or xwork. However, it can be fixed by wrapping the init method of FilterDispatcher in a try block, and calling ActionContext.setContext(null) in the finally block.

        3. The Struts FilterDispatcher's destroy method has a similar problem to #2, except this time the offending ActionContext is ultimately created from a call in org.apache.struts2.config.StrutsXmlConfigurationProvider.needsReload. This leak can also be fixed by wrapping the destroy method in a try block and clearing the ActionContext in its finally.

        It is worth noting that in 2.1.0 and beyond of xwork, there is no initialValue implementation for the ActionContext thread local. I haven't looked at the xwork source closely enough to know if they are manually creating (and hopefully destroying) an ActionContext in cases that were previously handled by initialValue. If they are not, however, then the calls from init and destroy above will receive a null ActionContext, and need to change accordingly.

        So, to summarize there are two things that must happen in order for a Struts app to undeploy cleanly without classloader leaks:
        1) The XW-560 issue must be resolved
        2) The lifecycle methods on FilterDispatcher must properly clean up their ActionContext thread locals. This could probably be done by a change to xwork, but in the meantime a patch to FilterDispatcher can accomplish the same thing. I am attaching a patched version of FilterDispatcher that my testing has found to be leak-free.

        Now, even after the above is done there are a lot of other, non-Struts, leak culprits. I would advise any interested persons to read http://opensource.atlassian.com/confluence/spring/pages/viewpage.action?pageId=2669 for more possibilities if they are still having problems.

        Show
        Sean Kleinjung added a comment - Ok.. My previous comment was made a bit prematurely. Call me an optimist. It looks like there are actually three places where references are being kept and causing classloader leaks – all related to XWork ThreadLocals. 1. The InternalContext leak referenced by http://jira.opensymphony.com/browse/XW-560 . I previously stated this was resolved in version 2.1.0 of XWork, but upon closer inspection the patch was not fully applied. Applying the patch attached to that Jira issue WILL resolve this leak, but it is not yet fixed in the xwork trunk. 2. The Struts FilterDispatcher's init method results in an ActionContext being stored in a ThreadLocal that is never cleaned up. This is because it calls "Dispatcher.init()" which eventually results in a call to org.apache.struts2.config.StrutsXmlConfigurationProvider.loadPackages. This method calls ActionContext.getContext(), which attempts to access the ActionContext stored in a ThreadLocal. If there is none, one is created via the com.opensymphony.xwork2.ActionContext.ActionContextThreadLocal.initialValue method. This instance is not cleaned up anywhere by Struts or xwork. I am not certain who is responsible for cleaning up this 'default' ActionContext, so I cannot say for certain if this is a bug in Struts or xwork. However, it can be fixed by wrapping the init method of FilterDispatcher in a try block, and calling ActionContext.setContext(null) in the finally block. 3. The Struts FilterDispatcher's destroy method has a similar problem to #2, except this time the offending ActionContext is ultimately created from a call in org.apache.struts2.config.StrutsXmlConfigurationProvider.needsReload. This leak can also be fixed by wrapping the destroy method in a try block and clearing the ActionContext in its finally. It is worth noting that in 2.1.0 and beyond of xwork, there is no initialValue implementation for the ActionContext thread local. I haven't looked at the xwork source closely enough to know if they are manually creating (and hopefully destroying) an ActionContext in cases that were previously handled by initialValue. If they are not, however, then the calls from init and destroy above will receive a null ActionContext, and need to change accordingly. So, to summarize there are two things that must happen in order for a Struts app to undeploy cleanly without classloader leaks: 1) The XW-560 issue must be resolved 2) The lifecycle methods on FilterDispatcher must properly clean up their ActionContext thread locals. This could probably be done by a change to xwork, but in the meantime a patch to FilterDispatcher can accomplish the same thing. I am attaching a patched version of FilterDispatcher that my testing has found to be leak-free. Now, even after the above is done there are a lot of other, non-Struts, leak culprits. I would advise any interested persons to read http://opensource.atlassian.com/confluence/spring/pages/viewpage.action?pageId=2669 for more possibilities if they are still having problems.
        Hide
        Sean Kleinjung added a comment -

        Modified version of FilterDispatcher which contains finally blocks in the init and destroy methods. These blocks ensure that the ActionContext thread locals are cleaned up properly.

        Show
        Sean Kleinjung added a comment - Modified version of FilterDispatcher which contains finally blocks in the init and destroy methods. These blocks ensure that the ActionContext thread locals are cleaned up properly.
        Hide
        Sean Kleinjung added a comment -

        Searching for other places that call the init and cleanup methods of Dispatcher reveals two more locations that probably lead to the creation of ThreadLocals that do not get removed. First is the init() and destroy() methods of Jsr168Dispatcher. I do not have a portal application that I can use to test this class, although if there is a problem a similar try..finally to the one I previously posted should fix it. The last place I can see this problem occurring is in the org.apache.struts2.util.StrutsTestCaseHelper#initDispatcher method. This last one, however, is of a lower priority because unit tests rarely have the type of lifecycle where classloader leaks would be a serious problem.

        Show
        Sean Kleinjung added a comment - Searching for other places that call the init and cleanup methods of Dispatcher reveals two more locations that probably lead to the creation of ThreadLocals that do not get removed. First is the init() and destroy() methods of Jsr168Dispatcher. I do not have a portal application that I can use to test this class, although if there is a problem a similar try..finally to the one I previously posted should fix it. The last place I can see this problem occurring is in the org.apache.struts2.util.StrutsTestCaseHelper#initDispatcher method. This last one, however, is of a lower priority because unit tests rarely have the type of lifecycle where classloader leaks would be a serious problem.
        Hide
        Haroon Rafique added a comment -

        Can this get some attention from struts/xwork committers? Getting tired of my continuous integration machine croaking over every 5 or 6 builds because of struts2.

        Sean, you, sir, are a true giant among men. Kudos.

        Show
        Haroon Rafique added a comment - Can this get some attention from struts/xwork committers? Getting tired of my continuous integration machine croaking over every 5 or 6 builds because of struts2. Sean, you, sir, are a true giant among men. Kudos.
        Hide
        Tim Stavenger added a comment -

        As Haroon said, thank you Sean for looking into this. I certainly await the time when your fix is included in a release. I may even consider applying the patch on my current Struts libraries, if at the very least to test that the memory leak issues I have been seeing are fixed/reduced.

        Haroon – I have my continuous integration builds automatically restart WebLogic at the start of the build. This at least has made it longer before I need to worry about any OOM exceptions.

        Show
        Tim Stavenger added a comment - As Haroon said, thank you Sean for looking into this. I certainly await the time when your fix is included in a release. I may even consider applying the patch on my current Struts libraries, if at the very least to test that the memory leak issues I have been seeing are fixed/reduced. Haroon – I have my continuous integration builds automatically restart WebLogic at the start of the build. This at least has made it longer before I need to worry about any OOM exceptions.
        Hide
        Sean Kleinjung added a comment -

        It was certainly no problem to look into this... I was experiencing the same problem with our in-house integration server. Besides, a lot of the process involved had already been explored in resources linked to in this thread and elsewhere. In the end, I ended up fixing our application without having to patch the Struts jar itself. We are using a custom sub-class of FilterDispatcher that adds the necessary ActionContext cleanup call, which I am attaching to this issue. Simply add the ActionContextReleasingFilterDispatcher to your project, and use it in the "filter-class" element in your web.xml instead of org.apache.struts2.dispatcher.FilterDispatcher. Unfortunately, I found no work-around for the xwork ContainerImpl leaking a reference to InternalContext, and so you will still need to manually patch your xwork jar.

        Keep in mind, also, that having ANY single leak of this type will prevent the entire classloader (and all of its classes) from being unloaded. So if you want an app to unload cleanly, you must fix all the issues discussed here, and be prepared to research/resolve problems in other libraries. See the link I posted earlier for some starting points to look at.

        Show
        Sean Kleinjung added a comment - It was certainly no problem to look into this... I was experiencing the same problem with our in-house integration server. Besides, a lot of the process involved had already been explored in resources linked to in this thread and elsewhere. In the end, I ended up fixing our application without having to patch the Struts jar itself. We are using a custom sub-class of FilterDispatcher that adds the necessary ActionContext cleanup call, which I am attaching to this issue. Simply add the ActionContextReleasingFilterDispatcher to your project, and use it in the "filter-class" element in your web.xml instead of org.apache.struts2.dispatcher.FilterDispatcher. Unfortunately, I found no work-around for the xwork ContainerImpl leaking a reference to InternalContext, and so you will still need to manually patch your xwork jar. Keep in mind, also, that having ANY single leak of this type will prevent the entire classloader (and all of its classes) from being unloaded. So if you want an app to unload cleanly, you must fix all the issues discussed here, and be prepared to research/resolve problems in other libraries. See the link I posted earlier for some starting points to look at.
        Hide
        Sean Kleinjung added a comment -

        Sub-class of FilterDispatcher that can be used to workaround the ActionContext leaking in Struts/xwork when the FilterDispatcher is initialized or destroyed.

        Show
        Sean Kleinjung added a comment - Sub-class of FilterDispatcher that can be used to workaround the ActionContext leaking in Struts/xwork when the FilterDispatcher is initialized or destroyed.
        Hide
        Don Brown added a comment -

        Sean, thanks for the help. Could you attach a patch against trunk? Also, have you tested your patch with Sitemesh? It does some interesting things with multiple requests that cause Struts to bend over backwards, necessitating the cleanup filter.

        Show
        Don Brown added a comment - Sean, thanks for the help. Could you attach a patch against trunk? Also, have you tested your patch with Sitemesh? It does some interesting things with multiple requests that cause Struts to bend over backwards, necessitating the cleanup filter.
        Hide
        Jeromy Evans added a comment -

        The proposed workaround has been committed to trunk. Requires broader verification that the context is properly released.

        Show
        Jeromy Evans added a comment - The proposed workaround has been committed to trunk. Requires broader verification that the context is properly released.
        Hide
        Don Brown added a comment -

        Did you verify that this doesn't break Sitemesh decorators that use Struts 2 tags? The reason it isn't cleared directly is that Sitemesh decorators might contain Struts 2 tags that require an ActionContext. The current solution of the second cleanup filter is clunky, but I'm not sure what else we can do.

        Show
        Don Brown added a comment - Did you verify that this doesn't break Sitemesh decorators that use Struts 2 tags? The reason it isn't cleared directly is that Sitemesh decorators might contain Struts 2 tags that require an ActionContext. The current solution of the second cleanup filter is clunky, but I'm not sure what else we can do.
        Hide
        Jeromy Evans added a comment -

        It's difficult to verify properly. In my own applications I confirmed that a selection of struts2 tags still function correctly within a decorator (text, form, text and submit) and that a threadlocal ActionContext existed each time.

        I'm not sure that a threadlocal ActionContext is guaranteed to exist though. If the order of execution of the destroy methods of the filters is not guaranteed I guess it will not work.

        (verified in Tomcat 5.5 with struts-cleanup enabled)

        Show
        Jeromy Evans added a comment - It's difficult to verify properly. In my own applications I confirmed that a selection of struts2 tags still function correctly within a decorator (text, form, text and submit) and that a threadlocal ActionContext existed each time. I'm not sure that a threadlocal ActionContext is guaranteed to exist though. If the order of execution of the destroy methods of the filters is not guaranteed I guess it will not work. (verified in Tomcat 5.5 with struts-cleanup enabled)
        Hide
        Jeromy Evans added a comment -

        I've started seeing some peculiarities in the sitemesh decorator today. It's intermittently failing to decorate some requests. Until I can isolate the precise problem I'm reopening the issue. The work-around is still implemented on trunk.

        Show
        Jeromy Evans added a comment - I've started seeing some peculiarities in the sitemesh decorator today. It's intermittently failing to decorate some requests. Until I can isolate the precise problem I'm reopening the issue. The work-around is still implemented on trunk.
        Hide
        Jeromy Evans added a comment -

        Just noting I've seen no further problems with this. The intermittent error encountered can be attributed to something else.

        Show
        Jeromy Evans added a comment - Just noting I've seen no further problems with this. The intermittent error encountered can be attributed to something else.
        Hide
        Don Brown added a comment -

        Cool, let's call it fixed then.

        Show
        Don Brown added a comment - Cool, let's call it fixed then.
        Hide
        Matthew Payne added a comment -

        This definitely does mess with sitemesh decorators. re
        I would like to see this re-opened

        While simple decoration is fine, what is available to its context is different

        http://www.nabble.com/Struts-2.1.2(an-previous-releases)-vs-2.1.3-in-regards-to-actionMessages-in-a-sitemesh-decorator-td19361286.html#a19366415

        paste discussion

        matt.payne wrote:
        > There seem to be some type of regression that has happen with regards to
        > actionMessage/actionErrors.
        >
        > In the current build snapshots of 2.1.3 actionErrors/message are no longer
        > exposed my to the sitemesh decorator(I am using freemarker result
        > types/decorators). However that are still available to the direct result,
        > just not within the decorators.
        >

        In 2.1.3 there have been some changes around the FilterDispatcher and
        ActionContextCleanup.
        You may notice the FilterDispatcher is deprecated.

        See the struts-dev "New Filter Strategy RFC" thread started on 22 June 08.

        I haven't paid attention to what's changed and have some code running
        with the older filters and some on the new. I'm certain none of my code
        uses action messages inside decorators though.

        In 2.1.2, the resolution of WW-2167 also carried some risk for sitemesh
        decorators:
        https://issues.apache.org/struts/browse/WW-2167

        Show
        Matthew Payne added a comment - This definitely does mess with sitemesh decorators. re I would like to see this re-opened While simple decoration is fine, what is available to its context is different http://www.nabble.com/Struts-2.1.2(an-previous-releases)-vs-2.1.3-in-regards-to-actionMessages-in-a-sitemesh-decorator-td19361286.html#a19366415 paste discussion matt.payne wrote: > There seem to be some type of regression that has happen with regards to > actionMessage/actionErrors. > > In the current build snapshots of 2.1.3 actionErrors/message are no longer > exposed my to the sitemesh decorator(I am using freemarker result > types/decorators). However that are still available to the direct result, > just not within the decorators. > In 2.1.3 there have been some changes around the FilterDispatcher and ActionContextCleanup. You may notice the FilterDispatcher is deprecated. See the struts-dev "New Filter Strategy RFC" thread started on 22 June 08. I haven't paid attention to what's changed and have some code running with the older filters and some on the new. I'm certain none of my code uses action messages inside decorators though. In 2.1.2, the resolution of WW-2167 also carried some risk for sitemesh decorators: https://issues.apache.org/struts/browse/WW-2167
        Hide
        Jeromy Evans added a comment -

        Happy to reopen but this still need to see some better investigation.

        The code sets the threadlocal ActionContext to null in the filter's destroy method (which it should). If the destroy method is invoked before the sitemesh filter and the decorator's actionerrors call is trying to read the threadlocal then I'm satisfied there's a problem.

        I won't support re-enabling the memory leak though so this may be worthy of a separate issue. Not sure if the refactoring of the two struts filters helps this??

        Your post indicates some of the context is available to the sitemesh decorator. My testing confirmed properties could be access but I never examined action errors.

        Please also identify the container in case it relates to order of filter method invocation in a particular container.

        Show
        Jeromy Evans added a comment - Happy to reopen but this still need to see some better investigation. The code sets the threadlocal ActionContext to null in the filter's destroy method (which it should). If the destroy method is invoked before the sitemesh filter and the decorator's actionerrors call is trying to read the threadlocal then I'm satisfied there's a problem. I won't support re-enabling the memory leak though so this may be worthy of a separate issue. Not sure if the refactoring of the two struts filters helps this?? Your post indicates some of the context is available to the sitemesh decorator. My testing confirmed properties could be access but I never examined action errors. Please also identify the container in case it relates to order of filter method invocation in a particular container.
        Hide
        Matthew Payne added a comment -

        ^^ Good news.
        If I use the StrutsPrepareFilter(instead of ActionContextCleanUp) and StrutsExecuteFilter(instead of FilterDispatcher) things work just like old.
        This would seem to be the recommended path.

        So the deprecated FilterDispatcher/ActionContextCleanUp is deprecated and "broken" for 2.1.3, but if people use the new recommended filters things look to be okay.
        ^^

        Show
        Matthew Payne added a comment - ^^ Good news. If I use the StrutsPrepareFilter(instead of ActionContextCleanUp) and StrutsExecuteFilter(instead of FilterDispatcher) things work just like old. This would seem to be the recommended path. So the deprecated FilterDispatcher/ActionContextCleanUp is deprecated and "broken" for 2.1.3, but if people use the new recommended filters things look to be okay. ^^
        Hide
        Matthew Payne added a comment -

        ^^^
        To Jeromy Evans:

        Isn't the destroy method the same for 2.1.2 vs 2.1.3
        public void destroy() {
        if (dispatcher == null)

        { log.warn("something is seriously wrong, Dispatcher is not initialized (null) "); }

        else {
        try

        { dispatcher.cleanup(); }

        finally

        { ActionContext.setContext(null); }

        }
        }

        That would mean there is something else different besides the destroy method

        – Matt

        Show
        Matthew Payne added a comment - ^^^ To Jeromy Evans: Isn't the destroy method the same for 2.1.2 vs 2.1.3 public void destroy() { if (dispatcher == null) { log.warn("something is seriously wrong, Dispatcher is not initialized (null) "); } else { try { dispatcher.cleanup(); } finally { ActionContext.setContext(null); } } } That would mean there is something else different besides the destroy method – Matt
        Hide
        Jeromy Evans added a comment -

        Hi Matt,
        Yes, I had assumed it was being more generally reported as "it used to work in 2.1.x"as some users are a little inaccurate with the version numbers.
        Happy to mark this resolved again? If so I'll raise the filter issue separately so it's not forgotten.

        Show
        Jeromy Evans added a comment - Hi Matt, Yes, I had assumed it was being more generally reported as "it used to work in 2.1.x"as some users are a little inaccurate with the version numbers. Happy to mark this resolved again? If so I'll raise the filter issue separately so it's not forgotten.
        Hide
        Matthew Payne added a comment -

        ^^^ okay with it as long as next release has release notes in it strongly indicating to switch to the new fitlers.

        --Matt

        Show
        Matthew Payne added a comment - ^^^ okay with it as long as next release has release notes in it strongly indicating to switch to the new fitlers. --Matt
        Hide
        Dave Newton added a comment -

        I would like to ask the panel what they would do if they were fitler.

        (Sorry; couldn't resist the Monty Python reference.)

        Show
        Dave Newton added a comment - I would like to ask the panel what they would do if they were fitler. (Sorry; couldn't resist the Monty Python reference.)
        Hide
        Torsten Krah added a comment -

        Some question to this bug and its proposed fix (svn commit).
        The patch to the FilterDispatcher should be applied to the Jsr168Dispatcher for a portlet environment too, shouldn't it?

        Show
        Torsten Krah added a comment - Some question to this bug and its proposed fix (svn commit). The patch to the FilterDispatcher should be applied to the Jsr168Dispatcher for a portlet environment too, shouldn't it?
        Hide
        Don Brown added a comment -

        Soo, it sounds like we are all agreed this can be closed?

        Show
        Don Brown added a comment - Soo, it sounds like we are all agreed this can be closed?
        Hide
        Matthew Payne added a comment -

        ^^ agreed. note in release notes. lets get 2.1.3 out.....

        Show
        Matthew Payne added a comment - ^^ agreed. note in release notes. lets get 2.1.3 out.....
        Hide
        Nils-Helge Garli added a comment -

        Can this issue be closed now?

        Show
        Nils-Helge Garli added a comment - Can this issue be closed now?
        Hide
        Crume, Adam added a comment -

        I'm afraid I'm busy and don't have time to verify the fix. If it's supposed to be fixed, then I'm okay with the issue being closed.

        Show
        Crume, Adam added a comment - I'm afraid I'm busy and don't have time to verify the fix. If it's supposed to be fixed, then I'm okay with the issue being closed.
        Hide
        Wes Wannemacher added a comment -

        just noticed this is still open. Bumping to 2.1.9 because I don't have to research before 2.1.8

        Show
        Wes Wannemacher added a comment - just noticed this is still open. Bumping to 2.1.9 because I don't have to research before 2.1.8
        Hide
        Lukasz Lenart added a comment -

        I investigated the code and the issue can be closed. Using the new filters will solve the problem either integration with Sitemesh or without Sitemesh.

        Regards

        Lukasz

        Show
        Lukasz Lenart added a comment - I investigated the code and the issue can be closed. Using the new filters will solve the problem either integration with Sitemesh or without Sitemesh. Regards – Lukasz
        Hide
        Lukasz Lenart added a comment -

        Already solved

        Show
        Lukasz Lenart added a comment - Already solved

          People

          • Assignee:
            Unassigned
            Reporter:
            Adam Crume
          • Votes:
            7 Vote for this issue
            Watchers:
            13 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development