On Tue, May 10, 2016 at 11:36 PM, William A Rowe Jr <wr...@rowe-clan.net> wrote:
> On Tue, May 10, 2016 at 7:26 PM, Yann Ylavic <ylavic....@gmail.com> wrote: > >> >> The case where this happens is for keepalive/lingering-close handling. >> Suppose thread t1 handles request r1 on connection c1, and t1 is >> released after r1 (when c1 is kept alive). >> > So thread t1 has traditionally reported 'K'eepalive state, but that is based on the fact that t1 is still 'busy' waiting on the next request from c1, in the classic prefork or worker models. In the event case, thread t1 should now be reported as Idle, because that worker no longer has any work assigned to it; however, the score slot should continue to display t1's last request r1 until a new connection or request occupies that slot, at which time everything related to r1 must be zeroed out (replaced again by c1 where appropriate). What we might want in the score is to track "unaffiliated connections" with no thread assigned, in the state they were last in, but right now we have no such reporting mechanism. > Same for t2 with r2 on c2. >> >> Now the following cases (not exhaustive): >> 1. r3 arrives on c1, t2 handles the event (read). >> 2. c2 is closed/reset remotely, t1 handles the event (read). >> 2. c1 is closed/reset remotely, t2 handles the event (read). >> >> Without my patch: >> 1. t2 reports r3 on c1, "losing" r2 on c2 (no more reported by the >> scoreboard), while t1 still reports r1 on c1 (although "superseded" by > > r3). > > That behavior is by design, because t2 isn't performing any further work on r2 from c2 - it is performing work for c1. At this moment, c1 should appear in both score slots, t1 still reports r1, but in an idle state so that we know that isn't what the thread is doing right now, while t2 also reports c1, which it picked up, in an active processing state. The t2 should not report r1 because r1 is "done and finished" and t2 never performed work on r1. At this time, c2 shouldn't appear in the scoreboard at this time; as I'd observed above, that scoreboard functionality doesn't exist right now. > 2. t1 reads/closes c2 and clobbers r1 (blank). >> > That's fine, t1's only work on c2 was to read a request, which it did not find. t1 is working on c2. t1 did not handle the request r1. > 3. t2 reads/closes c1 and clobbers r3 (blank). >> => t1 and t2 scoreboard's request/vhost are blank. >> > At the time we read no request, if we have not yet yielded the thread, the existing score slot for t2 should simply be updated with the status of closing, no change to the request or vhost because these fields should not be updated if no request is resolved. If t2 yields after completing r3, when it resumes it has no request. This is true whether you have jumped threads, or managed to land back in the same thread as you illustrate above for c2. > With my patch: > 1. t2 reports r3 on c1, "losing" r2 on c2 (no more reported by the > scoreboard), while t1 still reports r1 on c1 (although "superseded" by > r3). > AIUI the propsoed trunk patch introduces an edge case where t2 will briefly report r1 while t1 is still reporting r1, until the request is read complete, and we are toggling to write status... is this correct? > 2. t1 reads/closes c2 and restores r2. > Incorrect behavior, IMO. t1 performed no work on r2. > 3. t2 reads/closes c1 and restores r3. > => t1 and t2 scoreboard's request/vhost contain relevent (last) info. > So in bullet 2. above, it isn't the last relevant activity of the thread t1. You can argue that r3 was the most recent activity performed on t2, but we would want to perform an identity check that the score slot has simply resumed the same connection, that it had not jumped threads, and that there was not intervening activity on that thread. In httpd 2.4 and prior, the scoreboard is relating what each thread's action is. In httpd 3.0, you could certainly propose that the score is connection oriented, with one slot per connection irrespective of which thread (if any) is handling the request. Then the proposed behavior makes a great deal of sense. I agree that we should simply not update the scoreboard with a fresh "W" update when we have no request to write (the client disconnected and went away). That's why I cast a -1 on the test you added for r->the_request, that isn't the right way to know that there is no request being written. I need to still dig further but am really glad you pointed our attention at the offending status update. I expect we can dodge that update altogether for a no-request state. Believe that we may be missing an update to the scoreboard at the moment we wake up an event thread, so I'm looking in that corner as well. In httpd 3.0 we will have to recall what request is in-flight since we will be able to set-aside a request and wake it back up on a different thread, and update the scoreboard appropriately, but I still don't believe that's the correct behavior on 2.4 maintenance branch..
