On Sun, May 8, 2016 at 7:29 AM, Yann Ylavic <ylavic....@gmail.com> wrote:
> > 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. > That's FINE. In the previous call we are already on to another thread, this thread did not perform work on the previous request (or if it had, it had surrendered the thread and accidentally ended back up there.) The scoreboard reports what work this *thread* has performed. So far, if it is a wakeup to read a new request, on the event MPM in 2.4 this is not request-based work. 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). > It isn't workable because every time we hack around this issue, we mask the actual errors in the calls to the scoreboard API. In 2.4, the API design is DSS. Complicating it is a really bad idea, and misleads, if not actually breaking existing consumers of the scoreboard. > 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). > Which is all wrong IMHO. If the request completed, that thread still reports the previous request in an idle or keepalive state until that thread is woken up to do new work. > 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? > This is the right direction for <trunk> - we will be waking up threads to resume work on an already in-flight request, correct? That request needs to be displayed because that is the work this thread is performing. On trunk and even with your patch as-is, IIUC you are filling in the score request field for a thread that never performed work on a given request, which is invalid data. If we refine this idea somewhat to null out the request once the request has been entirely fulfilled on a given thread, such that the thread still reflects what last request work it performed, but the resumed connection does not report a request it didn't satisfy, I think we will have a solid fix for trunk. For 2.4 - I'm strongly -1 until convinced we can't navigate without it, given that 2.4 does not allow cross-threaded request_rec lifespans.
