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

          Christophe Cordenier created issue -
          Christophe Cordenier made changes -
          Field Original Value New Value
          Attachment TAP5-986.txt [ 12430259 ]
          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.
          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 -

          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 -

          Duplicated here TAP5-1017

          Show
          Christophe Cordenier added a comment - Duplicated here TAP5-1017
          Christophe Cordenier made changes -
          Status Open [ 1 ] Closed [ 6 ]
          Resolution Duplicate [ 3 ]
          Ulrich Stärk made changes -
          Link This issue is duplicated by TAP5-1017 [ TAP5-1017 ]
          Ulrich Stärk made changes -
          Resolution Duplicate [ 3 ]
          Status Closed [ 6 ] Reopened [ 4 ]
          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
          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
          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.
          Kalle Korhonen made changes -
          Assignee Kalle Korhonen [ kaosko ]
          Hide
          Kalle Korhonen added a comment -

          Fixed as suggested.

          Show
          Kalle Korhonen added a comment - Fixed as suggested.
          Kalle Korhonen made changes -
          Status Reopened [ 4 ] Resolved [ 5 ]
          Fix Version/s 5.3.6 [ 12322961 ]
          Fix Version/s 5.4 [ 12316401 ]
          Resolution Fixed [ 1 ]
          Howard M. Lewis Ship made changes -
          Summary sendError in onActivate / Tapestry error dispatching A request can fail with an NPE in some cases, when a Tapestry page is acting as the servlet container error page
          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.
          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Closed Closed
          31d 18h 32m 1 Christophe Cordenier 15/Feb/10 09:13
          Closed Closed Reopened Reopened
          17m 8s 1 Ulrich Stärk 15/Feb/10 09:30
          Reopened Reopened Resolved Resolved
          960d 19h 38m 1 Kalle Korhonen 03/Oct/12 06:08

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development