2014-05-29 16:31 GMT+04:00 Mark Thomas <ma...@apache.org>:
> You've probably noticed I've been poking around the ErrorReportValve,
> the Response objects and other related code. I've been looking into an
> issue that one of the groups at $work has raised around Tomcat's error
> handling and I've been cleaning things up as I notice them to stop me
> getting distracted while I try and concentrate on the larger problem.
>
> I've also spotted a few places where I think we are duplicating
> functionality unnecessarily. I'm trying to clean this up as well. Partly
> because it is more efficient and partly because the code is easier to
> follow without it.
>
> So, that larger problem...
>
> It is to do with how uncaught exceptions are handled.
> ...
>
Reviewing r1600449 and followups.
// ErrorReportValve.java
[[[
if (response.isCommitted()) {
if (response.isErrorAfterCommit()) {
// Flush any data that is still to be written to the client
response.flushBuffer();
// Close immediately to signal to the client that something went
// wrong
response.getCoyoteResponse().action(ActionCode.CLOSE_NOW, null);
}
return;
}
]]]
1. If original error was due to I/O failure, then I expect
"response.flushBuffer()" call to fail with an IOException.
In ErrorReportValve#report() any IOException is silently swallowed. So
I think we can swallow it here.
// ErrorReportValve.java
[[[
if (throwable != null) {
// Make sure that the necessary methods have been called on the
// response. (It is possible a component may just have set the
// Throwable. Tomcat won't do that but other components might.)
// These are safe to call at this point as we know that the response
// has not been committed.
response.reset();
response.sendError(
HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
}
// One way or another, response.sendError() will have been called before
// execution reaches this point and suspended the response. Need to
// reverse that so this valve can write to the response.
response.setSuspended(false);
try {
report(request, response, throwable);
} catch (Throwable tt) {
ExceptionUtils.handleThrowable(tt);
}
]]]
I know that the above calls were essentially present before these
changes. Looking at them I have the following thoughts:
2. Looking at "if (throwable != null)".
The error might have already been handled either by custom error page
(StandardHostValve.custom(...)) or if there are several
ErrorReportValve valves in the chain.
In both cases the RequestDispatcher.ERROR_EXCEPTION attribute is not cleared.
The StandardHostValve saves us from this by always calling
response.flushBuffer();
just after invoking its custom() method. Thus "if
response.isCommitted()" branch is triggered and the error report valve
invocation returns early.
ErrorReportValve.report() can be updated to call flushBuffer() as
well. Otherwise I think if there are several ErrorReportValve valves
the "if throwable != null" branch will reset the response and it will
be generated a-new.
Though if there is no other good reason to flush, then it looks like a
hack (in StandardHostValve as well). (A known drawback of
flushBuffer() is that it causes chunked encoding with HTTP/1.1, even
if the whole response fits in a buffer).
I wonder if those flushBuffer() calls can be removed.
3. Some Tomcat components set RequestDispatcher.ERROR_EXCEPTION
attribute without setting any other error flags.
E.g. o.a.c.connector.Request#notifyAttributeAssigned),
#notifyAttributeRemoved().
I think this is the only use case where "if (throwable != null)"
branch and response.reset() call there are justified.
If response have been already committed the "if
response.isCommitted()" branch will be triggered, but not "if
response.isErrorAfterCommit()", as error flag has not been set.
So I interpret this use case as "Some error was detected, but it is
not fatal. If response have not been committed then report it. If it
has been committed, ignore it".
For 3. I think maybe do the following:
Replace "if (throwable != null)"
with "if (throwable != null && !response.isError())"
If the above replacement is done then I think that maybe those
flushBuffer() calls in StandardHostValve mentioned in "2." can be
removed.
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
