Shindig
  1. Shindig
  2. SHINDIG-1770

Improve JsServlet to respond with an error and error message if there was a problem

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.5.0
    • Fix Version/s: 2.5.0-beta2
    • Component/s: Java
    • Labels:
      None

      Description

      Currently if JsServlet encounters an error processing a response, such as a closure compile error, it will simply respond with the error status code but won't respond with an error message. To improve this behavior, I'd like to have the servlet send a proper error response with the error status code and the error string.

      1. shindig-1770.patch
        3 kB
        Stanton Sievers

        Activity

        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/5070/#review7802
        -----------------------------------------------------------

        Ship it!

        Committed revision 1337115.

        • Stanton

        On 2012-05-08 19:17:32, Stanton Sievers wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/5070/

        -----------------------------------------------------------

        (Updated 2012-05-08 19:17:32)

        Review request for shindig.

        Summary

        -------

        I had initially made the change in JsServlet itself to check for errors before calling emitJsResponse, however, DefaultJsServingPipeline.process() seemed like a better place. JsServingPipeline.process' javadoc indicates the method should throw an exception if any of the processing steps resulted in an error, so it seems reasonable to check for errors and throw before returning.

        Jira text:

        Currently if JsServlet encounters an error processing a response, such as a closure compile error, it will simply respond with the error status code but won't respond with an error message. To improve this behavior, I'd like to have the servlet send a proper error response with the error status code and the error string.

        This addresses bug SHINDIG-1770.

        https://issues.apache.org/jira/browse/SHINDIG-1770

        Diffs

        -----

        http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DefaultJsServingPipeline.java 1335416

        http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/JsServletTest.java 1335416

        Diff: https://reviews.apache.org/r/5070/diff

        Testing

        -------

        Added a new test in JsServlet to ensure the error makes it through.

        Manually introduced JavaScript errors that would cause Closure to complain and verified that the error text and status code made it through when hitting the JsServlet in the browser.

        Thanks,

        Stanton

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5070/#review7802 ----------------------------------------------------------- Ship it! Committed revision 1337115. Stanton On 2012-05-08 19:17:32, Stanton Sievers wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5070/ ----------------------------------------------------------- (Updated 2012-05-08 19:17:32) Review request for shindig. Summary ------- I had initially made the change in JsServlet itself to check for errors before calling emitJsResponse, however, DefaultJsServingPipeline.process() seemed like a better place. JsServingPipeline.process' javadoc indicates the method should throw an exception if any of the processing steps resulted in an error, so it seems reasonable to check for errors and throw before returning. Jira text: Currently if JsServlet encounters an error processing a response, such as a closure compile error, it will simply respond with the error status code but won't respond with an error message. To improve this behavior, I'd like to have the servlet send a proper error response with the error status code and the error string. This addresses bug SHINDIG-1770 . https://issues.apache.org/jira/browse/SHINDIG-1770 Diffs ----- http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DefaultJsServingPipeline.java 1335416 http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/JsServletTest.java 1335416 Diff: https://reviews.apache.org/r/5070/diff Testing ------- Added a new test in JsServlet to ensure the error makes it through. Manually introduced JavaScript errors that would cause Closure to complain and verified that the error text and status code made it through when hitting the JsServlet in the browser. Thanks, Stanton
        Hide
        Stanton Sievers added a comment -

        Committed revision 1337115.

        Show
        Stanton Sievers added a comment - Committed revision 1337115.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/5070/#review7786
        -----------------------------------------------------------

        Ship it!

        +1

        • Henry

        On 2012-05-08 19:17:32, Stanton Sievers wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/5070/

        -----------------------------------------------------------

        (Updated 2012-05-08 19:17:32)

        Review request for shindig.

        Summary

        -------

        I had initially made the change in JsServlet itself to check for errors before calling emitJsResponse, however, DefaultJsServingPipeline.process() seemed like a better place. JsServingPipeline.process' javadoc indicates the method should throw an exception if any of the processing steps resulted in an error, so it seems reasonable to check for errors and throw before returning.

        Jira text:

        Currently if JsServlet encounters an error processing a response, such as a closure compile error, it will simply respond with the error status code but won't respond with an error message. To improve this behavior, I'd like to have the servlet send a proper error response with the error status code and the error string.

        This addresses bug SHINDIG-1770.

        https://issues.apache.org/jira/browse/SHINDIG-1770

        Diffs

        -----

        http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DefaultJsServingPipeline.java 1335416

        http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/JsServletTest.java 1335416

        Diff: https://reviews.apache.org/r/5070/diff

        Testing

        -------

        Added a new test in JsServlet to ensure the error makes it through.

        Manually introduced JavaScript errors that would cause Closure to complain and verified that the error text and status code made it through when hitting the JsServlet in the browser.

        Thanks,

        Stanton

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5070/#review7786 ----------------------------------------------------------- Ship it! +1 Henry On 2012-05-08 19:17:32, Stanton Sievers wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5070/ ----------------------------------------------------------- (Updated 2012-05-08 19:17:32) Review request for shindig. Summary ------- I had initially made the change in JsServlet itself to check for errors before calling emitJsResponse, however, DefaultJsServingPipeline.process() seemed like a better place. JsServingPipeline.process' javadoc indicates the method should throw an exception if any of the processing steps resulted in an error, so it seems reasonable to check for errors and throw before returning. Jira text: Currently if JsServlet encounters an error processing a response, such as a closure compile error, it will simply respond with the error status code but won't respond with an error message. To improve this behavior, I'd like to have the servlet send a proper error response with the error status code and the error string. This addresses bug SHINDIG-1770 . https://issues.apache.org/jira/browse/SHINDIG-1770 Diffs ----- http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DefaultJsServingPipeline.java 1335416 http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/JsServletTest.java 1335416 Diff: https://reviews.apache.org/r/5070/diff Testing ------- Added a new test in JsServlet to ensure the error makes it through. Manually introduced JavaScript errors that would cause Closure to complain and verified that the error text and status code made it through when hitting the JsServlet in the browser. Thanks, Stanton
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/5070/#review7771
        -----------------------------------------------------------

        Ship it!

        LGTM

        • Ryan

        On 2012-05-08 19:17:32, Stanton Sievers wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/5070/

        -----------------------------------------------------------

        (Updated 2012-05-08 19:17:32)

        Review request for shindig.

        Summary

        -------

        I had initially made the change in JsServlet itself to check for errors before calling emitJsResponse, however, DefaultJsServingPipeline.process() seemed like a better place. JsServingPipeline.process' javadoc indicates the method should throw an exception if any of the processing steps resulted in an error, so it seems reasonable to check for errors and throw before returning.

        Jira text:

        Currently if JsServlet encounters an error processing a response, such as a closure compile error, it will simply respond with the error status code but won't respond with an error message. To improve this behavior, I'd like to have the servlet send a proper error response with the error status code and the error string.

        This addresses bug SHINDIG-1770.

        https://issues.apache.org/jira/browse/SHINDIG-1770

        Diffs

        -----

        http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DefaultJsServingPipeline.java 1335416

        http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/JsServletTest.java 1335416

        Diff: https://reviews.apache.org/r/5070/diff

        Testing

        -------

        Added a new test in JsServlet to ensure the error makes it through.

        Manually introduced JavaScript errors that would cause Closure to complain and verified that the error text and status code made it through when hitting the JsServlet in the browser.

        Thanks,

        Stanton

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5070/#review7771 ----------------------------------------------------------- Ship it! LGTM Ryan On 2012-05-08 19:17:32, Stanton Sievers wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5070/ ----------------------------------------------------------- (Updated 2012-05-08 19:17:32) Review request for shindig. Summary ------- I had initially made the change in JsServlet itself to check for errors before calling emitJsResponse, however, DefaultJsServingPipeline.process() seemed like a better place. JsServingPipeline.process' javadoc indicates the method should throw an exception if any of the processing steps resulted in an error, so it seems reasonable to check for errors and throw before returning. Jira text: Currently if JsServlet encounters an error processing a response, such as a closure compile error, it will simply respond with the error status code but won't respond with an error message. To improve this behavior, I'd like to have the servlet send a proper error response with the error status code and the error string. This addresses bug SHINDIG-1770 . https://issues.apache.org/jira/browse/SHINDIG-1770 Diffs ----- http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DefaultJsServingPipeline.java 1335416 http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/JsServletTest.java 1335416 Diff: https://reviews.apache.org/r/5070/diff Testing ------- Added a new test in JsServlet to ensure the error makes it through. Manually introduced JavaScript errors that would cause Closure to complain and verified that the error text and status code made it through when hitting the JsServlet in the browser. Thanks, Stanton
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-05-08 19:39:58, Dan Dumont wrote:

        > Quick question before I dig in. Do we cache the compile error response so that we don't get hammered trying to compile it over and over again?

        >

        > Did we before your change?

        Stanton Sievers wrote:

        Did some extra testing. I introduced a compile error in embedded-experiences feature and hit this url in my browser: http://localhost:8080/gadgets/js/embedded-experiences.js?c=1&container=default

        I set a breakpoint in JsServlet.doGet() and regardless of whether my patch was applied or not, we were doing the processing every time. The only difference before my patch was that on subsequent requests the browser would actually get a 304 Not Modified back instead of the 404. Is that because of the ETag filter or the cache control headers?

        Response headers before my patch:

        Cache-Control: public,max-age=3600

        Content-Length: 0

        Content-Type: text/html;charset=utf-8

        Date: Wed, 09 May 2012 13:52:15 GMT

        Etag: "d41d8cd98f00b204e9800998ecf8427e"

        Expires: Wed, 09 May 2012 14:52:15 GMT

        Server: Apache-Coyote/1.1

        Response headers after my patch:

        Content-Length: 1636

        Content-Type: text/html;charset=utf-8

        Date: Wed, 09 May 2012 13:59:59 GMT

        Server: Apache-Coyote/1.1

        Henry Saputra wrote:

        Looks like the ClosureJsCompiler cache the compiled result based on key of hash value of the non-compiled JS content.

        Stanton Sievers wrote:

        Good call Henry. I never noticed the ClosureJsCompiler.cacheAndReturnErrorResult() method before, which will be called when the compiler has errors during processing. So on subsequent calls, we will go through the JS processor again, however, the compiler processor will have cached the errors and will return those instead of running the compiler again.

        And it does appear that it's the ETagFilter that is causing the 304 to come back in the case where my patch is not applied.

        Ryan Baxter wrote:

        So we are in fact caching the error response and not recompiling bad JS every time?

        Stanton Sievers wrote:

        Yes, but to be clear, it's being cached in the ClosureJsCompiler, not at the JsServlet level.

        Ryan Baxter wrote:

        So the difference being that the request will still go through all of the processors every time where as if it were cached at the JsServlet level it wouldn't correct?

        Right. My patch does not change any caching behavior. Today we would do the same thing if there were compilation problems, or if any of the processors had errors. I don't know enough about what all of the processors do to say whether this is a bad or a good thing. Some of the processors appear to actually be getting the JS content, so that processor could affect the compilation on subsequent attempts if, for instance, the JS content was different the second time around. These are problems I'm not trying to solve with this patch.

        • Stanton

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/5070/#review7695
        -----------------------------------------------------------

        On 2012-05-08 19:17:32, Stanton Sievers wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/5070/

        -----------------------------------------------------------

        (Updated 2012-05-08 19:17:32)

        Review request for shindig.

        Summary

        -------

        I had initially made the change in JsServlet itself to check for errors before calling emitJsResponse, however, DefaultJsServingPipeline.process() seemed like a better place. JsServingPipeline.process' javadoc indicates the method should throw an exception if any of the processing steps resulted in an error, so it seems reasonable to check for errors and throw before returning.

        Jira text:

        Currently if JsServlet encounters an error processing a response, such as a closure compile error, it will simply respond with the error status code but won't respond with an error message. To improve this behavior, I'd like to have the servlet send a proper error response with the error status code and the error string.

        This addresses bug SHINDIG-1770.

        https://issues.apache.org/jira/browse/SHINDIG-1770

        Diffs

        -----

        http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DefaultJsServingPipeline.java 1335416

        http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/JsServletTest.java 1335416

        Diff: https://reviews.apache.org/r/5070/diff

        Testing

        -------

        Added a new test in JsServlet to ensure the error makes it through.

        Manually introduced JavaScript errors that would cause Closure to complain and verified that the error text and status code made it through when hitting the JsServlet in the browser.

        Thanks,

        Stanton

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-05-08 19:39:58, Dan Dumont wrote: > Quick question before I dig in. Do we cache the compile error response so that we don't get hammered trying to compile it over and over again? > > Did we before your change? Stanton Sievers wrote: Did some extra testing. I introduced a compile error in embedded-experiences feature and hit this url in my browser: http://localhost:8080/gadgets/js/embedded-experiences.js?c=1&container=default I set a breakpoint in JsServlet.doGet() and regardless of whether my patch was applied or not, we were doing the processing every time. The only difference before my patch was that on subsequent requests the browser would actually get a 304 Not Modified back instead of the 404. Is that because of the ETag filter or the cache control headers? Response headers before my patch: Cache-Control: public,max-age=3600 Content-Length: 0 Content-Type: text/html;charset=utf-8 Date: Wed, 09 May 2012 13:52:15 GMT Etag: "d41d8cd98f00b204e9800998ecf8427e" Expires: Wed, 09 May 2012 14:52:15 GMT Server: Apache-Coyote/1.1 Response headers after my patch: Content-Length: 1636 Content-Type: text/html;charset=utf-8 Date: Wed, 09 May 2012 13:59:59 GMT Server: Apache-Coyote/1.1 Henry Saputra wrote: Looks like the ClosureJsCompiler cache the compiled result based on key of hash value of the non-compiled JS content. Stanton Sievers wrote: Good call Henry. I never noticed the ClosureJsCompiler.cacheAndReturnErrorResult() method before, which will be called when the compiler has errors during processing. So on subsequent calls, we will go through the JS processor again, however, the compiler processor will have cached the errors and will return those instead of running the compiler again. And it does appear that it's the ETagFilter that is causing the 304 to come back in the case where my patch is not applied. Ryan Baxter wrote: So we are in fact caching the error response and not recompiling bad JS every time? Stanton Sievers wrote: Yes, but to be clear, it's being cached in the ClosureJsCompiler, not at the JsServlet level. Ryan Baxter wrote: So the difference being that the request will still go through all of the processors every time where as if it were cached at the JsServlet level it wouldn't correct? Right. My patch does not change any caching behavior. Today we would do the same thing if there were compilation problems, or if any of the processors had errors. I don't know enough about what all of the processors do to say whether this is a bad or a good thing. Some of the processors appear to actually be getting the JS content, so that processor could affect the compilation on subsequent attempts if, for instance, the JS content was different the second time around. These are problems I'm not trying to solve with this patch. Stanton ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5070/#review7695 ----------------------------------------------------------- On 2012-05-08 19:17:32, Stanton Sievers wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5070/ ----------------------------------------------------------- (Updated 2012-05-08 19:17:32) Review request for shindig. Summary ------- I had initially made the change in JsServlet itself to check for errors before calling emitJsResponse, however, DefaultJsServingPipeline.process() seemed like a better place. JsServingPipeline.process' javadoc indicates the method should throw an exception if any of the processing steps resulted in an error, so it seems reasonable to check for errors and throw before returning. Jira text: Currently if JsServlet encounters an error processing a response, such as a closure compile error, it will simply respond with the error status code but won't respond with an error message. To improve this behavior, I'd like to have the servlet send a proper error response with the error status code and the error string. This addresses bug SHINDIG-1770 . https://issues.apache.org/jira/browse/SHINDIG-1770 Diffs ----- http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DefaultJsServingPipeline.java 1335416 http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/JsServletTest.java 1335416 Diff: https://reviews.apache.org/r/5070/diff Testing ------- Added a new test in JsServlet to ensure the error makes it through. Manually introduced JavaScript errors that would cause Closure to complain and verified that the error text and status code made it through when hitting the JsServlet in the browser. Thanks, Stanton
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-05-08 19:39:58, Dan Dumont wrote:

        > Quick question before I dig in. Do we cache the compile error response so that we don't get hammered trying to compile it over and over again?

        >

        > Did we before your change?

        Stanton Sievers wrote:

        Did some extra testing. I introduced a compile error in embedded-experiences feature and hit this url in my browser: http://localhost:8080/gadgets/js/embedded-experiences.js?c=1&container=default

        I set a breakpoint in JsServlet.doGet() and regardless of whether my patch was applied or not, we were doing the processing every time. The only difference before my patch was that on subsequent requests the browser would actually get a 304 Not Modified back instead of the 404. Is that because of the ETag filter or the cache control headers?

        Response headers before my patch:

        Cache-Control: public,max-age=3600

        Content-Length: 0

        Content-Type: text/html;charset=utf-8

        Date: Wed, 09 May 2012 13:52:15 GMT

        Etag: "d41d8cd98f00b204e9800998ecf8427e"

        Expires: Wed, 09 May 2012 14:52:15 GMT

        Server: Apache-Coyote/1.1

        Response headers after my patch:

        Content-Length: 1636

        Content-Type: text/html;charset=utf-8

        Date: Wed, 09 May 2012 13:59:59 GMT

        Server: Apache-Coyote/1.1

        Henry Saputra wrote:

        Looks like the ClosureJsCompiler cache the compiled result based on key of hash value of the non-compiled JS content.

        Stanton Sievers wrote:

        Good call Henry. I never noticed the ClosureJsCompiler.cacheAndReturnErrorResult() method before, which will be called when the compiler has errors during processing. So on subsequent calls, we will go through the JS processor again, however, the compiler processor will have cached the errors and will return those instead of running the compiler again.

        And it does appear that it's the ETagFilter that is causing the 304 to come back in the case where my patch is not applied.

        Ryan Baxter wrote:

        So we are in fact caching the error response and not recompiling bad JS every time?

        Stanton Sievers wrote:

        Yes, but to be clear, it's being cached in the ClosureJsCompiler, not at the JsServlet level.

        So the difference being that the request will still go through all of the processors every time where as if it were cached at the JsServlet level it wouldn't correct?

        • Ryan

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/5070/#review7695
        -----------------------------------------------------------

        On 2012-05-08 19:17:32, Stanton Sievers wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/5070/

        -----------------------------------------------------------

        (Updated 2012-05-08 19:17:32)

        Review request for shindig.

        Summary

        -------

        I had initially made the change in JsServlet itself to check for errors before calling emitJsResponse, however, DefaultJsServingPipeline.process() seemed like a better place. JsServingPipeline.process' javadoc indicates the method should throw an exception if any of the processing steps resulted in an error, so it seems reasonable to check for errors and throw before returning.

        Jira text:

        Currently if JsServlet encounters an error processing a response, such as a closure compile error, it will simply respond with the error status code but won't respond with an error message. To improve this behavior, I'd like to have the servlet send a proper error response with the error status code and the error string.

        This addresses bug SHINDIG-1770.

        https://issues.apache.org/jira/browse/SHINDIG-1770

        Diffs

        -----

        http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DefaultJsServingPipeline.java 1335416

        http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/JsServletTest.java 1335416

        Diff: https://reviews.apache.org/r/5070/diff

        Testing

        -------

        Added a new test in JsServlet to ensure the error makes it through.

        Manually introduced JavaScript errors that would cause Closure to complain and verified that the error text and status code made it through when hitting the JsServlet in the browser.

        Thanks,

        Stanton

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-05-08 19:39:58, Dan Dumont wrote: > Quick question before I dig in. Do we cache the compile error response so that we don't get hammered trying to compile it over and over again? > > Did we before your change? Stanton Sievers wrote: Did some extra testing. I introduced a compile error in embedded-experiences feature and hit this url in my browser: http://localhost:8080/gadgets/js/embedded-experiences.js?c=1&container=default I set a breakpoint in JsServlet.doGet() and regardless of whether my patch was applied or not, we were doing the processing every time. The only difference before my patch was that on subsequent requests the browser would actually get a 304 Not Modified back instead of the 404. Is that because of the ETag filter or the cache control headers? Response headers before my patch: Cache-Control: public,max-age=3600 Content-Length: 0 Content-Type: text/html;charset=utf-8 Date: Wed, 09 May 2012 13:52:15 GMT Etag: "d41d8cd98f00b204e9800998ecf8427e" Expires: Wed, 09 May 2012 14:52:15 GMT Server: Apache-Coyote/1.1 Response headers after my patch: Content-Length: 1636 Content-Type: text/html;charset=utf-8 Date: Wed, 09 May 2012 13:59:59 GMT Server: Apache-Coyote/1.1 Henry Saputra wrote: Looks like the ClosureJsCompiler cache the compiled result based on key of hash value of the non-compiled JS content. Stanton Sievers wrote: Good call Henry. I never noticed the ClosureJsCompiler.cacheAndReturnErrorResult() method before, which will be called when the compiler has errors during processing. So on subsequent calls, we will go through the JS processor again, however, the compiler processor will have cached the errors and will return those instead of running the compiler again. And it does appear that it's the ETagFilter that is causing the 304 to come back in the case where my patch is not applied. Ryan Baxter wrote: So we are in fact caching the error response and not recompiling bad JS every time? Stanton Sievers wrote: Yes, but to be clear, it's being cached in the ClosureJsCompiler, not at the JsServlet level. So the difference being that the request will still go through all of the processors every time where as if it were cached at the JsServlet level it wouldn't correct? Ryan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5070/#review7695 ----------------------------------------------------------- On 2012-05-08 19:17:32, Stanton Sievers wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5070/ ----------------------------------------------------------- (Updated 2012-05-08 19:17:32) Review request for shindig. Summary ------- I had initially made the change in JsServlet itself to check for errors before calling emitJsResponse, however, DefaultJsServingPipeline.process() seemed like a better place. JsServingPipeline.process' javadoc indicates the method should throw an exception if any of the processing steps resulted in an error, so it seems reasonable to check for errors and throw before returning. Jira text: Currently if JsServlet encounters an error processing a response, such as a closure compile error, it will simply respond with the error status code but won't respond with an error message. To improve this behavior, I'd like to have the servlet send a proper error response with the error status code and the error string. This addresses bug SHINDIG-1770 . https://issues.apache.org/jira/browse/SHINDIG-1770 Diffs ----- http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DefaultJsServingPipeline.java 1335416 http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/JsServletTest.java 1335416 Diff: https://reviews.apache.org/r/5070/diff Testing ------- Added a new test in JsServlet to ensure the error makes it through. Manually introduced JavaScript errors that would cause Closure to complain and verified that the error text and status code made it through when hitting the JsServlet in the browser. Thanks, Stanton
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-05-08 19:39:58, Dan Dumont wrote:

        > Quick question before I dig in. Do we cache the compile error response so that we don't get hammered trying to compile it over and over again?

        >

        > Did we before your change?

        Stanton Sievers wrote:

        Did some extra testing. I introduced a compile error in embedded-experiences feature and hit this url in my browser: http://localhost:8080/gadgets/js/embedded-experiences.js?c=1&container=default

        I set a breakpoint in JsServlet.doGet() and regardless of whether my patch was applied or not, we were doing the processing every time. The only difference before my patch was that on subsequent requests the browser would actually get a 304 Not Modified back instead of the 404. Is that because of the ETag filter or the cache control headers?

        Response headers before my patch:

        Cache-Control: public,max-age=3600

        Content-Length: 0

        Content-Type: text/html;charset=utf-8

        Date: Wed, 09 May 2012 13:52:15 GMT

        Etag: "d41d8cd98f00b204e9800998ecf8427e"

        Expires: Wed, 09 May 2012 14:52:15 GMT

        Server: Apache-Coyote/1.1

        Response headers after my patch:

        Content-Length: 1636

        Content-Type: text/html;charset=utf-8

        Date: Wed, 09 May 2012 13:59:59 GMT

        Server: Apache-Coyote/1.1

        Henry Saputra wrote:

        Looks like the ClosureJsCompiler cache the compiled result based on key of hash value of the non-compiled JS content.

        Stanton Sievers wrote:

        Good call Henry. I never noticed the ClosureJsCompiler.cacheAndReturnErrorResult() method before, which will be called when the compiler has errors during processing. So on subsequent calls, we will go through the JS processor again, however, the compiler processor will have cached the errors and will return those instead of running the compiler again.

        And it does appear that it's the ETagFilter that is causing the 304 to come back in the case where my patch is not applied.

        Ryan Baxter wrote:

        So we are in fact caching the error response and not recompiling bad JS every time?

        Yes, but to be clear, it's being cached in the ClosureJsCompiler, not at the JsServlet level.

        • Stanton

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/5070/#review7695
        -----------------------------------------------------------

        On 2012-05-08 19:17:32, Stanton Sievers wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/5070/

        -----------------------------------------------------------

        (Updated 2012-05-08 19:17:32)

        Review request for shindig.

        Summary

        -------

        I had initially made the change in JsServlet itself to check for errors before calling emitJsResponse, however, DefaultJsServingPipeline.process() seemed like a better place. JsServingPipeline.process' javadoc indicates the method should throw an exception if any of the processing steps resulted in an error, so it seems reasonable to check for errors and throw before returning.

        Jira text:

        Currently if JsServlet encounters an error processing a response, such as a closure compile error, it will simply respond with the error status code but won't respond with an error message. To improve this behavior, I'd like to have the servlet send a proper error response with the error status code and the error string.

        This addresses bug SHINDIG-1770.

        https://issues.apache.org/jira/browse/SHINDIG-1770

        Diffs

        -----

        http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DefaultJsServingPipeline.java 1335416

        http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/JsServletTest.java 1335416

        Diff: https://reviews.apache.org/r/5070/diff

        Testing

        -------

        Added a new test in JsServlet to ensure the error makes it through.

        Manually introduced JavaScript errors that would cause Closure to complain and verified that the error text and status code made it through when hitting the JsServlet in the browser.

        Thanks,

        Stanton

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-05-08 19:39:58, Dan Dumont wrote: > Quick question before I dig in. Do we cache the compile error response so that we don't get hammered trying to compile it over and over again? > > Did we before your change? Stanton Sievers wrote: Did some extra testing. I introduced a compile error in embedded-experiences feature and hit this url in my browser: http://localhost:8080/gadgets/js/embedded-experiences.js?c=1&container=default I set a breakpoint in JsServlet.doGet() and regardless of whether my patch was applied or not, we were doing the processing every time. The only difference before my patch was that on subsequent requests the browser would actually get a 304 Not Modified back instead of the 404. Is that because of the ETag filter or the cache control headers? Response headers before my patch: Cache-Control: public,max-age=3600 Content-Length: 0 Content-Type: text/html;charset=utf-8 Date: Wed, 09 May 2012 13:52:15 GMT Etag: "d41d8cd98f00b204e9800998ecf8427e" Expires: Wed, 09 May 2012 14:52:15 GMT Server: Apache-Coyote/1.1 Response headers after my patch: Content-Length: 1636 Content-Type: text/html;charset=utf-8 Date: Wed, 09 May 2012 13:59:59 GMT Server: Apache-Coyote/1.1 Henry Saputra wrote: Looks like the ClosureJsCompiler cache the compiled result based on key of hash value of the non-compiled JS content. Stanton Sievers wrote: Good call Henry. I never noticed the ClosureJsCompiler.cacheAndReturnErrorResult() method before, which will be called when the compiler has errors during processing. So on subsequent calls, we will go through the JS processor again, however, the compiler processor will have cached the errors and will return those instead of running the compiler again. And it does appear that it's the ETagFilter that is causing the 304 to come back in the case where my patch is not applied. Ryan Baxter wrote: So we are in fact caching the error response and not recompiling bad JS every time? Yes, but to be clear, it's being cached in the ClosureJsCompiler, not at the JsServlet level. Stanton ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5070/#review7695 ----------------------------------------------------------- On 2012-05-08 19:17:32, Stanton Sievers wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5070/ ----------------------------------------------------------- (Updated 2012-05-08 19:17:32) Review request for shindig. Summary ------- I had initially made the change in JsServlet itself to check for errors before calling emitJsResponse, however, DefaultJsServingPipeline.process() seemed like a better place. JsServingPipeline.process' javadoc indicates the method should throw an exception if any of the processing steps resulted in an error, so it seems reasonable to check for errors and throw before returning. Jira text: Currently if JsServlet encounters an error processing a response, such as a closure compile error, it will simply respond with the error status code but won't respond with an error message. To improve this behavior, I'd like to have the servlet send a proper error response with the error status code and the error string. This addresses bug SHINDIG-1770 . https://issues.apache.org/jira/browse/SHINDIG-1770 Diffs ----- http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DefaultJsServingPipeline.java 1335416 http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/JsServletTest.java 1335416 Diff: https://reviews.apache.org/r/5070/diff Testing ------- Added a new test in JsServlet to ensure the error makes it through. Manually introduced JavaScript errors that would cause Closure to complain and verified that the error text and status code made it through when hitting the JsServlet in the browser. Thanks, Stanton
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-05-08 19:39:58, Dan Dumont wrote:

        > Quick question before I dig in. Do we cache the compile error response so that we don't get hammered trying to compile it over and over again?

        >

        > Did we before your change?

        Stanton Sievers wrote:

        Did some extra testing. I introduced a compile error in embedded-experiences feature and hit this url in my browser: http://localhost:8080/gadgets/js/embedded-experiences.js?c=1&container=default

        I set a breakpoint in JsServlet.doGet() and regardless of whether my patch was applied or not, we were doing the processing every time. The only difference before my patch was that on subsequent requests the browser would actually get a 304 Not Modified back instead of the 404. Is that because of the ETag filter or the cache control headers?

        Response headers before my patch:

        Cache-Control: public,max-age=3600

        Content-Length: 0

        Content-Type: text/html;charset=utf-8

        Date: Wed, 09 May 2012 13:52:15 GMT

        Etag: "d41d8cd98f00b204e9800998ecf8427e"

        Expires: Wed, 09 May 2012 14:52:15 GMT

        Server: Apache-Coyote/1.1

        Response headers after my patch:

        Content-Length: 1636

        Content-Type: text/html;charset=utf-8

        Date: Wed, 09 May 2012 13:59:59 GMT

        Server: Apache-Coyote/1.1

        Henry Saputra wrote:

        Looks like the ClosureJsCompiler cache the compiled result based on key of hash value of the non-compiled JS content.

        Stanton Sievers wrote:

        Good call Henry. I never noticed the ClosureJsCompiler.cacheAndReturnErrorResult() method before, which will be called when the compiler has errors during processing. So on subsequent calls, we will go through the JS processor again, however, the compiler processor will have cached the errors and will return those instead of running the compiler again.

        And it does appear that it's the ETagFilter that is causing the 304 to come back in the case where my patch is not applied.

        So we are in fact caching the error response and not recompiling bad JS every time?

        • Ryan

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/5070/#review7695
        -----------------------------------------------------------

        On 2012-05-08 19:17:32, Stanton Sievers wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/5070/

        -----------------------------------------------------------

        (Updated 2012-05-08 19:17:32)

        Review request for shindig.

        Summary

        -------

        I had initially made the change in JsServlet itself to check for errors before calling emitJsResponse, however, DefaultJsServingPipeline.process() seemed like a better place. JsServingPipeline.process' javadoc indicates the method should throw an exception if any of the processing steps resulted in an error, so it seems reasonable to check for errors and throw before returning.

        Jira text:

        Currently if JsServlet encounters an error processing a response, such as a closure compile error, it will simply respond with the error status code but won't respond with an error message. To improve this behavior, I'd like to have the servlet send a proper error response with the error status code and the error string.

        This addresses bug SHINDIG-1770.

        https://issues.apache.org/jira/browse/SHINDIG-1770

        Diffs

        -----

        http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DefaultJsServingPipeline.java 1335416

        http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/JsServletTest.java 1335416

        Diff: https://reviews.apache.org/r/5070/diff

        Testing

        -------

        Added a new test in JsServlet to ensure the error makes it through.

        Manually introduced JavaScript errors that would cause Closure to complain and verified that the error text and status code made it through when hitting the JsServlet in the browser.

        Thanks,

        Stanton

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-05-08 19:39:58, Dan Dumont wrote: > Quick question before I dig in. Do we cache the compile error response so that we don't get hammered trying to compile it over and over again? > > Did we before your change? Stanton Sievers wrote: Did some extra testing. I introduced a compile error in embedded-experiences feature and hit this url in my browser: http://localhost:8080/gadgets/js/embedded-experiences.js?c=1&container=default I set a breakpoint in JsServlet.doGet() and regardless of whether my patch was applied or not, we were doing the processing every time. The only difference before my patch was that on subsequent requests the browser would actually get a 304 Not Modified back instead of the 404. Is that because of the ETag filter or the cache control headers? Response headers before my patch: Cache-Control: public,max-age=3600 Content-Length: 0 Content-Type: text/html;charset=utf-8 Date: Wed, 09 May 2012 13:52:15 GMT Etag: "d41d8cd98f00b204e9800998ecf8427e" Expires: Wed, 09 May 2012 14:52:15 GMT Server: Apache-Coyote/1.1 Response headers after my patch: Content-Length: 1636 Content-Type: text/html;charset=utf-8 Date: Wed, 09 May 2012 13:59:59 GMT Server: Apache-Coyote/1.1 Henry Saputra wrote: Looks like the ClosureJsCompiler cache the compiled result based on key of hash value of the non-compiled JS content. Stanton Sievers wrote: Good call Henry. I never noticed the ClosureJsCompiler.cacheAndReturnErrorResult() method before, which will be called when the compiler has errors during processing. So on subsequent calls, we will go through the JS processor again, however, the compiler processor will have cached the errors and will return those instead of running the compiler again. And it does appear that it's the ETagFilter that is causing the 304 to come back in the case where my patch is not applied. So we are in fact caching the error response and not recompiling bad JS every time? Ryan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5070/#review7695 ----------------------------------------------------------- On 2012-05-08 19:17:32, Stanton Sievers wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5070/ ----------------------------------------------------------- (Updated 2012-05-08 19:17:32) Review request for shindig. Summary ------- I had initially made the change in JsServlet itself to check for errors before calling emitJsResponse, however, DefaultJsServingPipeline.process() seemed like a better place. JsServingPipeline.process' javadoc indicates the method should throw an exception if any of the processing steps resulted in an error, so it seems reasonable to check for errors and throw before returning. Jira text: Currently if JsServlet encounters an error processing a response, such as a closure compile error, it will simply respond with the error status code but won't respond with an error message. To improve this behavior, I'd like to have the servlet send a proper error response with the error status code and the error string. This addresses bug SHINDIG-1770 . https://issues.apache.org/jira/browse/SHINDIG-1770 Diffs ----- http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DefaultJsServingPipeline.java 1335416 http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/JsServletTest.java 1335416 Diff: https://reviews.apache.org/r/5070/diff Testing ------- Added a new test in JsServlet to ensure the error makes it through. Manually introduced JavaScript errors that would cause Closure to complain and verified that the error text and status code made it through when hitting the JsServlet in the browser. Thanks, Stanton
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-05-08 19:39:58, Dan Dumont wrote:

        > Quick question before I dig in. Do we cache the compile error response so that we don't get hammered trying to compile it over and over again?

        >

        > Did we before your change?

        Stanton Sievers wrote:

        Did some extra testing. I introduced a compile error in embedded-experiences feature and hit this url in my browser: http://localhost:8080/gadgets/js/embedded-experiences.js?c=1&container=default

        I set a breakpoint in JsServlet.doGet() and regardless of whether my patch was applied or not, we were doing the processing every time. The only difference before my patch was that on subsequent requests the browser would actually get a 304 Not Modified back instead of the 404. Is that because of the ETag filter or the cache control headers?

        Response headers before my patch:

        Cache-Control: public,max-age=3600

        Content-Length: 0

        Content-Type: text/html;charset=utf-8

        Date: Wed, 09 May 2012 13:52:15 GMT

        Etag: "d41d8cd98f00b204e9800998ecf8427e"

        Expires: Wed, 09 May 2012 14:52:15 GMT

        Server: Apache-Coyote/1.1

        Response headers after my patch:

        Content-Length: 1636

        Content-Type: text/html;charset=utf-8

        Date: Wed, 09 May 2012 13:59:59 GMT

        Server: Apache-Coyote/1.1

        Henry Saputra wrote:

        Looks like the ClosureJsCompiler cache the compiled result based on key of hash value of the non-compiled JS content.

        Good call Henry. I never noticed the ClosureJsCompiler.cacheAndReturnErrorResult() method before, which will be called when the compiler has errors during processing. So on subsequent calls, we will go through the JS processor again, however, the compiler processor will have cached the errors and will return those instead of running the compiler again.

        And it does appear that it's the ETagFilter that is causing the 304 to come back in the case where my patch is not applied.

        • Stanton

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/5070/#review7695
        -----------------------------------------------------------

        On 2012-05-08 19:17:32, Stanton Sievers wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/5070/

        -----------------------------------------------------------

        (Updated 2012-05-08 19:17:32)

        Review request for shindig.

        Summary

        -------

        I had initially made the change in JsServlet itself to check for errors before calling emitJsResponse, however, DefaultJsServingPipeline.process() seemed like a better place. JsServingPipeline.process' javadoc indicates the method should throw an exception if any of the processing steps resulted in an error, so it seems reasonable to check for errors and throw before returning.

        Jira text:

        Currently if JsServlet encounters an error processing a response, such as a closure compile error, it will simply respond with the error status code but won't respond with an error message. To improve this behavior, I'd like to have the servlet send a proper error response with the error status code and the error string.

        This addresses bug SHINDIG-1770.

        https://issues.apache.org/jira/browse/SHINDIG-1770

        Diffs

        -----

        http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DefaultJsServingPipeline.java 1335416

        http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/JsServletTest.java 1335416

        Diff: https://reviews.apache.org/r/5070/diff

        Testing

        -------

        Added a new test in JsServlet to ensure the error makes it through.

        Manually introduced JavaScript errors that would cause Closure to complain and verified that the error text and status code made it through when hitting the JsServlet in the browser.

        Thanks,

        Stanton

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-05-08 19:39:58, Dan Dumont wrote: > Quick question before I dig in. Do we cache the compile error response so that we don't get hammered trying to compile it over and over again? > > Did we before your change? Stanton Sievers wrote: Did some extra testing. I introduced a compile error in embedded-experiences feature and hit this url in my browser: http://localhost:8080/gadgets/js/embedded-experiences.js?c=1&container=default I set a breakpoint in JsServlet.doGet() and regardless of whether my patch was applied or not, we were doing the processing every time. The only difference before my patch was that on subsequent requests the browser would actually get a 304 Not Modified back instead of the 404. Is that because of the ETag filter or the cache control headers? Response headers before my patch: Cache-Control: public,max-age=3600 Content-Length: 0 Content-Type: text/html;charset=utf-8 Date: Wed, 09 May 2012 13:52:15 GMT Etag: "d41d8cd98f00b204e9800998ecf8427e" Expires: Wed, 09 May 2012 14:52:15 GMT Server: Apache-Coyote/1.1 Response headers after my patch: Content-Length: 1636 Content-Type: text/html;charset=utf-8 Date: Wed, 09 May 2012 13:59:59 GMT Server: Apache-Coyote/1.1 Henry Saputra wrote: Looks like the ClosureJsCompiler cache the compiled result based on key of hash value of the non-compiled JS content. Good call Henry. I never noticed the ClosureJsCompiler.cacheAndReturnErrorResult() method before, which will be called when the compiler has errors during processing. So on subsequent calls, we will go through the JS processor again, however, the compiler processor will have cached the errors and will return those instead of running the compiler again. And it does appear that it's the ETagFilter that is causing the 304 to come back in the case where my patch is not applied. Stanton ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5070/#review7695 ----------------------------------------------------------- On 2012-05-08 19:17:32, Stanton Sievers wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5070/ ----------------------------------------------------------- (Updated 2012-05-08 19:17:32) Review request for shindig. Summary ------- I had initially made the change in JsServlet itself to check for errors before calling emitJsResponse, however, DefaultJsServingPipeline.process() seemed like a better place. JsServingPipeline.process' javadoc indicates the method should throw an exception if any of the processing steps resulted in an error, so it seems reasonable to check for errors and throw before returning. Jira text: Currently if JsServlet encounters an error processing a response, such as a closure compile error, it will simply respond with the error status code but won't respond with an error message. To improve this behavior, I'd like to have the servlet send a proper error response with the error status code and the error string. This addresses bug SHINDIG-1770 . https://issues.apache.org/jira/browse/SHINDIG-1770 Diffs ----- http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DefaultJsServingPipeline.java 1335416 http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/JsServletTest.java 1335416 Diff: https://reviews.apache.org/r/5070/diff Testing ------- Added a new test in JsServlet to ensure the error makes it through. Manually introduced JavaScript errors that would cause Closure to complain and verified that the error text and status code made it through when hitting the JsServlet in the browser. Thanks, Stanton
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-05-08 19:39:58, Dan Dumont wrote:

        > Quick question before I dig in. Do we cache the compile error response so that we don't get hammered trying to compile it over and over again?

        >

        > Did we before your change?

        Stanton Sievers wrote:

        Did some extra testing. I introduced a compile error in embedded-experiences feature and hit this url in my browser: http://localhost:8080/gadgets/js/embedded-experiences.js?c=1&container=default

        I set a breakpoint in JsServlet.doGet() and regardless of whether my patch was applied or not, we were doing the processing every time. The only difference before my patch was that on subsequent requests the browser would actually get a 304 Not Modified back instead of the 404. Is that because of the ETag filter or the cache control headers?

        Response headers before my patch:

        Cache-Control: public,max-age=3600

        Content-Length: 0

        Content-Type: text/html;charset=utf-8

        Date: Wed, 09 May 2012 13:52:15 GMT

        Etag: "d41d8cd98f00b204e9800998ecf8427e"

        Expires: Wed, 09 May 2012 14:52:15 GMT

        Server: Apache-Coyote/1.1

        Response headers after my patch:

        Content-Length: 1636

        Content-Type: text/html;charset=utf-8

        Date: Wed, 09 May 2012 13:59:59 GMT

        Server: Apache-Coyote/1.1

        Looks like the ClosureJsCompiler cache the compiled result based on key of hash value of the non-compiled JS content.

        • Henry

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/5070/#review7695
        -----------------------------------------------------------

        On 2012-05-08 19:17:32, Stanton Sievers wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/5070/

        -----------------------------------------------------------

        (Updated 2012-05-08 19:17:32)

        Review request for shindig.

        Summary

        -------

        I had initially made the change in JsServlet itself to check for errors before calling emitJsResponse, however, DefaultJsServingPipeline.process() seemed like a better place. JsServingPipeline.process' javadoc indicates the method should throw an exception if any of the processing steps resulted in an error, so it seems reasonable to check for errors and throw before returning.

        Jira text:

        Currently if JsServlet encounters an error processing a response, such as a closure compile error, it will simply respond with the error status code but won't respond with an error message. To improve this behavior, I'd like to have the servlet send a proper error response with the error status code and the error string.

        This addresses bug SHINDIG-1770.

        https://issues.apache.org/jira/browse/SHINDIG-1770

        Diffs

        -----

        http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DefaultJsServingPipeline.java 1335416

        http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/JsServletTest.java 1335416

        Diff: https://reviews.apache.org/r/5070/diff

        Testing

        -------

        Added a new test in JsServlet to ensure the error makes it through.

        Manually introduced JavaScript errors that would cause Closure to complain and verified that the error text and status code made it through when hitting the JsServlet in the browser.

        Thanks,

        Stanton

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-05-08 19:39:58, Dan Dumont wrote: > Quick question before I dig in. Do we cache the compile error response so that we don't get hammered trying to compile it over and over again? > > Did we before your change? Stanton Sievers wrote: Did some extra testing. I introduced a compile error in embedded-experiences feature and hit this url in my browser: http://localhost:8080/gadgets/js/embedded-experiences.js?c=1&container=default I set a breakpoint in JsServlet.doGet() and regardless of whether my patch was applied or not, we were doing the processing every time. The only difference before my patch was that on subsequent requests the browser would actually get a 304 Not Modified back instead of the 404. Is that because of the ETag filter or the cache control headers? Response headers before my patch: Cache-Control: public,max-age=3600 Content-Length: 0 Content-Type: text/html;charset=utf-8 Date: Wed, 09 May 2012 13:52:15 GMT Etag: "d41d8cd98f00b204e9800998ecf8427e" Expires: Wed, 09 May 2012 14:52:15 GMT Server: Apache-Coyote/1.1 Response headers after my patch: Content-Length: 1636 Content-Type: text/html;charset=utf-8 Date: Wed, 09 May 2012 13:59:59 GMT Server: Apache-Coyote/1.1 Looks like the ClosureJsCompiler cache the compiled result based on key of hash value of the non-compiled JS content. Henry ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5070/#review7695 ----------------------------------------------------------- On 2012-05-08 19:17:32, Stanton Sievers wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5070/ ----------------------------------------------------------- (Updated 2012-05-08 19:17:32) Review request for shindig. Summary ------- I had initially made the change in JsServlet itself to check for errors before calling emitJsResponse, however, DefaultJsServingPipeline.process() seemed like a better place. JsServingPipeline.process' javadoc indicates the method should throw an exception if any of the processing steps resulted in an error, so it seems reasonable to check for errors and throw before returning. Jira text: Currently if JsServlet encounters an error processing a response, such as a closure compile error, it will simply respond with the error status code but won't respond with an error message. To improve this behavior, I'd like to have the servlet send a proper error response with the error status code and the error string. This addresses bug SHINDIG-1770 . https://issues.apache.org/jira/browse/SHINDIG-1770 Diffs ----- http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DefaultJsServingPipeline.java 1335416 http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/JsServletTest.java 1335416 Diff: https://reviews.apache.org/r/5070/diff Testing ------- Added a new test in JsServlet to ensure the error makes it through. Manually introduced JavaScript errors that would cause Closure to complain and verified that the error text and status code made it through when hitting the JsServlet in the browser. Thanks, Stanton
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-05-08 19:39:58, Dan Dumont wrote:

        > Quick question before I dig in. Do we cache the compile error response so that we don't get hammered trying to compile it over and over again?

        >

        > Did we before your change?

        Did some extra testing. I introduced a compile error in embedded-experiences feature and hit this url in my browser: http://localhost:8080/gadgets/js/embedded-experiences.js?c=1&container=default

        I set a breakpoint in JsServlet.doGet() and regardless of whether my patch was applied or not, we were doing the processing every time. The only difference before my patch was that on subsequent requests the browser would actually get a 304 Not Modified back instead of the 404. Is that because of the ETag filter or the cache control headers?

        Response headers before my patch:
        Cache-Control: public,max-age=3600
        Content-Length: 0
        Content-Type: text/html;charset=utf-8
        Date: Wed, 09 May 2012 13:52:15 GMT
        Etag: "d41d8cd98f00b204e9800998ecf8427e"
        Expires: Wed, 09 May 2012 14:52:15 GMT
        Server: Apache-Coyote/1.1

        Response headers after my patch:
        Content-Length: 1636
        Content-Type: text/html;charset=utf-8
        Date: Wed, 09 May 2012 13:59:59 GMT
        Server: Apache-Coyote/1.1

        • Stanton

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/5070/#review7695
        -----------------------------------------------------------

        On 2012-05-08 19:17:32, Stanton Sievers wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/5070/

        -----------------------------------------------------------

        (Updated 2012-05-08 19:17:32)

        Review request for shindig.

        Summary

        -------

        I had initially made the change in JsServlet itself to check for errors before calling emitJsResponse, however, DefaultJsServingPipeline.process() seemed like a better place. JsServingPipeline.process' javadoc indicates the method should throw an exception if any of the processing steps resulted in an error, so it seems reasonable to check for errors and throw before returning.

        Jira text:

        Currently if JsServlet encounters an error processing a response, such as a closure compile error, it will simply respond with the error status code but won't respond with an error message. To improve this behavior, I'd like to have the servlet send a proper error response with the error status code and the error string.

        This addresses bug SHINDIG-1770.

        https://issues.apache.org/jira/browse/SHINDIG-1770

        Diffs

        -----

        http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DefaultJsServingPipeline.java 1335416

        http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/JsServletTest.java 1335416

        Diff: https://reviews.apache.org/r/5070/diff

        Testing

        -------

        Added a new test in JsServlet to ensure the error makes it through.

        Manually introduced JavaScript errors that would cause Closure to complain and verified that the error text and status code made it through when hitting the JsServlet in the browser.

        Thanks,

        Stanton

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-05-08 19:39:58, Dan Dumont wrote: > Quick question before I dig in. Do we cache the compile error response so that we don't get hammered trying to compile it over and over again? > > Did we before your change? Did some extra testing. I introduced a compile error in embedded-experiences feature and hit this url in my browser: http://localhost:8080/gadgets/js/embedded-experiences.js?c=1&container=default I set a breakpoint in JsServlet.doGet() and regardless of whether my patch was applied or not, we were doing the processing every time. The only difference before my patch was that on subsequent requests the browser would actually get a 304 Not Modified back instead of the 404. Is that because of the ETag filter or the cache control headers? Response headers before my patch: Cache-Control: public,max-age=3600 Content-Length: 0 Content-Type: text/html;charset=utf-8 Date: Wed, 09 May 2012 13:52:15 GMT Etag: "d41d8cd98f00b204e9800998ecf8427e" Expires: Wed, 09 May 2012 14:52:15 GMT Server: Apache-Coyote/1.1 Response headers after my patch: Content-Length: 1636 Content-Type: text/html;charset=utf-8 Date: Wed, 09 May 2012 13:59:59 GMT Server: Apache-Coyote/1.1 Stanton ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5070/#review7695 ----------------------------------------------------------- On 2012-05-08 19:17:32, Stanton Sievers wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5070/ ----------------------------------------------------------- (Updated 2012-05-08 19:17:32) Review request for shindig. Summary ------- I had initially made the change in JsServlet itself to check for errors before calling emitJsResponse, however, DefaultJsServingPipeline.process() seemed like a better place. JsServingPipeline.process' javadoc indicates the method should throw an exception if any of the processing steps resulted in an error, so it seems reasonable to check for errors and throw before returning. Jira text: Currently if JsServlet encounters an error processing a response, such as a closure compile error, it will simply respond with the error status code but won't respond with an error message. To improve this behavior, I'd like to have the servlet send a proper error response with the error status code and the error string. This addresses bug SHINDIG-1770 . https://issues.apache.org/jira/browse/SHINDIG-1770 Diffs ----- http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DefaultJsServingPipeline.java 1335416 http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/JsServletTest.java 1335416 Diff: https://reviews.apache.org/r/5070/diff Testing ------- Added a new test in JsServlet to ensure the error makes it through. Manually introduced JavaScript errors that would cause Closure to complain and verified that the error text and status code made it through when hitting the JsServlet in the browser. Thanks, Stanton
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/5070/#review7695
        -----------------------------------------------------------

        Quick question before I dig in. Do we cache the compile error response so that we don't get hammered trying to compile it over and over again?

        Did we before your change?

        • Dan

        On 2012-05-08 19:17:32, Stanton Sievers wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/5070/

        -----------------------------------------------------------

        (Updated 2012-05-08 19:17:32)

        Review request for shindig.

        Summary

        -------

        I had initially made the change in JsServlet itself to check for errors before calling emitJsResponse, however, DefaultJsServingPipeline.process() seemed like a better place. JsServingPipeline.process' javadoc indicates the method should throw an exception if any of the processing steps resulted in an error, so it seems reasonable to check for errors and throw before returning.

        Jira text:

        Currently if JsServlet encounters an error processing a response, such as a closure compile error, it will simply respond with the error status code but won't respond with an error message. To improve this behavior, I'd like to have the servlet send a proper error response with the error status code and the error string.

        This addresses bug SHINDIG-1770.

        https://issues.apache.org/jira/browse/SHINDIG-1770

        Diffs

        -----

        http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DefaultJsServingPipeline.java 1335416

        http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/JsServletTest.java 1335416

        Diff: https://reviews.apache.org/r/5070/diff

        Testing

        -------

        Added a new test in JsServlet to ensure the error makes it through.

        Manually introduced JavaScript errors that would cause Closure to complain and verified that the error text and status code made it through when hitting the JsServlet in the browser.

        Thanks,

        Stanton

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5070/#review7695 ----------------------------------------------------------- Quick question before I dig in. Do we cache the compile error response so that we don't get hammered trying to compile it over and over again? Did we before your change? Dan On 2012-05-08 19:17:32, Stanton Sievers wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5070/ ----------------------------------------------------------- (Updated 2012-05-08 19:17:32) Review request for shindig. Summary ------- I had initially made the change in JsServlet itself to check for errors before calling emitJsResponse, however, DefaultJsServingPipeline.process() seemed like a better place. JsServingPipeline.process' javadoc indicates the method should throw an exception if any of the processing steps resulted in an error, so it seems reasonable to check for errors and throw before returning. Jira text: Currently if JsServlet encounters an error processing a response, such as a closure compile error, it will simply respond with the error status code but won't respond with an error message. To improve this behavior, I'd like to have the servlet send a proper error response with the error status code and the error string. This addresses bug SHINDIG-1770 . https://issues.apache.org/jira/browse/SHINDIG-1770 Diffs ----- http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DefaultJsServingPipeline.java 1335416 http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/JsServletTest.java 1335416 Diff: https://reviews.apache.org/r/5070/diff Testing ------- Added a new test in JsServlet to ensure the error makes it through. Manually introduced JavaScript errors that would cause Closure to complain and verified that the error text and status code made it through when hitting the JsServlet in the browser. Thanks, Stanton
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/5070/
        -----------------------------------------------------------

        Review request for shindig.

        Summary
        -------

        I had initially made the change in JsServlet itself to check for errors before calling emitJsResponse, however, DefaultJsServingPipeline.process() seemed like a better place. JsServingPipeline.process' javadoc indicates the method should throw an exception if any of the processing steps resulted in an error, so it seems reasonable to check for errors and throw before returning.

        Jira text:
        Currently if JsServlet encounters an error processing a response, such as a closure compile error, it will simply respond with the error status code but won't respond with an error message. To improve this behavior, I'd like to have the servlet send a proper error response with the error status code and the error string.

        This addresses bug SHINDIG-1770.
        https://issues.apache.org/jira/browse/SHINDIG-1770

        Diffs


        http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DefaultJsServingPipeline.java 1335416
        http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/JsServletTest.java 1335416

        Diff: https://reviews.apache.org/r/5070/diff

        Testing
        -------

        Added a new test in JsServlet to ensure the error makes it through.

        Manually introduced JavaScript errors that would cause Closure to complain and verified that the error text and status code made it through when hitting the JsServlet in the browser.

        Thanks,

        Stanton

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5070/ ----------------------------------------------------------- Review request for shindig. Summary ------- I had initially made the change in JsServlet itself to check for errors before calling emitJsResponse, however, DefaultJsServingPipeline.process() seemed like a better place. JsServingPipeline.process' javadoc indicates the method should throw an exception if any of the processing steps resulted in an error, so it seems reasonable to check for errors and throw before returning. Jira text: Currently if JsServlet encounters an error processing a response, such as a closure compile error, it will simply respond with the error status code but won't respond with an error message. To improve this behavior, I'd like to have the servlet send a proper error response with the error status code and the error string. This addresses bug SHINDIG-1770 . https://issues.apache.org/jira/browse/SHINDIG-1770 Diffs http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DefaultJsServingPipeline.java 1335416 http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/JsServletTest.java 1335416 Diff: https://reviews.apache.org/r/5070/diff Testing ------- Added a new test in JsServlet to ensure the error makes it through. Manually introduced JavaScript errors that would cause Closure to complain and verified that the error text and status code made it through when hitting the JsServlet in the browser. Thanks, Stanton

          People

          • Assignee:
            Stanton Sievers
            Reporter:
            Stanton Sievers
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development