On Mon, Mar 12, 2018 at 3:17 PM, Eric Covener <cove...@gmail.com> wrote:
> On Mon, Mar 12, 2018 at 9:39 AM, Rainer Jung <rainer.j...@kippdata.de> wrote:
>>
>> Am 12.03.2018 um 13:24 schrieb yla...@apache.org:
>>>
>>> ==============================================================================
>>> --- httpd/httpd/branches/2.4.x/modules/http/http_request.c (original)
>>> +++ httpd/httpd/branches/2.4.x/modules/http/http_request.c Mon Mar 12
>>> 12:24:27 2018
>>> @@ -480,13 +480,9 @@ AP_DECLARE(void) ap_process_request(requ
>>> * Notice a timeout as an error message. This might be
>>> * valuable for detecting clients with broken network
>>> * connections or possible DoS attacks.
>>> - *
>>> - * It is still safe to use r / r->pool here as the eor bucket
>>> - * could not have been destroyed in the event of a timeout.
>>> */
>>> - ap_log_rerror(APLOG_MARK, APLOG_INFO, rv, r, APLOGNO(01581)
>>> - "Timeout while writing data for URI %s to the"
>>> - " client", r->unparsed_uri);
>>> + ap_log_cerror(APLOG_MARK, APLOG_INFO, rv, c, APLOGNO(01581)
>>> + "flushing data to the client");
>>> }
>>> }
>>> if (ap_extended_status) {
>>
>>
>> was it intentional to apply this directly to 2.4 without trunk (don't know
>> whether it applies there) and STATUS? Maybe because it is a logging only
>> change?
Oops, really not at all :/
Will revert there and re-commit in trunk, thanks for noticing.
>>
>> And was it intentional to change the contents of the log message, not only
>> cerror/c instead of rerror/r? If the change in log message text was
>> intentional, shouldn't we then use a new APLOGNO? And while "Timeout" sounds
>> like INFO or higher, "flushing" sounds more like a DEBUG or lower, at least
>> to me.
Actually I don't know the "rule" to change (or not) an APLOGNO when a
message is modified...
>
> I had the same thought re: the content change, but I guess the timeout
> part is redundant due to the rv we will pass in. I didn't look in
> context for write vs flush but i assume it is more accurate (just
> confidence in Yann)
Yes, strerror(APR_TIMEUP) mentions the timeout already, so while at it
I took the liberty to simplify the message.
We are flushing outgoing data before (possibly) blocking for incoming
ones here, and "flushing data to the client" is aligned to the
core_output_filter's logging style (since it's now connection level I
thought it made sense).
>
> All things being equal I think keeping the ID is probably the best
> compromise -- since it's the same condition.
My opinion too...
