Click
  1. Click
  2. CLK-282

Context is leaking under heavy usage

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: core
    • Labels:
      None
    • Environment:
      Click 1.4RC2

      Description

      Attached is the profiler output after 10 000 requests. A number of garbage collections was run so the JVM can reclaim memory. Notice the in the difference column that Context and ClickRequestWrapper increased by 38 instances.

      The profiler also indicated that Context is referenced by ThreadLocal variable.

      By explicitly nullifying the ThreadLocal Context is bound to removes the leak.

      protected void handleRequest(...) {
      try

      { .... }

      finally

      { ... Context.setThreadLocalContext(null); }

      }

      PS: I vaguely remember handleRequest did nullify the ThreadLocal. If so what was the reason for taking it out? Only reason I can think of is if there are Filters needing access to the Context after handleRequest returned?

        Activity

        Hide
        Bob Schellink added a comment -

        I forgot to mention this test was done with Tomcat 6.0.13

        Show
        Bob Schellink added a comment - I forgot to mention this test was done with Tomcat 6.0.13
        Hide
        Malcolm Edgar added a comment -

        The reason why I did not clear the ThreadLocal Context variable was due to a NPE when forwarding from one Page to another. (Note this assumes that the same request thread will handle the forward, I am not sure if the Servlet spec has anything to say about this).

        Looking at the ClickServlet code I can't see straight away why this would occur. I will need to refresh my memory on this one.

        I do have a remember thinking that a Filter would be required to manage the ThreadLocal Context variable, however I didn't want to go down this path, as it would require users to update the web.xml files and make configuring the Click applications more difficult.

        We will need to investigate this issue, to determine the appropriate strategy.

        I am also wondering if CLK-281 is the main memory culprit, if this item is a genuine memory "leak". I would have though that when the Context is reset the ThreadLocal reference to the Context would be garbage collected.

        Show
        Malcolm Edgar added a comment - The reason why I did not clear the ThreadLocal Context variable was due to a NPE when forwarding from one Page to another. (Note this assumes that the same request thread will handle the forward, I am not sure if the Servlet spec has anything to say about this). Looking at the ClickServlet code I can't see straight away why this would occur. I will need to refresh my memory on this one. I do have a remember thinking that a Filter would be required to manage the ThreadLocal Context variable, however I didn't want to go down this path, as it would require users to update the web.xml files and make configuring the Click applications more difficult. We will need to investigate this issue, to determine the appropriate strategy. I am also wondering if CLK-281 is the main memory culprit, if this item is a genuine memory "leak". I would have though that when the Context is reset the ThreadLocal reference to the Context would be garbage collected.
        Hide
        Bob Schellink added a comment -

        Your assumption about the same request handling the forward is correct. Here is a snippet from the RequestDispatcher.forward method:

        The request and response parameters must be either the same objects as were
        passed to the calling servlet's service method or be subclasses of the
        ServletRequestWrapper or ServletResponseWrapper classes that wrap
        them.

        I am also not sure why this would occur. Perhaps there is a bug with Tomcat 6.0.13. I will test with other containers too.

        What I like about the current approach of NOT setting Context to null after handleRequest(..) is that helpers like ClickUtils (some methods have a dependency on Context) can be used in Filters. If we nullify the Context, a Filter cannot use these...

        Show
        Bob Schellink added a comment - Your assumption about the same request handling the forward is correct. Here is a snippet from the RequestDispatcher.forward method: The request and response parameters must be either the same objects as were passed to the calling servlet's service method or be subclasses of the ServletRequestWrapper or ServletResponseWrapper classes that wrap them. I am also not sure why this would occur. Perhaps there is a bug with Tomcat 6.0.13. I will test with other containers too. What I like about the current approach of NOT setting Context to null after handleRequest(..) is that helpers like ClickUtils (some methods have a dependency on Context) can be used in Filters. If we nullify the Context, a Filter cannot use these...
        Hide
        Bob Schellink added a comment -

        Ah I think I know what the issue might be. Containers can pool threads. Under heavy load it increases its pool size do deal with the load. So if we do not nullify the ThreadLocal, the Context would linger on the thread in the pool. I will run some tests.

        Show
        Bob Schellink added a comment - Ah I think I know what the issue might be. Containers can pool threads. Under heavy load it increases its pool size do deal with the load. So if we do not nullify the ThreadLocal, the Context would linger on the thread in the pool. I will run some tests.
        Hide
        Malcolm Edgar added a comment -

        I think your idea of the container request thread pooling increasing sounds correct, a test for this would be to see if the number is capped to the number of request handling threads. Tomcat 5.5.25 server.xml configures the Connector to have a maxThreads of 150 by default.

        Not unbinding the Contexts from ThreadLocal feels like a bad practice to me, and I am starting to feel guilty about this.

        I did some testing with page forwarding:

        /click-examples/navigation-a.htm?actionLink=forwardParamLink&value=1

        Where we nullify the ThreadLocal Context, a NPE occurs around line 948 of the ClickServlet when destroying page (Page A) resources. This is because the forwarded to page (Page B) has already been processed and has nullified the ThreadLocal context.

        The only way I can think of being able to nullify the ThreadLocal context and still support page forwarding is to have a ClickContextFilter which creates Contexts and binds and unbinds them from the request Thread.

        In thinking about having a ClickContextFilter:

        Advantages

        • unbinds the Context from request threads, more proper behavior, better memory management, as users can stuff items in the request as attributes
        • simplify the ClickSerlvet code, which is getting more complex than I like
        • supports helper methods in other filters, if the filter chaining is configured correctly

        Disadvantages

        • next release will require mandatory web.xml configuration changes for applications to work
        • configuring applications slightly more complex
        • more documentation work to do

        What are your thoughts. Personally I am leaning towards introducing the filter.

        Show
        Malcolm Edgar added a comment - I think your idea of the container request thread pooling increasing sounds correct, a test for this would be to see if the number is capped to the number of request handling threads. Tomcat 5.5.25 server.xml configures the Connector to have a maxThreads of 150 by default. Not unbinding the Contexts from ThreadLocal feels like a bad practice to me, and I am starting to feel guilty about this. I did some testing with page forwarding: /click-examples/navigation-a.htm?actionLink=forwardParamLink&value=1 Where we nullify the ThreadLocal Context, a NPE occurs around line 948 of the ClickServlet when destroying page (Page A) resources. This is because the forwarded to page (Page B) has already been processed and has nullified the ThreadLocal context. The only way I can think of being able to nullify the ThreadLocal context and still support page forwarding is to have a ClickContextFilter which creates Contexts and binds and unbinds them from the request Thread. In thinking about having a ClickContextFilter: Advantages unbinds the Context from request threads, more proper behavior, better memory management, as users can stuff items in the request as attributes simplify the ClickSerlvet code, which is getting more complex than I like supports helper methods in other filters, if the filter chaining is configured correctly Disadvantages next release will require mandatory web.xml configuration changes for applications to work configuring applications slightly more complex more documentation work to do What are your thoughts. Personally I am leaning towards introducing the filter.
        Hide
        Bob Schellink added a comment -

        I ran the tests again under load, and the profiler showed that the thread and context count is the same. So that concludes where the leak comes from.

        Also ran into the NPE when trying to unbind the context. At least the problem is now defined

        Been thinking about using a counter to increment each time we forward a page. Then unroll after the forward returns. And only unbind when the counter is 0. An immediate problem is that a JSP page can also forward. A customer RequestDispatcher can fix that. Will play with this option tomorrow.

        Something else to keep in mind is that Servlet 2.4 provides us more options. (Also Servlet 2.4 goes hand in hand with JDK 1.4?)

        1) In 2.4 filters can handle request/forward/include and errors. In 2.3 they can only handle requests. So instead of having ClickServlet and ContextFilter we could replace ClickServlet with a ClickFilter. Users only need to switch web.xml from a servlet to a filter. OK I have not thought this thru but knowing about it could be useful someday. It seems very flexible since Click could be used to decorate an existing Servlet application

        2) 2.4 supports ServletRequestListener with methods requestDestroyed and requestInitialized. This seems like a solution without needing filters. However the listener still needs to be registered in web.xml.

        About the ClickContextFilter I agree on all your points, but perhaps we should call it ClickFilter to generalize its functionality for future for example:

        1) the filter could be used to solve the "Exclude pages" issue we have. It can match exclude patterns against the path and render those pages directly instead of letting Click handle them. Also people can subclass the filter and override a method eg "public boolean exclude(String path)"
        2) From the mail list it seems people want to map to different extensions for example html, xml, REST etc. The filter could be used to rewrite these extensions to Click proper.

        Show
        Bob Schellink added a comment - I ran the tests again under load, and the profiler showed that the thread and context count is the same. So that concludes where the leak comes from. Also ran into the NPE when trying to unbind the context. At least the problem is now defined Been thinking about using a counter to increment each time we forward a page. Then unroll after the forward returns. And only unbind when the counter is 0. An immediate problem is that a JSP page can also forward. A customer RequestDispatcher can fix that. Will play with this option tomorrow. Something else to keep in mind is that Servlet 2.4 provides us more options. (Also Servlet 2.4 goes hand in hand with JDK 1.4?) 1) In 2.4 filters can handle request/forward/include and errors. In 2.3 they can only handle requests. So instead of having ClickServlet and ContextFilter we could replace ClickServlet with a ClickFilter. Users only need to switch web.xml from a servlet to a filter. OK I have not thought this thru but knowing about it could be useful someday. It seems very flexible since Click could be used to decorate an existing Servlet application 2) 2.4 supports ServletRequestListener with methods requestDestroyed and requestInitialized. This seems like a solution without needing filters. However the listener still needs to be registered in web.xml. About the ClickContextFilter I agree on all your points, but perhaps we should call it ClickFilter to generalize its functionality for future for example: 1) the filter could be used to solve the "Exclude pages" issue we have. It can match exclude patterns against the path and render those pages directly instead of letting Click handle them. Also people can subclass the filter and override a method eg "public boolean exclude(String path)" 2) From the mail list it seems people want to map to different extensions for example html, xml, REST etc. The filter could be used to rewrite these extensions to Click proper.
        Hide
        Bob Schellink added a comment -

        Just a heads up, I think there is a quickfix solution after all.

        The counter I mentioned above is already available in the form of the request object. It goes like this:

        The first time handleRequest is called the request is a HttpServletRequest instance. When we forward to either jsp or other pages the request we pass in is the ClickRequestWrapper not the original request.

        So we can have code in handleRequest like so:

        public void handleRequest(...) {
        if (!(request instanceof ClickRequestWrapper))

        { Context context = createContext(...); Context.setThreadLocalContext(context); }

        ...
        //forwarding occurs here
        ...

        if (!(request instanceof ClickRequestWrapper))

        { Context.setThreadLocalContext(null); }

        On forwarded calls the request will be instanceof ClickRequestWrapper so binding/unbinding won't occur. Once all the forward/includes are done, and the stack unwinds the request will be instanceof HttpServletRequest again and we unbind.

        I will still test this tomorrow and provide a patch for review.

        PS: You are right about the complexity of ClickServlet increasing, especially with the Stateful support. Lots of "if" statements to keep track.of

        Show
        Bob Schellink added a comment - Just a heads up, I think there is a quickfix solution after all. The counter I mentioned above is already available in the form of the request object. It goes like this: The first time handleRequest is called the request is a HttpServletRequest instance. When we forward to either jsp or other pages the request we pass in is the ClickRequestWrapper not the original request. So we can have code in handleRequest like so: public void handleRequest(...) { if (!(request instanceof ClickRequestWrapper)) { Context context = createContext(...); Context.setThreadLocalContext(context); } ... //forwarding occurs here ... if (!(request instanceof ClickRequestWrapper)) { Context.setThreadLocalContext(null); } On forwarded calls the request will be instanceof ClickRequestWrapper so binding/unbinding won't occur. Once all the forward/includes are done, and the stack unwinds the request will be instanceof HttpServletRequest again and we unbind. I will still test this tomorrow and provide a patch for review. PS: You are right about the complexity of ClickServlet increasing, especially with the Stateful support. Lots of "if" statements to keep track.of
        Hide
        Malcolm Edgar added a comment -

        Using the ClickRequestWrappers sounds like a very good solution.

        I am not very keen on replacing ClickServlet outright with (mainly fear of the unknown).

        I think we should still give some consideration to adding a "ClickFilter", for the reasons we have discussed above. I will play with this option, and see how it looks.

        Show
        Malcolm Edgar added a comment - Using the ClickRequestWrappers sounds like a very good solution. I am not very keen on replacing ClickServlet outright with (mainly fear of the unknown). I think we should still give some consideration to adding a "ClickFilter", for the reasons we have discussed above. I will play with this option, and see how it looks.
        Hide
        Bob Schellink added a comment -

        Attached a patch to fix the issue. After running 10 000 requests there are no visible leaks.

        Show
        Bob Schellink added a comment - Attached a patch to fix the issue. After running 10 000 requests there are no visible leaks.
        Hide
        Malcolm Edgar added a comment -

        I have applied the patch, which looks like a great solution.

        Unfortunately I have found a new problem with the stateful pages being loaded out of the session. Do see this problem? It could be related to some other changes I was making.

        java.lang.NullPointerException
        at org.apache.catalina.connector.Request.parseParameters(Request.java:2346)
        at org.apache.catalina.connector.Request.getParameterNames(Request.java:1047)
        at org.apache.catalina.connector.RequestFacade.getParameterNames(RequestFacade.java:369)
        at net.sf.click.ClickRequestWrapper.getParameterNames(ClickRequestWrapper.java:178)
        at net.sf.click.ClickServlet.processPageRequestParams(ClickServlet.java:1105)
        at net.sf.click.ClickServlet.initPage(ClickServlet.java:1071)
        at net.sf.click.ClickServlet.createPage(ClickServlet.java:892)
        at net.sf.click.ClickServlet.handleRequest(ClickServlet.java:363)
        at net.sf.click.ClickServlet.doGet(ClickServlet.java:284)
        at javax.servlet.http.HttpServlet.service(HttpServlet.java:690)
        at javax.servlet.http.HttpServlet.service(HttpServlet.java:803)
        at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:269)
        at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:188)
        at net.sf.click.extras.cayenne.DataContextFilter.doFilter(DataContextFilter.java:238)
        at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:215)
        at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:188)
        at org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:213)
        at org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:174)
        at org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:127)
        at org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:117)
        at org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:108)
        at org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:151)
        at org.apache.coyote.http11.Http11Processor.process(Http11Processor.java:874)
        at org.apache.coyote.http11.Http11BaseProtocol$Http11ConnectionHandler.processConnection(Http11BaseProtocol.java:665)
        at org.apache.tomcat.util.net.PoolTcpEndpoint.processSocket(PoolTcpEndpoint.java:528)
        at org.apache.tomcat.util.net.LeaderFollowerWorkerThread.runIt(LeaderFollowerWorkerThread.java:81)
        at org.apache.tomcat.util.threads.ThreadPool$ControlRunnable.run(ThreadPool.java:689)
        at java.lang.Thread.run(Thread.java:595)

        Show
        Malcolm Edgar added a comment - I have applied the patch, which looks like a great solution. Unfortunately I have found a new problem with the stateful pages being loaded out of the session. Do see this problem? It could be related to some other changes I was making. java.lang.NullPointerException at org.apache.catalina.connector.Request.parseParameters(Request.java:2346) at org.apache.catalina.connector.Request.getParameterNames(Request.java:1047) at org.apache.catalina.connector.RequestFacade.getParameterNames(RequestFacade.java:369) at net.sf.click.ClickRequestWrapper.getParameterNames(ClickRequestWrapper.java:178) at net.sf.click.ClickServlet.processPageRequestParams(ClickServlet.java:1105) at net.sf.click.ClickServlet.initPage(ClickServlet.java:1071) at net.sf.click.ClickServlet.createPage(ClickServlet.java:892) at net.sf.click.ClickServlet.handleRequest(ClickServlet.java:363) at net.sf.click.ClickServlet.doGet(ClickServlet.java:284) at javax.servlet.http.HttpServlet.service(HttpServlet.java:690) at javax.servlet.http.HttpServlet.service(HttpServlet.java:803) at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:269) at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:188) at net.sf.click.extras.cayenne.DataContextFilter.doFilter(DataContextFilter.java:238) at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:215) at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:188) at org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:213) at org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:174) at org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:127) at org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:117) at org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:108) at org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:151) at org.apache.coyote.http11.Http11Processor.process(Http11Processor.java:874) at org.apache.coyote.http11.Http11BaseProtocol$Http11ConnectionHandler.processConnection(Http11BaseProtocol.java:665) at org.apache.tomcat.util.net.PoolTcpEndpoint.processSocket(PoolTcpEndpoint.java:528) at org.apache.tomcat.util.net.LeaderFollowerWorkerThread.runIt(LeaderFollowerWorkerThread.java:81) at org.apache.tomcat.util.threads.ThreadPool$ControlRunnable.run(ThreadPool.java:689) at java.lang.Thread.run(Thread.java:595)
        Hide
        Malcolm Edgar added a comment -

        OK I found the bug, it was a caused by a performance opitimization I put in the Page class where I was caching a transient instance of the Context. This transient instance was still present after coming out of the session as was pointing to a discarded session (dangers of caching)

        Patch checked into SVN, will be available in 1.4 RC3

        Show
        Malcolm Edgar added a comment - OK I found the bug, it was a caused by a performance opitimization I put in the Page class where I was caching a transient instance of the Context. This transient instance was still present after coming out of the session as was pointing to a discarded session (dangers of caching) Patch checked into SVN, will be available in 1.4 RC3
        Hide
        Bob Schellink added a comment -

        Did the exception start only after you applied the patch? I have not seen this before. However I did notice that the ClickRequestWrapper still lingers around in the session. Looks like the Page model still refers to them. I guess we need to clean the Page model too?

        I did receive "context is null" quite often when testing it in my own code. Seems one cannot have a dependency on Context in constructor of page? Neither can Click or custom controls. That is why you moved the Menu.getRootMenu to onInit as well?

        Also the price one pays for stateful pages seems cheap initially. But there are alot of hidden costs one need to be aware of unfortunately.

        Show
        Bob Schellink added a comment - Did the exception start only after you applied the patch? I have not seen this before. However I did notice that the ClickRequestWrapper still lingers around in the session. Looks like the Page model still refers to them. I guess we need to clean the Page model too? I did receive "context is null" quite often when testing it in my own code. Seems one cannot have a dependency on Context in constructor of page? Neither can Click or custom controls. That is why you moved the Menu.getRootMenu to onInit as well? Also the price one pays for stateful pages seems cheap initially. But there are alot of hidden costs one need to be aware of unfortunately.
        Hide
        Bob Schellink added a comment -

        Heh my previous post is in reply to your one dated: 11/Jan/08 09:26 AM

        Show
        Bob Schellink added a comment - Heh my previous post is in reply to your one dated: 11/Jan/08 09:26 AM
        Hide
        Malcolm Edgar added a comment -

        Yes that is an interesting point about the page model, which contains a reference to the SessionMap, which contains a reference to the session.

        Regarding the stateful pages and constructor dependency in, yes that is why the getRootMenu was moved on onInit(). I think this should be written up in the Stateful Pages session in the doco.

        Agreed on the stateful pages, model is easy to understand but there are lots of little things to be aware of, often which are very subtle.

        Show
        Malcolm Edgar added a comment - Yes that is an interesting point about the page model, which contains a reference to the SessionMap, which contains a reference to the session. Regarding the stateful pages and constructor dependency in, yes that is why the getRootMenu was moved on onInit(). I think this should be written up in the Stateful Pages session in the doco. Agreed on the stateful pages, model is easy to understand but there are lots of little things to be aware of, often which are very subtle.

          People

          • Assignee:
            Malcolm Edgar
            Reporter:
            Bob Schellink
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development