[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:
duplicate
duplicates MAGNOLIA-3566 Cache filter should ignore ClientAbor... Closed
relation
is related to MAGNOLIA-5113 Generalize/centralize treatment of br... Closed
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 fix

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

  • info.magnolia.rendering.engine.RenderingFilter#doFilter
  • info.magnolia.rendering.engine.RenderingFilter#handleResourceRequest
  • info.magnolia.cms.servlets.ClasspathSpool#streamSingleFile

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).
After discussions, we decided to do this in UseCache instead, thus avoiding accidentally swallowing IOExceptions coming from further down the chain.

Proposed improvements

Centralize and avoid code duplication; would allow for smarter/more complete handling of this. See MAGNOLIA-5113.

Symptoms

bundle (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:

This exception is caused by writing to a socket that's been closed by
the remote end.
Your client decided to abruptly close the socket.
If this is a real log (as opposed to a test log), the client could
have closed the browser, put the computer in sleep, etc.

Just ignore it.

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

Comment by Magnolia International [ 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.

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

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 MAGNOLIA-5113. However, if MAGNOLIA-5113 does a much more selective job at treating those cases than we currently do in the localized patches, it might be ok. Thoughts ?

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!

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.

Generated at Mon Feb 12 04:02:06 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.