> 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 > >
