Tapestry 5
  1. Tapestry 5
  2. TAP5-986

A request can fail with an NPE in some cases, when a Tapestry page is acting as the servlet container error page

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 5.1.0.5
    • Fix Version/s: 5.3.6, 5.4
    • Component/s: tapestry-core
    • Labels:
      None

      Description

      With this kind of configuration in web.xml :

      <filter-mapping>
      <filter-name>tapestryFilter</filter-name>
      <url-pattern>/*</url-pattern>
      <dispatcher>ERROR</dispatcher>
      <dispatcher>REQUEST</dispatcher>
      </filter-mapping>
      <error-page>
      <error-code>403</error-code>
      <location>/error/AccessDenied</location>
      </error-page>
      <error-page>
      <error-code>404</error-code>
      <location>/error/NotFound</location>
      </error-page>

      RestoreDirtySessionObjects is generating a NullPointerException with this line :

      Session session = request.getSession(false);

      It seems that the dispatching is done in one single thread, then the initial class to RestoreDirtySessionObjects is delay, and request object is lost.

      1. TAP5-986.txt
        25 kB
        Christophe Cordenier

        Issue Links

          Activity

          Hide
          Kalle Korhonen added a comment -

          The changed summary is wrong - the request doesn't fail with NPE.

          Show
          Kalle Korhonen added a comment - The changed summary is wrong - the request doesn't fail with NPE.
          Hide
          Kalle Korhonen added a comment -

          Fixed as suggested.

          Show
          Kalle Korhonen added a comment - Fixed as suggested.
          Hide
          Kalle Korhonen added a comment -

          This happens at least on Jetty and all versions, including 5.4 snapshots. The issue is related to servlet standard error pages and how (at least) Jetty handles error dispatches. You'll get the exception whenever a response.sendError is invoked, because the container's error dispatcher immediately starts processing the error request in the same thread. We probably don't see this in Tomcat because it likely issues a new thread for processing the error request. Anyway, Tapestry successfully cleans up the thread and ends processing the request, after which the original request finishes up, invoking EndOfRequestEventHub.fire() but the thread is already cleaned up. That also explains why everything still works even though you see the exception. A corrected, but naive implementation of EndOfRequestEventHub would check if the request still exists, for example (note the added "if (requestGlobals.getRequest() == null) return;" in fire() ):

          @ServiceId("GuardedEndOfRequestEventHub")
          public EndOfRequestEventHub buildEndOfRequestEventHub(final RequestGlobals requestGlobals) {
          return new EndOfRequestEventHub() {
          private final List<EndOfRequestListener> listeners = CollectionFactory.newThreadSafeList();

          public void addEndOfRequestListener(EndOfRequestListener listener)

          { listeners.add(listener); }

          public void removeEndOfRequestListener(EndOfRequestListener listener)

          { listeners.remove(listener); }

          public void fire() {
          if (requestGlobals.getRequest() == null) return;
          for (EndOfRequestListener l : listeners)

          { l.requestDidComplete(); }

          }
          };
          }

          It's easy to override the original EndOfRequestEventHub since it has no dependencies. That's just stopgap measure, I'll bring the topic of multiple container dispatches on the dev list as it may imply a larger issue with the expectations T5 makes about the isolation level of container threads.

          Show
          Kalle Korhonen added a comment - This happens at least on Jetty and all versions, including 5.4 snapshots. The issue is related to servlet standard error pages and how (at least) Jetty handles error dispatches. You'll get the exception whenever a response.sendError is invoked, because the container's error dispatcher immediately starts processing the error request in the same thread. We probably don't see this in Tomcat because it likely issues a new thread for processing the error request. Anyway, Tapestry successfully cleans up the thread and ends processing the request, after which the original request finishes up, invoking EndOfRequestEventHub.fire() but the thread is already cleaned up. That also explains why everything still works even though you see the exception. A corrected, but naive implementation of EndOfRequestEventHub would check if the request still exists, for example (note the added "if (requestGlobals.getRequest() == null) return;" in fire() ): @ServiceId("GuardedEndOfRequestEventHub") public EndOfRequestEventHub buildEndOfRequestEventHub(final RequestGlobals requestGlobals) { return new EndOfRequestEventHub() { private final List<EndOfRequestListener> listeners = CollectionFactory.newThreadSafeList(); public void addEndOfRequestListener(EndOfRequestListener listener) { listeners.add(listener); } public void removeEndOfRequestListener(EndOfRequestListener listener) { listeners.remove(listener); } public void fire() { if (requestGlobals.getRequest() == null) return; for (EndOfRequestListener l : listeners) { l.requestDidComplete(); } } }; } It's easy to override the original EndOfRequestEventHub since it has no dependencies. That's just stopgap measure, I'll bring the topic of multiple container dispatches on the dev list as it may imply a larger issue with the expectations T5 makes about the isolation level of container threads.
          Hide
          Sebastian Arming added a comment -

          Yes, the HttpError from TAP5-1174 leads to the same problem in 5.2.0

          Show
          Sebastian Arming added a comment - Yes, the HttpError from TAP5-1174 leads to the same problem in 5.2.0
          Hide
          Josh Canfield added a comment -

          In 5.2 there is an HttpError object that can be returned by an event handler to send an error response to the client. Does that meet the requirements of this defect?

          Show
          Josh Canfield added a comment - In 5.2 there is an HttpError object that can be returned by an event handler to send an error response to the client. Does that meet the requirements of this defect?
          Hide
          Christophe Cordenier added a comment -

          Duplicated here TAP5-1017

          Show
          Christophe Cordenier added a comment - Duplicated here TAP5-1017
          Hide
          Christophe Cordenier added a comment -

          Actually, after more test this solution does not work, i am still looking for a solution.

          RestoreDirtySessionObjects.requestDidComplete still generate a NullPointerException for the first reques i guess.

          Show
          Christophe Cordenier added a comment - Actually, after more test this solution does not work, i am still looking for a solution. RestoreDirtySessionObjects.requestDidComplete still generate a NullPointerException for the first reques i guess.
          Hide
          Christophe Cordenier added a comment -

          After more test i have seen that my sendError request bypass default ErrorRequestFilter by catching throwable, new code is :

          RequestFilter sendErrorFilter = new RequestFilter() {
          public boolean service(Request request, Response response, RequestHandler handler) throws IOException {
          try

          { return handler.service(request, response); }

          catch (HttpErrorException htex)

          { response.sendError(htex.getHttpError().getStatus(), htex.getHttpError().getMessage()); return true; }

          }
          };

          Show
          Christophe Cordenier added a comment - After more test i have seen that my sendError request bypass default ErrorRequestFilter by catching throwable, new code is : RequestFilter sendErrorFilter = new RequestFilter() { public boolean service(Request request, Response response, RequestHandler handler) throws IOException { try { return handler.service(request, response); } catch (HttpErrorException htex) { response.sendError(htex.getHttpError().getStatus(), htex.getHttpError().getMessage()); return true; } } };
          Hide
          Christophe Cordenier added a comment -

          About the patch :

          I have implement a ComponentEventResultProcessor that throw an exception when the type return is an instance of HttpError.

          The sendError call is effectively made in a RequestFilter so the RestoreDirtySessionObjects dirty is bypassed like as RequestErrorFilter does (return true in catch Throwable clause)

          One advantage of this solution is that it's easy for the developper to send an error without injecting Response service in its page.

          Show
          Christophe Cordenier added a comment - About the patch : I have implement a ComponentEventResultProcessor that throw an exception when the type return is an instance of HttpError. The sendError call is effectively made in a RequestFilter so the RestoreDirtySessionObjects dirty is bypassed like as RequestErrorFilter does (return true in catch Throwable clause) One advantage of this solution is that it's easy for the developper to send an error without injecting Response service in its page.

            People

            • Assignee:
              Kalle Korhonen
              Reporter:
              Christophe Cordenier
            • Votes:
              3 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development