Velocity Tools
  1. Velocity Tools
  2. VELTOOLS-115

Improve exception and HTTP error management

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0
    • Component/s: VelocityView
    • Labels:
      None
    • Environment:
      Servlet container

      Description

      Currently the exception management and missing resources are not satisfactory.
      In particular:
      1. if an exception happen during rendering a template, an exception stack trace is put inside the result, with an HTTP 200 code;
      2. if a resource is missing, but it is intercepted by the VelocityViewServlet, it renders with a page with HTTP 200.

      It should be:
      1. HTTP 500 with error page. If it is not possible (if the response is committed), the error page should be included.
      2. HTTP 404 with error page.
      In both cases the configuration of the webapp should be considered.

      1. veltools-exception.diff
        3 kB
        Antonio Petrelli
      2. veltools-exception.diff
        6 kB
        Antonio Petrelli

        Activity

        Hide
        Antonio Petrelli added a comment -

        FreeMarker servlet works this way, I suppose that I could grab a bit of code (it's BSD-licensed after all).

        Show
        Antonio Petrelli added a comment - FreeMarker servlet works this way, I suppose that I could grab a bit of code (it's BSD-licensed after all).
        Hide
        Claude Brisson added a comment -

        This would rather target the VelocityLayoutServlet (which for now sends back a standard error template).

        Since by default the VelocityViewServlet will stream out directly the response without any buffering, the HTTP headers have already been sent at the moment the exception is thrown in the template.

        Anyway, this feature can only exists if you bufferize the response.

        Show
        Claude Brisson added a comment - This would rather target the VelocityLayoutServlet (which for now sends back a standard error template). Since by default the VelocityViewServlet will stream out directly the response without any buffering, the HTTP headers have already been sent at the moment the exception is thrown in the template. Anyway, this feature can only exists if you bufferize the response.
        Hide
        Antonio Petrelli added a comment -

        Agreed. Miracles are possible, but not in Java :-D
        Anyway I think that template syntax errors can be catched. Moreover, doing it "in a standard way" (like JSP and FreeMarker do, for example), i.e. including a page error in case the response has been committed, should be feasible.

        Show
        Antonio Petrelli added a comment - Agreed. Miracles are possible, but not in Java :-D Anyway I think that template syntax errors can be catched. Moreover, doing it "in a standard way" (like JSP and FreeMarker do, for example), i.e. including a page error in case the response has been committed, should be feasible.
        Hide
        Byron Foster added a comment -

        It seems like the default behavior should be to buffer the entire merge before you write it to the response. I'm not sure how else you correctly handle errors if you don't do this. I don't use Velocity servlets, but in my own usage this is what I do.

        What do you mean include a page error?

        Show
        Byron Foster added a comment - It seems like the default behavior should be to buffer the entire merge before you write it to the response. I'm not sure how else you correctly handle errors if you don't do this. I don't use Velocity servlets, but in my own usage this is what I do. What do you mean include a page error?
        Hide
        Antonio Petrelli added a comment -

        1. You are right that we cannot send an HTTP 500 when the response is committed. The response is committed when you flush the PrintWriter of the response (or the PrintWriter is auto-flushed):
        http://java.sun.com/javaee/5/docs/api/javax/servlet/ServletResponse.html#getWriter()
        And since the VelocityWriter is used to wrap the response's writer (or better, the writer that is configured through the "bufferOutput" parameter, that could be a StringWriter or the response's writer), that buffer the output itself, then there may be various cases in which the response may be uncommitted.

        2. By "including a page error" I mean something like how PageContextImpl of Tomcat does it:
        http://svn.eu.apache.org/repos/asf/tomcat/tc6.0.x/trunk/java/org/apache/jasper/runtime/PageContextImpl.java
        See the "doHandlePageException": it tries to forward to the error page, if it fails it includes the error page.

        3. I have a suspect though: if the exception is left "popping up" at servlet level, without catching, what happens? Does it handle the exceptions like I depicted here?

        Show
        Antonio Petrelli added a comment - 1. You are right that we cannot send an HTTP 500 when the response is committed. The response is committed when you flush the PrintWriter of the response (or the PrintWriter is auto-flushed): http://java.sun.com/javaee/5/docs/api/javax/servlet/ServletResponse.html#getWriter( ) And since the VelocityWriter is used to wrap the response's writer (or better, the writer that is configured through the "bufferOutput" parameter, that could be a StringWriter or the response's writer), that buffer the output itself, then there may be various cases in which the response may be uncommitted. 2. By "including a page error" I mean something like how PageContextImpl of Tomcat does it: http://svn.eu.apache.org/repos/asf/tomcat/tc6.0.x/trunk/java/org/apache/jasper/runtime/PageContextImpl.java See the "doHandlePageException": it tries to forward to the error page, if it fails it includes the error page. 3. I have a suspect though: if the exception is left "popping up" at servlet level, without catching, what happens? Does it handle the exceptions like I depicted here?
        Hide
        Byron Foster added a comment -

        Thanks for the links. If you don't catch the exceptions as you describe in (3), then the exceptions will propagate up into Tomcat, which will give you behavior (2). I agree that if possible, the response is not commited, then the HTTP 200 OK status should not be returned if errors occur.

        Show
        Byron Foster added a comment - Thanks for the links. If you don't catch the exceptions as you describe in (3), then the exceptions will propagate up into Tomcat, which will give you behavior (2). I agree that if possible, the response is not commited, then the HTTP 200 OK status should not be returned if errors occur.
        Hide
        Nathan Bubna added a comment -

        And for the record, i don't particularly care anymore whether the output is buffered by default or not. So, if you guys thing we should change that default, i've got no problem with that.

        Show
        Nathan Bubna added a comment - And for the record, i don't particularly care anymore whether the output is buffered by default or not. So, if you guys thing we should change that default, i've got no problem with that.
        Hide
        Byron Foster added a comment -

        Looking at this some more. I don't like what I originally said about buffering. The response buffer size can be set manually to control how much data is written before the servlet container commits. The point being is why perform any local buffering at all? Is the VelocityWriter even necessary in a servlet container? A servlet container like Tomcat should provide any buffer functionality necessary, and probably does it better (memory management, etc...), sorry this is a little off topic, but related

        Show
        Byron Foster added a comment - Looking at this some more. I don't like what I originally said about buffering. The response buffer size can be set manually to control how much data is written before the servlet container commits. The point being is why perform any local buffering at all? Is the VelocityWriter even necessary in a servlet container? A servlet container like Tomcat should provide any buffer functionality necessary, and probably does it better (memory management, etc...), sorry this is a little off topic, but related
        Hide
        Antonio Petrelli added a comment -

        Byron, can you open a new issue about buffering, or write to the Velocity Developers mailing list?
        I think that we should discuss it further.

        Thanks

        Show
        Antonio Petrelli added a comment - Byron, can you open a new issue about buffering, or write to the Velocity Developers mailing list? I think that we should discuss it further. Thanks
        Hide
        Antonio Petrelli added a comment -

        Attached patch to fix exception management.
        If the response is not committed, let the exception pass.
        Otherwise, the exception is logged the usual way, but does not go ahead rendering the page.

        Show
        Antonio Petrelli added a comment - Attached patch to fix exception management. If the response is not committed, let the exception pass. Otherwise, the exception is logged the usual way, but does not go ahead rendering the page.
        Hide
        Antonio Petrelli added a comment -

        Just another comment: notice that this patch does not fix the 404 problem, but only the 500 one.

        Show
        Antonio Petrelli added a comment - Just another comment: notice that this patch does not fix the 404 problem, but only the 500 one.
        Hide
        Antonio Petrelli added a comment -

        A new version of the patch, in which HTTP 404 messages are managed.

        Show
        Antonio Petrelli added a comment - A new version of the patch, in which HTTP 404 messages are managed.
        Hide
        Antonio Petrelli added a comment -

        Don't you like the patch? Please tell me what's wrong, so I can fix it.

        Show
        Antonio Petrelli added a comment - Don't you like the patch? Please tell me what's wrong, so I can fix it.
        Hide
        Nathan Bubna added a comment -

        Sorry, Antonio, this somehow missed getting on my todo list. Added now...

        Show
        Nathan Bubna added a comment - Sorry, Antonio, this somehow missed getting on my todo list. Added now...
        Hide
        Nathan Bubna added a comment -

        Thanks, Antonio!

        Show
        Nathan Bubna added a comment - Thanks, Antonio!

          People

          • Assignee:
            Unassigned
            Reporter:
            Antonio Petrelli
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development