Wicket
  1. Wicket
  2. WICKET-1478

AbortWithWebErrorCodeException in onBeforeRender causes WicketRuntimeException

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.3.2
    • Fix Version/s: 1.3.5
    • Component/s: wicket
    • Labels:
      None

      Description

      Using custom error pages as described on http://cwiki.apache.org/WICKET/error-pages-and-feedback-messages.html, a wicket RTE gets thrown "There is no application attached to current thread [threadname]" when a AbortWithWebErrorCodeException is thrown in the onBeforeRender method of a WebPage. Note that this only happens when using a custom error page defined in web.xml.

      See attached quickstart for the complete stacktrace (and my usecase)
      See also the mailinglist entry on http://www.nabble.com/Throw-AbortWithWebErrorCodeException-in-onBeforeRender-to16446341.html#a16473849

      1. quickstart404.jar
        16 kB
        Michael Sparer

        Activity

        Hide
        Johan Compagner added a comment -

        OK, when setting an error status (httpServletResponse.sendError(errorCode)), the container will try to resolve that in that call itself. This will result in a second wicket call in the same thread. Because of that we will clean all thread locals like RequestCycle and Application. I do reset those now to the previous values.

        The problem is that we have more thread locals which also could be affected. Like Sessions thread locals (dirty pages) Also the RequestContext for our portlet support how to handle that?
        We need to look at this in a next release a bit more (1.4)

        So for the most part this is fixed (if not everything) for the next 1.3.x release.

        Ate: can you quickly look at this code in WicketFilter:
        // unset the application thread local if it didn't exist already.
        if (previous == null)

        { Application.unset(); RequestContext.unset(); }

        else

        { Application.set(previous); }

        You see i dont do anything with RequestContext when the previous app was there..
        Can i do that? Or is this not an issue at all in portlets?

        Reassign to me when you have looked at it.

        Show
        Johan Compagner added a comment - OK, when setting an error status (httpServletResponse.sendError(errorCode)), the container will try to resolve that in that call itself. This will result in a second wicket call in the same thread. Because of that we will clean all thread locals like RequestCycle and Application. I do reset those now to the previous values. The problem is that we have more thread locals which also could be affected. Like Sessions thread locals (dirty pages) Also the RequestContext for our portlet support how to handle that? We need to look at this in a next release a bit more (1.4) So for the most part this is fixed (if not everything) for the next 1.3.x release. Ate: can you quickly look at this code in WicketFilter: // unset the application thread local if it didn't exist already. if (previous == null) { Application.unset(); RequestContext.unset(); } else { Application.set(previous); } You see i dont do anything with RequestContext when the previous app was there.. Can i do that? Or is this not an issue at all in portlets? Reassign to me when you have looked at it.
        Hide
        Ate Douma added a comment -

        Johan,

        I don't have time to evaluate this right now: this is something too tricky to be able to review by looking at the code itself. I need to test this out in the portal to be sure what happens.
        What I'm curious about though is: why do you "reset" the Application to previous? Why can't you just clear it properly (e.g. setting the threadlocals really to null) instead?

        I'm willing to test this out and see how it works when running as a portlet, but it'll have to wait until after the ApacheCon (so next week the earliest).
        For now, you're changes should be fine for plain webapps as the default non-portlet RequestContext instance doesn't keep any state anyway (the PortletRequestContext does however).

        Show
        Ate Douma added a comment - Johan, I don't have time to evaluate this right now: this is something too tricky to be able to review by looking at the code itself. I need to test this out in the portal to be sure what happens. What I'm curious about though is: why do you "reset" the Application to previous? Why can't you just clear it properly (e.g. setting the threadlocals really to null) instead? I'm willing to test this out and see how it works when running as a portlet, but it'll have to wait until after the ApacheCon (so next week the earliest). For now, you're changes should be fine for plain webapps as the default non-portlet RequestContext instance doesn't keep any state anyway (the PortletRequestContext does however).
        Hide
        Johan Compagner added a comment -

        We did clean it up, but the problem is that it is already set for the first request
        As i said doing this:

        httpServletResponse.sendError(errorCode)

        will result in a redispatch of that code immediately so inside that call this is sort of the stacktrace:

        wicketfilter.doget()
        wicketfilter.dofilter() -> does the second request
        jettycode
        httpServletResponse.sendError(errorCode)
        WebErrorResponseRequestTarget.response()
        wicketfilter.doget()
        wicketfilter.dofilter() -> does the first request

        so you see if we would cleanup ALL the thread locals in the second request wicketfilter.
        We have a problem when the WebErrorResponseRequestTarget.response() comes back
        Because everything is cleared, The Application is gone the RequestCycle is gone.
        So everything that comes after that (detach and so on) will bomb out because there are no more threadlocals.

        Show
        Johan Compagner added a comment - We did clean it up, but the problem is that it is already set for the first request As i said doing this: httpServletResponse.sendError(errorCode) will result in a redispatch of that code immediately so inside that call this is sort of the stacktrace: wicketfilter.doget() wicketfilter.dofilter() -> does the second request jettycode httpServletResponse.sendError(errorCode) WebErrorResponseRequestTarget.response() wicketfilter.doget() wicketfilter.dofilter() -> does the first request so you see if we would cleanup ALL the thread locals in the second request wicketfilter. We have a problem when the WebErrorResponseRequestTarget.response() comes back Because everything is cleared, The Application is gone the RequestCycle is gone. So everything that comes after that (detach and so on) will bomb out because there are no more threadlocals.
        Hide
        Johan Compagner added a comment -

        Also the discussion there is about session leakage and so on on the mailing list
        i also did get some mails where people where checking Application/Session/RequestCycle and RequestContext
        if they where still there at points (complete beginning of a request)
        And they told me that RequestContext was still there quite a lot.. So that seems wrong somehow i am also investigation that some more
        But if you could check for places where the RequestContext is not cleared (and application and session are) ...

        Show
        Johan Compagner added a comment - Also the discussion there is about session leakage and so on on the mailing list i also did get some mails where people where checking Application/Session/RequestCycle and RequestContext if they where still there at points (complete beginning of a request) And they told me that RequestContext was still there quite a lot.. So that seems wrong somehow i am also investigation that some more But if you could check for places where the RequestContext is not cleared (and application and session are) ...
        Hide
        Igor Vaynberg added a comment -

        where are we with this johan?

        Show
        Igor Vaynberg added a comment - where are we with this johan?
        Hide
        Johan Compagner added a comment -

        i am closing this one, should be fixed

        Show
        Johan Compagner added a comment - i am closing this one, should be fixed

          People

          • Assignee:
            Johan Compagner
            Reporter:
            Michael Sparer
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development