On 06/06/2014 12:29, Konstantin Kolinko wrote:
<snip/>
> 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.
Agreed. Fixed.
> // 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.
finishResponse() would avoid chunking if the response fitted into the
buffer.
> 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.
Ah. Interesting. I'll take a closer look at that.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
