Click
  1. Click
  2. CLK-314

Context is cleared from ThreadLocal for forward calls

    Details

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

      Description

      Ricardo identified a serious bug in ClickServlet where we check if Context should be cleared from the ThreadLocal.

      Currently we check that if the forwarded request is of type ClickRequestWrapper, we do not clear the ThreadLocal, otherwise we do.

      I wrongly concluded that the forwarded request will always be the request passed into #requestDispatcher.forward(..).

      However the spec RequestDispatcher#forward states:

      "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."

      meaning the request we pass to #forward might be wrapped by a subclass of ServletRequestWrapper. Jetty 5.12 does this by wrapping ClickRequestWrapper in a class called "DispatcherRequest".

      We can fix the problem by unwinding the request and look for an instance of ClickRequestWrapper.

      Unfortunately the above fix is not enough. After patching ClickServlet with the above fix, Jetty would not forward to the correct page. Instead it kept forwarding back to the original requests path.

      I think the problem is that ClickRequestWrapper references the original request instead of "DispatcherRequest". Will investigate further.

        Activity

        Hide
        Malcolm Edgar added a comment -

        Been thinking about this one today. I think a good solution for this would be for the underlying implementation of the ThreadLocal variable should use a stack, so for each request that goes through an new Context is created and added to the stack, once the request has been handled the Context should be popped off the stack.

        For example:

        public class Context {

        public static Context getThreadLocalContext()

        { return popThreadLocalContext(); }

        static void pushThreadLocalContext(Context context)

        { .. }

        static Context popThreadLocalContext()

        { .. }

        }

        Show
        Malcolm Edgar added a comment - Been thinking about this one today. I think a good solution for this would be for the underlying implementation of the ThreadLocal variable should use a stack, so for each request that goes through an new Context is created and added to the stack, once the request has been handled the Context should be popped off the stack. For example: public class Context { public static Context getThreadLocalContext() { return popThreadLocalContext(); } static void pushThreadLocalContext(Context context) { .. } static Context popThreadLocalContext() { .. } }
        Hide
        Bob Schellink added a comment -

        I have also worked on a solution using a Stack, but instead of stacking new Contexts, I stack the requests in the same Context.

        So each request is pushed onto the Context's stack, and the Context replaces its underlying request with the new one.

        So there are at least two options here:

        #1 create new Contexts and push them onto a stack
        #2 use existing Context and push requests onto Context#stack

        With both solutions we need to keep a reference to ClickRequestWrapper, because of the #getFileItem methods.

        The only problem I have with #1 is that on each forward the request parameters will have to be re-parsed for the new Context. With file-uploads this might be a problem?

        I have run some tests on Websphere6, Glassfish2, Tomcat5 + 6 and Jetty 5 + 6. Only Jetty 5 wraps the ClickRequestWrapper object with its own. Not sure about other servers eg JBoss, WebLogic, JOnAS etc.

        Show
        Bob Schellink added a comment - I have also worked on a solution using a Stack, but instead of stacking new Contexts, I stack the requests in the same Context. So each request is pushed onto the Context's stack, and the Context replaces its underlying request with the new one. So there are at least two options here: #1 create new Contexts and push them onto a stack #2 use existing Context and push requests onto Context#stack With both solutions we need to keep a reference to ClickRequestWrapper, because of the #getFileItem methods. The only problem I have with #1 is that on each forward the request parameters will have to be re-parsed for the new Context. With file-uploads this might be a problem? I have run some tests on Websphere6, Glassfish2, Tomcat5 + 6 and Jetty 5 + 6. Only Jetty 5 wraps the ClickRequestWrapper object with its own. Not sure about other servers eg JBoss, WebLogic, JOnAS etc.
        Hide
        Malcolm Edgar added a comment -

        How does #2 work, did you add a package private method to update the requests.

        I think I have a preference for #1, as it keeps the Context design immutable. There would be a little more work with a FileUpload doing a forward, but I think this is a somewhat rare use case.

        Regarding application servers, good work with the testing. JBoss uses Tomcat internally as its Servlet Container.

        regards Malcolm Edgar

        Show
        Malcolm Edgar added a comment - How does #2 work, did you add a package private method to update the requests. I think I have a preference for #1, as it keeps the Context design immutable. There would be a little more work with a FileUpload doing a forward, but I think this is a somewhat rare use case. Regarding application servers, good work with the testing. JBoss uses Tomcat internally as its Servlet Container. regards Malcolm Edgar
        Hide
        Bob Schellink added a comment -

        Yep #2 uses a package private method pushRequest/popRequest so Context becomes mutable.

        Also since it looks like most containers will not wrap the ClickRequestWrapper, I first check if request is instanceof ClickRequestWrapper, and only if it is not, do I call the push/pop methods.

        I will test #1 as well to keep Context immutable.

        With an eye on CLK-303 perhaps we need to have new package private constructors so we can pick the right one? Or even static package private newContext methods?
        That way we can skip extracting parameters a second time by calling a different constructor / newInstance method.

        Show
        Bob Schellink added a comment - Yep #2 uses a package private method pushRequest/popRequest so Context becomes mutable. Also since it looks like most containers will not wrap the ClickRequestWrapper, I first check if request is instanceof ClickRequestWrapper, and only if it is not, do I call the push/pop methods. I will test #1 as well to keep Context immutable. With an eye on CLK-303 perhaps we need to have new package private constructors so we can pick the right one? Or even static package private newContext methods? That way we can skip extracting parameters a second time by calling a different constructor / newInstance method.
        Hide
        Bob Schellink added a comment -

        Hi Malcolm,

        Attached are two initial patches for #1 and #2.

        Both are implemented to ensure that the stacks are only accessed on Jetty5 type containers. Also the parameters are only extracted once, forwarded requests refers to the previous ClickRequestWrapper.

        The patches seem to work on Jetty5 now. Although more testing would need to be done.

        PS: In my previous post the related bug is actually CLK-313.

        Show
        Bob Schellink added a comment - Hi Malcolm, Attached are two initial patches for #1 and #2. Both are implemented to ensure that the stacks are only accessed on Jetty5 type containers. Also the parameters are only extracted once, forwarded requests refers to the previous ClickRequestWrapper. The patches seem to work on Jetty5 now. Although more testing would need to be done. PS: In my previous post the related bug is actually CLK-313 .
        Hide
        Bob Schellink added a comment -

        Malcolm, I've assigned this issue back you for review. Also might be better ways of solving this...

        regards

        bob

        Show
        Bob Schellink added a comment - Malcolm, I've assigned this issue back you for review. Also might be better ways of solving this... regards bob
        Hide
        Malcolm Edgar added a comment -

        Hi Bob, thanks for uploading the patches. I have had a look through them. They both look good, I will probably work through #1, though #2 is simpler in some ways

        regards Malcolm Edgar

        Show
        Malcolm Edgar added a comment - Hi Bob, thanks for uploading the patches. I have had a look through them. They both look good, I will probably work through #1, though #2 is simpler in some ways regards Malcolm Edgar
        Hide
        Malcolm Edgar added a comment -

        Have checked in ThreadLocal context stack to fix this issue. Makes the code a little simpler in ClickServlet, have made not attempt to save parsed mutli-part request parameters. This was tested on Jetty 5.1.12.

        I did observer an strange NPE in PerformanceFilter related to versioned assets being forwarded to, but have not subsequently been able to reproduce this. Please keep an eye out for this.

        Regarding Context constructor, have modified it to use ClickServlet as suggested. We should probably look at introducing a package private constructor, for UnitTests which does not use ClickRequestWapper. This does make me wonder if the Context's constructor should be package visibility, and not public, as it is not intended to be created by any other code besides ClickServlet and a MockContext subclass.

        regards Malcolm Edgar

        Show
        Malcolm Edgar added a comment - Have checked in ThreadLocal context stack to fix this issue. Makes the code a little simpler in ClickServlet, have made not attempt to save parsed mutli-part request parameters. This was tested on Jetty 5.1.12. I did observer an strange NPE in PerformanceFilter related to versioned assets being forwarded to, but have not subsequently been able to reproduce this. Please keep an eye out for this. Regarding Context constructor, have modified it to use ClickServlet as suggested. We should probably look at introducing a package private constructor, for UnitTests which does not use ClickRequestWapper. This does make me wonder if the Context's constructor should be package visibility, and not public, as it is not intended to be created by any other code besides ClickServlet and a MockContext subclass. regards Malcolm Edgar
        Hide
        Bob Schellink added a comment -

        Hi Malcolm,

        Thanks for fix, very nice!

        I did notice that ContextStack is leaking on the thread. Will plug this later tonight.

        Regarding a public constructor, I have a few use cases where I want to create a custom Context in my apps through ClickServlet#createContext. For example having convenience methods to access primitives instead of doing NPE checks eg.

        public int attrToInt(String);
        public boolean attrToBoolean(String);
        public int paramToInt(String);
        public int paramToBoolean(String);

        Show
        Bob Schellink added a comment - Hi Malcolm, Thanks for fix, very nice! I did notice that ContextStack is leaking on the thread. Will plug this later tonight. Regarding a public constructor, I have a few use cases where I want to create a custom Context in my apps through ClickServlet#createContext. For example having convenience methods to access primitives instead of doing NPE checks eg. public int attrToInt(String); public boolean attrToBoolean(String); public int paramToInt(String); public int paramToBoolean(String);
        Hide
        Bob Schellink added a comment -

        I found a couple of issues and attach a possible patch:

        #1 Does not look like re-parsing of a multipart request is supported (makes sense since files are uploading via inputStream). FileUpload FAQ confirms this: ->
        http://commons.apache.org/fileupload/faq.html#empty-parse
        #2 With your fix we only need to check the stack size to determine if we should parse the request or just keep a reference to it. The problem with this is that request#setCharacterEncoding is called multiple times on an already parsed request. To resolve this I moved that logic from ClickServlet to Context.

        Not sure if the above resolves the NPE you observed, but I found no NPE yet.

        The patch worked for Tomcat6 and Jetty5, still need to test with other containers.

        That Jetty5 is a pretty strict little bugger, can't get away with nothing!

        kind regards

        bob

        Show
        Bob Schellink added a comment - I found a couple of issues and attach a possible patch: #1 Does not look like re-parsing of a multipart request is supported (makes sense since files are uploading via inputStream). FileUpload FAQ confirms this: -> http://commons.apache.org/fileupload/faq.html#empty-parse #2 With your fix we only need to check the stack size to determine if we should parse the request or just keep a reference to it. The problem with this is that request#setCharacterEncoding is called multiple times on an already parsed request. To resolve this I moved that logic from ClickServlet to Context. Not sure if the above resolves the NPE you observed, but I found no NPE yet. The patch worked for Tomcat6 and Jetty5, still need to test with other containers. That Jetty5 is a pretty strict little bugger, can't get away with nothing! kind regards bob
        Hide
        Malcolm Edgar added a comment -

        Hi Bob, the patch looks good. I like your approach for dealing with the FileItems, straight forward and efficient.

        I am assigning this back to you please apply the patch.

        regards Malcolm Edgar

        Show
        Malcolm Edgar added a comment - Hi Bob, the patch looks good. I like your approach for dealing with the FileItems, straight forward and efficient. I am assigning this back to you please apply the patch. regards Malcolm Edgar
        Hide
        Bob Schellink added a comment -

        Patch has been applied. Tested on Glassfish2, Websphere6, Jetty5/6, Tomcat6.

        Ricardo can you check if this fixes the issue for you as well?

        regards

        bob

        Show
        Bob Schellink added a comment - Patch has been applied. Tested on Glassfish2, Websphere6, Jetty5/6, Tomcat6. Ricardo can you check if this fixes the issue for you as well? regards bob
        Hide
        Ricardo R. Lecheta added a comment -

        Hi Bob,

        It worked on my Jetty 5 plus too.

        thank you

        Show
        Ricardo R. Lecheta added a comment - Hi Bob, It worked on my Jetty 5 plus too. thank you
        Hide
        Malcolm Edgar added a comment -

        Issue resolved, and fix will be available in 1.4 Final. Thanks everyone for your help with this.

        regards Malcolm Edgar

        Show
        Malcolm Edgar added a comment - Issue resolved, and fix will be available in 1.4 Final. Thanks everyone for your help with this. regards Malcolm Edgar

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development