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