[MAGNOLIA-5112] Responses served from cache can throw a RuntimeException that should be ignored (Occasional broken pipe for DINWebPro.woff) Created: 13/Jun/13 Updated: 14/Jun/13 Resolved: 14/Jun/13 |
|
| Status: | Closed |
| Project: | Magnolia |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | 4.5.10, 5.0 |
| Type: | Bug | Priority: | Major |
| Reporter: | Daniel Lipp | Assignee: | Magnolia International |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||||||
| Template: |
|
||||||||||||||||
| Patch included: |
Yes
|
||||||||||||||||
| Acceptance criteria: |
Empty
|
||||||||||||||||
| Task DoD: |
[ ]*
Doc/release notes changes? Comment present?
[ ]*
Downstream builds green?
[ ]*
Solution information and context easily available?
[ ]*
Tests
[ ]*
FixVersion filled and not yet released
[ ] 
Architecture Decision Record (ADR)
|
||||||||||||||||
| Bug DoR: |
[ ]*
Steps to reproduce, expected, and actual results filled
[ ]*
Affected version filled
|
||||||||||||||||
| Date of First Response: | |||||||||||||||||
| Sprint: | RC 1, RC 2 | ||||||||||||||||
| Description |
Cause and proposed fixWhen a request is interrupted (by the browser), containers typically throw a subclass of IOException (Tomcat throws a org.apache.catalina.connector.ClientAbortException, Jetty throws an org.mortbay.jetty.EofException, ...). We already treat this in a few places: (look for catch (IOException) snippet)
We could do the exact same thing in CacheFilter. We already catch all exceptions; checking if it's an IO and swallowing it there isn't perfect or ideal, but at least it'll be consistent with the rest. (In this specific case we need to make we still unlock the cache if needed). Proposed improvementsCentralize and avoid code duplication; would allow for smarter/more complete handling of this. See Symptomsbundle (integration tests) occasionally fail because of the following error:
[INFO] [talledLocalContainer] 2013-06-13 06:31:55,647 ERROR info.magnolia.module.cache.filter.CacheFilter : A request started to cache but failed with an exception (IOException: Broken pipe). [url=http://localhost:8088/magnoliaTestPublic/.resources/defaultLoginForm/css/fonts/DINWebPro.woff], [key=DefaultCacheKey{uri='/.resources/defaultLoginForm/css/fonts/DINWebPro.woff', serverName='localhost', locale='en', channel='desktop', params={}', secure='false'}]
[WARNING] [talledLocalContainer] 2013-06-13 06:31:55.676:WARN::/magnoliaTestPublic/.resources/defaultLoginForm/css/fonts/DINWebPro.woff
[WARNING] [talledLocalContainer] java.lang.RuntimeException: org.mortbay.jetty.EofException
This sometimes also happens locally now... |
| Comments |
| Comment by Mikaël Geljić [ 13/Jun/13 ] |
|
Quick explanation found on a related topic:
https://groups.google.com/forum/?fromgroups#!topic/cometd-users/VFsaayHjRFo |
| Comment by Jan Haderka [ 14/Jun/13 ] |
|
IMHO committed "solution" is incorrect. This should be treated at the source where we know what happens, not when-all-else-fails in the cache. The real issue here is that even tho writing to the output stream where IOE occurs happens inside of Renderer.render() method, signature of this method allows only RenderException to be thrown thus forcing all renderers wrap IOE in RE and effectively bypassing the check in RenderingFilter. The check in the filter will be evaluated only if what is written to the output stream is small enough to fit in the buffer and IOE occurs when filter is flushing the buffer. To solve this properly we should allow IOE to be thrown from Renderer.render() method and not wrap IOE in RE in any of the renderers. The renderer that is used in case of the fonts is the one provided by the m-m-resources module. |
| Comment by Jan Haderka [ 14/Jun/13 ] |
|
Also, please note that your assumption that the exception that reaches CacheFilter is IOE is incorrect. The log message is constructed using ExceptionUtils to recursively go back through exception chain and logs the very root exception, not the top level one that was caught! And as per explanation above, if block added in commit in this issue will not be evaluated true when you expect it, since exception caught there is not IOE. |
| Comment by Jan Haderka [ 14/Jun/13 ] |
|
Not that I wanna be whining about it, but please when committing something on .x branch, make sure you change fix version from .x to the next minor release on that branch to make sure that commit doesn't go into the release unnoticed. |
| Comment by Magnolia International [ 14/Jun/13 ] |
Ok, after discussion, I now agree with this. Didn't consider that the catch block that was already in the CacheFilter was already a catch-all for anything that happens below the cache.
The issue we were looking at has nothing to do with renderers. The font file was served by the ClasspathSpool servlet (serving stuff from mgnl-resources under /.resources); that servlet already treats interrupted requests. The issue occurred once the file was in the cache; UseCache does not treat this case. Considering the above, I agree to move this treatment up into UseCache or its parent class, to avoid accidentally hiding other exceptions. Unfortunately, this change kind-of cancels the improvement proposed in
Not an assumption; it's your assumption that we were talking about renderers that was incorrect |
| Comment by Magnolia International [ 14/Jun/13 ] |
|
Moved fix from CacheFilter to UseCache. |