On 06/06/2014 18:01, Mark Thomas wrote:
> 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.
Looking at the execution flow, they could be removed but at the cost of
the ErrorReportValve doing more processing before it decided it wasn't
going to do anything. I think on reflection I'll go the finishResponse()
route.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
