Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.2
    • Fix Version/s: 2.2
    • Labels:
      None
    • Affects version (Component):
      Servlet Service Framework - 1.0.0-RC2-dev
    • Fix version (Component):
      Servlet Service Framework - 1.0.0-RC2-dev

      Description

      This is the exception shown on the console:

      java.lang.IllegalStateException: Committed
              at org.mortbay.jetty.Response.resetBuffer(Response.java:855)
              at org.mortbay.jetty.Response.reset(Response.java:834)
              at javax.servlet.ServletResponseWrapper.reset(ServletResponseWrapper.java:182)
              at org.apache.cocoon.servletservice.ServletServiceContext$PathDispatcher.forward(ServletServiceContext.java:576)
              at org.apache.cocoon.servletservice.ServletServiceContext$PathDispatcher.forward(ServletServiceContext.java:545)
              at org.apache.cocoon.servletservice.spring.ServletFactoryBean$ServiceInterceptor.invoke(ServletFactoryBean.java:230)
              at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:171)
              at org.springframework.aop.framework.JdkDynamicAopProxy.invoke(JdkDynamicAopProxy.java:204)
              at $Proxy2.service(Unknown Source)
              at org.apache.cocoon.servletservice.DispatcherServlet.service(DispatcherServlet.java:102)
              at javax.servlet.http.HttpServlet.service(HttpServlet.java:802)
              at org.mortbay.jetty.servlet.ServletHolder.handle(ServletHolder.java:459)

      It seems to be thrown whenever the response object is reseted after the actual response has been sent by the sitemap error handler. In this case reset is no longer possible since the response has already been committed as stated in the error message.

        Activity

        Hide
        Jörg Heinicke added a comment -
        Reinhard mentioned the suspicion about the error handler: http://marc.info/?l=xml-cocoon-dev&m=119582926910325&w=4.
        I can confirm it in my case (http://marc.info/?t=119580475700001&r=1&w=4) since I got a proper error message returned to the browser (i.e. response was sent by error handler).
        Show
        Jörg Heinicke added a comment - Reinhard mentioned the suspicion about the error handler: http://marc.info/?l=xml-cocoon-dev&m=119582926910325&w=4 . I can confirm it in my case ( http://marc.info/?t=119580475700001&r=1&w=4 ) since I got a proper error message returned to the browser (i.e. response was sent by error handler).
        Hide
        Carsten Ziegeler added a comment -
        If an error handler has already written to the response, than the error has been handled. In this case no exception should be thrown.
        I don't know if this is the case and I've no
        idea what the PathDispatcher.forward method really does and why it forwards to super in case of an exception.
        But I think the simplest fix for this is to check if the response has already been committed and only if not, it should reset it and forward to super.

        But it seems to me that the whole processing should be checked.
        Show
        Carsten Ziegeler added a comment - If an error handler has already written to the response, than the error has been handled. In this case no exception should be thrown. I don't know if this is the case and I've no idea what the PathDispatcher.forward method really does and why it forwards to super in case of an exception. But I think the simplest fix for this is to check if the response has already been committed and only if not, it should reset it and forward to super. But it seems to me that the whole processing should be checked.
        Hide
        Carsten Ziegeler added a comment -
        Looking again at the code, I guess that the problematic part is the check of the status code in ServletServiceContext.java:575.

        If an error handler has been running it might have set the error code to 500.

        I'm wondering why this status code checking is done anyway?
        Show
        Carsten Ziegeler added a comment - Looking again at the code, I guess that the problematic part is the check of the status code in ServletServiceContext.java:575. If an error handler has been running it might have set the error code to 500. I'm wondering why this status code checking is done anyway?
        Hide
        Grzegorz Kossakowski added a comment -
        This should be fixed before we make a final release of 2.2.
        Show
        Grzegorz Kossakowski added a comment - This should be fixed before we make a final release of 2.2.
        Hide
        Grzegorz Kossakowski added a comment -
        I isolated the problem and committed additional checks that if uncommented will exhibit the problematic behaviour.

        The offending code snippet is (ServletServiceContext:507):
                        if (se != null || (status < 200 || status >= 400)) {
                            wrappedResponse.reset();
                            NamedDispatcher _super = (NamedDispatcher) ServletServiceContext.this.getNamedDispatcher(SUPER);
                            if (_super != null) {
                                _super.forward(request, wrappedResponse);
                            } else {
                                wrappedResponse.getWriter().println("Resource not found");
                                wrappedResponse.setStatus(HttpServletResponse.SC_NOT_FOUND);
                                throw se;
                            }
                        }

        Since this code resets response (which is unavoidable) I think we will need to buffer response and let it commit until ServletServiceContext checks are done. At least we would need to buffer response if status code is 404 (and possibly other of similar meaning).

        WDYT?
        Show
        Grzegorz Kossakowski added a comment - I isolated the problem and committed additional checks that if uncommented will exhibit the problematic behaviour. The offending code snippet is (ServletServiceContext:507):                 if (se != null || (status < 200 || status >= 400)) {                     wrappedResponse.reset();                     NamedDispatcher _super = (NamedDispatcher) ServletServiceContext.this.getNamedDispatcher(SUPER);                     if (_super != null) {                         _super.forward(request, wrappedResponse);                     } else {                         wrappedResponse.getWriter().println("Resource not found");                         wrappedResponse.setStatus(HttpServletResponse.SC_NOT_FOUND);                         throw se;                     }                 } Since this code resets response (which is unavoidable) I think we will need to buffer response and let it commit until ServletServiceContext checks are done. At least we would need to buffer response if status code is 404 (and possibly other of similar meaning). WDYT?
        Hide
        Grzegorz Kossakowski added a comment -
        Show
        Grzegorz Kossakowski added a comment - FYI: I'm going to work on fix described in a comment https://issues.apache.org/jira/browse/COCOON-2150?focusedCommentId=12558427#action_12558427
        Hide
        Grzegorz Kossakowski added a comment -
        It looks like I fixed this issue rr615697.

        Please test and reopen if there are still any problems.
        Show
        Grzegorz Kossakowski added a comment - It looks like I fixed this issue rr615697. Please test and reopen if there are still any problems.
        Hide
        Felix Knecht added a comment -
        Make a clean build and
        - pointing browser to http://localhost:8888/ - this looks really ugly
        - pointing browser to http://localhost:8888/samples/forms/form1- throws exceptions, looks ugly as well, form tabs are not working

        Browser: firefox
        Show
        Felix Knecht added a comment - Make a clean build and - pointing browser to http://localhost:8888/ - this looks really ugly - pointing browser to http://localhost:8888/samples/forms/form1- throws exceptions, looks ugly as well, form tabs are not working Browser: firefox
        Hide
        Grzegorz Kossakowski added a comment -
        I confirmed that my fix was wrong in this thread: http://article.gmane.org/gmane.text.xml.cocoon.devel/76831

        Switching back to Major priority because, with reverted commit, this issue no longer breaks people's applications. At least, in general.
        Show
        Grzegorz Kossakowski added a comment - I confirmed that my fix was wrong in this thread: http://article.gmane.org/gmane.text.xml.cocoon.devel/76831 Switching back to Major priority because, with reverted commit, this issue no longer breaks people's applications. At least, in general.
        Hide
        Reinhard Poetz added a comment - - edited
        Grek, any chance to get the setting of the status code fixed for the upcoming 1.0.0 release? I don't think it's a good idea to make a final release while this important issue is open. WDYT?
        Show
        Reinhard Poetz added a comment - - edited Grek, any chance to get the setting of the status code fixed for the upcoming 1.0.0 release? I don't think it's a good idea to make a final release while this important issue is open. WDYT?
        Hide
        Rice Yeh added a comment -
        I do not know whether I am naive to say just adding !wrappedResponse.isCommited() on the if statement on line 482 in ServletServiceContext.java may solve the problem. The following is the patch.

        Index: C:/coffee/cocoon/trunk/core/cocoon-servlet-service/cocoon-servlet-service-impl/src/main/java/org/apache/cocoon/servletservice/ServletServiceContext.java
        ===================================================================
        --- C:/coffee/cocoon/trunk/core/cocoon-servlet-service/cocoon-servlet-service-impl/src/main/java/org/apache/cocoon/servletservice/ServletServiceContext.java (revision 635788)
        +++ C:/coffee/cocoon/trunk/core/cocoon-servlet-service/cocoon-servlet-service-impl/src/main/java/org/apache/cocoon/servletservice/ServletServiceContext.java (working copy)
        @@ -479,7 +479,7 @@
                         }
         
                         int status = wrappedResponse.getStatus();
        - if (se != null || (status < 200 || status >= 400)) {
        + if (!wrappedResponse.isCommitted() && (se != null || (status < 200 || status >= 400))) {
                             wrappedResponse.reset();
                             NamedDispatcher _super = (NamedDispatcher) ServletServiceContext.this.getNamedDispatcher(SUPER);
                             if (_super != null) {
        Show
        Rice Yeh added a comment - I do not know whether I am naive to say just adding !wrappedResponse.isCommited() on the if statement on line 482 in ServletServiceContext.java may solve the problem. The following is the patch. Index: C:/coffee/cocoon/trunk/core/cocoon-servlet-service/cocoon-servlet-service-impl/src/main/java/org/apache/cocoon/servletservice/ServletServiceContext.java =================================================================== --- C:/coffee/cocoon/trunk/core/cocoon-servlet-service/cocoon-servlet-service-impl/src/main/java/org/apache/cocoon/servletservice/ServletServiceContext.java (revision 635788) +++ C:/coffee/cocoon/trunk/core/cocoon-servlet-service/cocoon-servlet-service-impl/src/main/java/org/apache/cocoon/servletservice/ServletServiceContext.java (working copy) @@ -479,7 +479,7 @@                  }                    int status = wrappedResponse.getStatus(); - if (se != null || (status < 200 || status >= 400)) { + if (!wrappedResponse.isCommitted() && (se != null || (status < 200 || status >= 400))) {                      wrappedResponse.reset();                      NamedDispatcher _super = (NamedDispatcher) ServletServiceContext.this.getNamedDispatcher(SUPER);                      if (_super != null) {
        Hide
        Grzegorz Kossakowski added a comment -
        @Reinhard: I agree that it would be the best to fix this one before final release. Unfortunately, this involves implementing special proxy class for HttpResponse that will buffer whole response. It's not that hard but it demands a little of work and testing. I could have a look at this on Friday (unfortunely, I have a lot of subjects at my university in this term...).

        @Rice: No, this is not a good fix because response must be always discarded no matter if it's been already comitted or not. Hence the need for buffering response that will not commit anything to proxied response object until it's explicitly asked by ServletServiceContext class.
        Show
        Grzegorz Kossakowski added a comment - @Reinhard: I agree that it would be the best to fix this one before final release. Unfortunately, this involves implementing special proxy class for HttpResponse that will buffer whole response. It's not that hard but it demands a little of work and testing. I could have a look at this on Friday (unfortunely, I have a lot of subjects at my university in this term...). @Rice: No, this is not a good fix because response must be always discarded no matter if it's been already comitted or not. Hence the need for buffering response that will not commit anything to proxied response object until it's explicitly asked by ServletServiceContext class.
        Hide
        Jörg Heinicke added a comment -
        I wonder what's the point of creating a response that will always be discarded.

        For the buffering remember COCOON-2168: Cocoon has already a BufferedOutputStream that never flushes - with all the side effects like OOME.
        Show
        Jörg Heinicke added a comment - I wonder what's the point of creating a response that will always be discarded. For the buffering remember COCOON-2168 : Cocoon has already a BufferedOutputStream that never flushes - with all the side effects like OOME.
        Hide
        Grzegorz Kossakowski added a comment -
        @Joerg: See my response to Vadim who raised exactly the same concerns like some of you: http://article.gmane.org/gmane.text.xml.cocoon.devel/77006

        I'll buffer only response if the response code has been set to 404 and will set sane default (like 1MB) of a buffer to avoid OOMEs. If you are generating more than 1MB response with 404 code then it's definitively a problem with your application and FATAL ERROR will be logged.

        Does it sound fine?
        Show
        Grzegorz Kossakowski added a comment - @Joerg: See my response to Vadim who raised exactly the same concerns like some of you: http://article.gmane.org/gmane.text.xml.cocoon.devel/77006 I'll buffer only response if the response code has been set to 404 and will set sane default (like 1MB) of a buffer to avoid OOMEs. If you are generating more than 1MB response with 404 code then it's definitively a problem with your application and FATAL ERROR will be logged. Does it sound fine?
        Hide
        Grzegorz Kossakowski added a comment -
        FYI: I've been working on this today and I have implemented everything I planned in order to resolve this issue. The only remaining problem is that even our test-cases covering this issue pass there are some edge cases when I observe that Cocoon puts my proxy class into illegal state. It's already very late here in Poland so I'll have a look at this tomorrow.
        Show
        Grzegorz Kossakowski added a comment - FYI: I've been working on this today and I have implemented everything I planned in order to resolve this issue. The only remaining problem is that even our test-cases covering this issue pass there are some edge cases when I observe that Cocoon puts my proxy class into illegal state. It's already very late here in Poland so I'll have a look at this tomorrow.
        Hide
        Grzegorz Kossakowski added a comment -
        Fixed (hopefully) definitively in r637416, r637417, r637418 and r637419.
        Show
        Grzegorz Kossakowski added a comment - Fixed (hopefully) definitively in r637416, r637417, r637418 and r637419.

          People

          • Assignee:
            Grzegorz Kossakowski
            Reporter:
            Jörg Heinicke
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development