Am 08.05.2016 um 14:29 schrieb Yann Ylavic:
On Sun, May 8, 2016 at 12:30 PM, <rj...@apache.org> wrote:
+ * Don't globber scoreboard request info if read_request_line() fails with
+ a timeout. In that case there's not yet any new useful request info
+ available.
+ Noticed via server-status showing request "NULL" after keep-alive
+ connections timed out.
+ No CHANGES entry needed, because there's already one for PR 59333.
+ Sorry for the two patches. The second is better. Both change the same
+ line, so technically only the second is needed, but merging both keeps
+ mergeinfo more complete.
+ trunk patch: http://svn.apache.org/r1742791
+ http://svn.apache.org/r1742792
This is to avoid "NULL" instead of blank, right?
No, "NULL" instead of the request line from the previous request.
ISTM that the request line has already been clobbered (blanked) by the
previous call to ap_update_child_status_from_conn(SERVER_BUSY_READ) in
ap_process_http_[a]sync_connection(), where we already lost the
previous request's line.
I observed the problem for prefork. The sync connection loop differs
from the async loop in that it calls ap_update_child_status_from_conn()
only once and then loops over the requests. The async loop calls it
before each request. That's why I observed it for prefork. For event you
are right, the field would've already been clobbered.
That's why I proposed to save the previous request line and host in
the conn_rec, and use them in the scoreboard until a new (real)
request is available (it didn't work as expected because the
scoreboard was clobbered in multiple places before Bill's changes, but
it should work better/easily now).
I guess we need to define, at which point in time we want the shown data
to switch between previous conn/req and current conn/req and how much
consistency we need for the conn data vs. the req data. conn data would
be client IP, req data request line and vhost.
I'd say when a new connection comes in, it would be OK to immediately
reset the data, set client IP and keep conn and req data consistent.
For further steps in an existing connection, I'd prefer to keep the data
until another request was read, so especially if a timeout prevented the
next one from being read.
One problem is if the client IP comes from a header (mod_remoteip). In
that case, we would have to switch behavior for a new connection,
because we don't know (yet) the client IP when the new connection
starts. Or we accept to use the connection client IP until the first
request comes in. IMHO that would be OK.
If that would be consensus, it would mean, we should only reset the
request info at the start of a connection. For sync connections, that's
exactly what happens already. For http2 I'm not sure. For async
connections I don't know, whether we can identify from the conn_rec,
whether we are at the start of the connection or not. If we could, then
we could simply adjust ap_update_child_status_from_conn() to only
overwrite the request info with the empty string if we are at the start
of the connection. I don't think we really need to safe the full request
line and vhost in the conn_rec, a flag that tells us whether we are at
start or not should suffice.
That would be: when no r is available but c is, use the saved
request-line/host from c; when r is available, use the ones from r and
update them in c.
That way there would be no blank at all (IOW, the scoreboard entry
would be left with the latest request handled on the connection, not a
blank which inevitably ends every connection after keepalive timeout
or close, and shouldn't be taken into account as an empty request
IMHO).
Agreed. The only blank that might be OK is after a new connection came
in, so we update the client IP address. Then having the new IP address
but the old request line would be inconsistent data, so it would be
better to zero the request info.
Also, that would be consistent in both MPMs worker and event, whereas
currently event is more likely to show this behaviour, thanks to
queuing (no worker thread used for keepalive handling and hence no
BUSY_READ during that time, no clobbering either).
Thus for mpm_event, we'd need to use
ap_update_child_status_from_conn(SERVER_CLOSING, c) so that the saved
request-line/vhost from the connection get reused when/if a worker
thread handles the lingering close.
How about the attached patch?
Looks reasonable if we want to safe the strings in the conn_rec. I'll
give it a try.
Regards,
Rainer
