Uploaded image for project: 'Repose'
  1. REP-6588

commitToResponse Method of the Response Wrapper Throws An Exception in Glassfish

    Details

    • Type: Story
    • Status: Resolved (View workflow)
    • Priority: Medium
    • Resolution: Done
    • Affects Version/s: None
    • Fix Version/s: 8.8.2.0
    • Component/s: None
    • Labels:
      None
    • Sprint:
      Sprint 162, Sprint 163, Sprint 164
    • Story Points:
      2
    • Capitalizable:
      True

      Description

      The commitToResponse method of our response wrapper needs to be updated so that the body is not written before calling the sendError method, if applicable, on the wrapped response. The current order causes Tomcat's Response object to consider itself committed before sendError is called, leading to an IllegalStateException being thrown.

      We do not believe that the servlet specification states how to handle any written body when sendError is called. As a result, Tomcat handles it differently from Jetty. With Jetty, we do not have an issue, while with Tomcat, we do.

      Context:
      https://rackspace.slack.com/archives/G11GUQJ9Z/p1517868791000092

      Damien Johnson [4:13 PM]
      @adrian.george I think that's it. Check this out:
      http://grepcode.com/file/repo1.maven.org/maven2/org.apache.tomcat/tomcat-catalina/7.0.42/org/apache/catalina/connector/Response.java#Response.isAppCommitted%28%29
      grepcode.com
      GC: Response - org.apache.catalina.connector.Response (.java) - GrepCode Class Source
      org.apache.catalina.connector.Response - Wrapper object for the Coyote response
      So I don't know what version of Tomcat is being used, but...

      Damien Johnson [4:20 PM]
      As part of writing the body, we totally set the content length and write the content:
      https://github.com/rackerlabs/repose/blob/8.7.3.0/repose-aggregator/commons/commons-utilities/src/main/scala/org/openrepose/commons/utils/servlet/http/HttpServletResponseWrapper.scala#L492-L496 (edited)
      GitHub
      rackerlabs/repose
      repose - The powerful, programmable, API Middleware Platform
      Oh, I should note that the `ResponseFacade` calls through to `Response`, which is why I linked that class.
      That is, `ResponseFacade.sendError` calls `isCommitted` which is this:
      http://grepcode.com/file/repo1.maven.org/maven2/org.apache.tomcat/tomcat-catalina/7.0.42/org/apache/catalina/connector/ResponseFacade.java#334
      grepcode.com
      GC: ResponseFacade - org.apache.catalina.connector.ResponseFacade (.java) - GrepCode Class Source
      org.apache.catalina.connector.ResponseFacade - Facade class that wraps a Coyote response object
      So there you go. Change the order of things. If we are sending an error, don't write the body, otherwise do.
      For a little more context for everyone else, check out my earlier comment about calling `uncommit` before `commitToResponse`.
      The key was that `uncommit` was not failing even though it checks if the underlying response was already committed, but `commitToResponse` was.
      ... the quick and dirty is that the Tomcat response object considers itself committed if content length has been set and content has been written to the output stream, and the order in which we do things when we `commitToResponse` writes what we have for the body before calling `sendError`. That `sendError` call is where we fail.
      Oh, and we suspect this is not a problem in Valve because the Jetty response object is not as strict.
      Which is also why we don't have tests that caught this.

      Acceptance Criteria:

      • Our servlet wrapper supports calling sendError when running in a Glassfish container.

        Attachments

          Issue links

            Activity

              People

              • Assignee:
                damien.johnson Damien Johnson
                Reporter:
                damien.johnson Damien Johnson
              • Votes:
                0 Vote for this issue
                Watchers:
                1 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: