wrong handling of exceptions for Resource#respond and ComponentResourceRequestTarget#respond

• Key: WICKET-144
• Type: Bug
• Status: Resolved
• Resolution: Fixed
• Priority: Major
• Assignee: Eelco Hillenius
• Reporter: Eelco Hillenius
• Component/s: wicket
• Affects Version/s: 1.2.4, 1.3.0-beta1, 2.0 branch (discontinued)
• Fix Version/s: 1.2.4, 1.3.0-beta1, 2.0 branch (discontinued)

Description

Resource's exception handling is too fragile. Currently (before fix) it looks like this:

private final void respond(final IResourceStream resourceStream, final Response response)
{
try
{
final OutputStream out = response.getOutputStream();
try
{
Streams.copy(resourceStream.getInputStream(), out);
}
finally
{
resourceStream.close();
out.flush();
}
}
catch (Exception e)
{
Throwable throwable = e;
boolean ignoreException = false;
while (throwable != null)
{
if (throwable instanceof SocketException)
{
String message = throwable.getMessage();
ignoreException = message != null
&& (message.indexOf("Connection reset by peer") != -1 || message
.indexOf("Software caused connection abort") != -1);
}
else
{
ignoreException = throwable.getClass().getName()
.indexOf("ClientAbortException") >= 0;
if (ignoreException)
{
if (log.isDebugEnabled())
{
log.debug("Socket exception ignored for sending Resource "
+ "response to client (ClientAbort)", e);
}
break;
}
}
throwable = throwable.getCause();
}
if (!ignoreException)
{
throw new WicketRuntimeException("Unable to render resource stream "
+ resourceStream, e);
}
}
}

and ComponentResourceRequestTarget re-threw this exception, resulting in Wicket trying to render an error page. One problem with the above code is that it is simply does not cover all appservers/versions/os-ses, nor it that something we should want to support. For instance, an aborted request in Jetty on Linux/ OSX results in an IOException (or more concrete EofException).

Besides the fact that this can get you weird behavior in general, handling the exception like we did there is never a good idea. We should never try to render an error page for any exception during a resource request but instead set an appropriate status code. I propose status code 500 (INTERNAL_SERVER_ERROR) for this and put that in ComponentResourceRequestTarget, and alter Resource so that it always re-throws any exception.

Comments

[Eelco Hillenius]

see http://www.nabble.com/Re%3A-svn-commit%3A-r482863---in--incubator-wicket%3A-branches-wicket-1.2.x-wicket-src-main-java-wicket--branches-wicket-1.2.x-wicket-src-main-java-wicket-request-target-resource--branches-wicket-1.x-wicket-src-main-java-wicket--branches-wicket-1.x-wicket--tf2770886.html

[Eelco Hillenius]

We can put that checking code in back again, and extend it with the cases for unix variants. What we should not put in is re-throwing of exceptions that aren't caught in that algorithm. They were ultimately caught by Wicket and Wicket tried to render an error page for it. Which doesn't make sense/ work for resource requests in the first place.

[Johan Compagner]

that change in ComponentResourceRequestTarget is fine
But code that tries to not throw an exception at all should be back in.
And improved for other situations/setups.

It is pretty easy testing if you can test it in a debugger.
Just set a break point in the respond method.
And hold the break there. Then press the stop loading button of the browser.
Then when you write again. Some kind of abort/reset socket exception should be thrown.

[Eelco Hillenius]

Rolled back the change to Resource and included a FIXME that notes we don't cover all cases. But that doesn't lead to very serious problems anymore as it least the exception isn't thrown up in ComponentResourceRequestTarget anymore.