I was offline today so couldn't comment on the different messages on the subject, so I'll try to summarize (here) my understanding, so far...
On Wed, Apr 27, 2016 at 8:41 PM, <wr...@apache.org> wrote: > Author: wrowe > Date: Wed Apr 27 18:41:49 2016 > New Revision: 1741310 > > URL: http://svn.apache.org/viewvc?rev=1741310&view=rev > Log: > > Ensure http2 follows http in the meaning of > status WRITE (meaning 'in the request processing > phase' even if still consuming the request body, > not literally in a 'now writing' state). This is indeed consistent with how we report http1 state currently, but maybe it could be more intuitive to report READ until the body is consumed in http1 rather than changing http2? Unless we want to minimize scoreboard updates, for performance reasons... > > Ensure a number of MPMs and the h2 connection io > no longer clobber the request status line during > state-only changes. While at it, clean up some > very ugly formatting and unnecessary decoration, > and avoid the wordy _from_conn() flavor when we > are not passing a connection_rec. Why ap_update_child_status_from_conn(), given a real or NULL c, would clobber the request line? The request line is untouched unless a non-NULL r is passed to update_child_status_internal(), and ap_update_child_status_from_conn() sets r to NULL. AIUI, what sets the request line to NULL in mpm_worker is when ap_read_request() fails (mostly after connection closed remotely or keep-alive timeout, actually any empty/NULL r->the_request from read_request_line() would do it). This may also happen in mpm_event but since the worker (scoreboard entry) has probably changed in between, this is possibly less visible. So how about: Index: server/protocol.c =================================================================== --- server/protocol.c (revision 1741108) +++ server/protocol.c (working copy) @@ -1093,13 +1093,14 @@ request_rec *ap_read_request(conn_rec *conn) access_status = r->status; r->status = HTTP_OK; ap_die(access_status, r); - ap_update_child_status(conn->sbh, SERVER_BUSY_LOG, r); + ap_update_child_status(conn->sbh, SERVER_BUSY_LOG, + r->the_request ? r : NULL); ap_run_log_transaction(r); r = NULL; apr_brigade_destroy(tmp_bb); goto traceout; case HTTP_REQUEST_TIME_OUT: - ap_update_child_status(conn->sbh, SERVER_BUSY_LOG, r); + ap_update_child_status(conn->sbh, SERVER_BUSY_LOG, NULL); if (!r->connection->keepalives) ap_run_log_transaction(r); apr_brigade_destroy(tmp_bb); ? Regarding this commit, the goal seems more about not clobbering the *client IP* until we read the next/real request on each connection, but this implies that we report more the last requests handled by each worker than the workers states themselves... Isn't the access log more appropriate for that purpose finally, whereas the scoreboard is more about workers' activity? However I agree that without doing something like this (I tried a different approach in PR 59333, not working yet... but it could with more work IMHO), with mpm_event we may find mixed data (from different requests/connections) in the same scoreboard entry :/ I think Stefan tried to address this (observed by testing http2 scores?) in 2.4.20 by blanking the fields before reading each request (unfortunately that broke the usual reporting, but I think the intention was laudable). So now, either we choose to not report workers' activity in between requests (there are just multi-purpose workers/request-pollers after all), or we save some "last request" informations in each conn_rec and restore them on the worker (score) handling the connection at each time (this is more reporting the workers' activity than the requests lines, which will pass from one worker to the other, even possibly duplicating requests lines with low activity). I prefer the latter option, but I understand the former too... Regards, Yann.
