Re: svn commit: r1742794 - /httpd/httpd/branches/2.4.x/STATUS

[William A Rowe Jr]

On Tue, May 10, 2016 at 11:36 PM, William A Rowe Jr 
wrote:

> On Tue, May 10, 2016 at 7:26 PM, Yann Ylavic  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..

Re: svn commit: r1742794 - /httpd/httpd/branches/2.4.x/STATUS

[William A Rowe Jr]

On Tue, May 10, 2016 at 7:26 PM, Yann Ylavic  wrote:

> On Tue, May 10, 2016 at 5:23 PM, William A Rowe Jr 
> wrote:
> > On Sun, May 8, 2016 at 7:29 AM, Yann Ylavic 
> 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.)
>
> Right, that's why/when the new thread clobbers the request-line.
> We are talking about MPM event only here, with sync MPMs we do
> SERVER_BUSY_READ for the first request on the connection only, and my
> goal is to have consistent reporting for all MPMs.
>

But that's not what the third suggested backport does, it toggles the
request
state to "W"rite when there is actually no request received (either it was
actually not ready, or had spuriously disconnected)


> > 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.
>
> The scoreboard reports that the thread is now working on a new
> connection (possibly kept alive), but clears half of the data
> associated with that connection (i.e. the last request/vhost)
> altogether.
>

Of course it should, because the old request is still parked on the old
worker thread until it is reclaimed, and the newly activated thread for
this connection never processed work for the old thread...


> >> 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.
>
> DSS?


Dirt simple stupid.


> > Complicating it is a really bad idea, and
> > misleads, if not actually breaking existing consumers of the scoreboard.
>
> My proposed patch (not that complicated IMHO) shouldn't break anything
> (if working as expected), it is meant for consistency between event
> and other MPMs (now that Rainer fixed the blanks there due to
> keepalive/close), see below.
>

I still need to further review, but you toggle a writing state for a request
with no inbound request, which is clearly wrong.


> >> 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.
>
> Right, for MPM event we can indeed consider that the reporting of the
> previous request for a thread is dead once this thread is given
> another connection, but when yet another thread will take care of the
> former connection, the previous request could show up in its reporting
> (that's how my patch works, faithfully reporting how each MPM works,
> without clobbering data unnecessarily).
>
> 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).
> 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).
> 2. t1 reads/closes c2 and clobbers r1 (blank).
> 3. t2 reads/closes c1 and clobbers r3 (blank).
> => t1 and t2 scoreboard's request/vhost are blank.
>
> 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).
> 2. t1 reads/closes c2 and restores r2.
> 3. t2 reads/closes c1 and restores r3.
> => t1 and t2 scoreboard's request/vhost contain relevent (last) info.
>

Thanks for all of the additional details, it would be helpful throughout the
rest of this d

Re: svn commit: r1742794 - /httpd/httpd/branches/2.4.x/STATUS

[Yann Ylavic]

On Tue, May 10, 2016 at 5:23 PM, William A Rowe Jr  wrote:
>
> On Sun, May 8, 2016 at 7:29 AM, Yann Ylavic  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.)

Right, that's why/when the new thread clobbers the request-line.
We are talking about MPM event only here, with sync MPMs we do
SERVER_BUSY_READ for the first request on the connection only, and my
goal is to have consistent reporting for all MPMs.

>
> 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.

The scoreboard reports that the thread is now working on a new
connection (possibly kept alive), but clears half of the data
associated with that connection (i.e. the last request/vhost)
altogether.

>
>> 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.

DSS?

> Complicating it is a really bad idea, and
> misleads, if not actually breaking existing consumers of the scoreboard.

My proposed patch (not that complicated IMHO) shouldn't break anything
(if working as expected), it is meant for consistency between event
and other MPMs (now that Rainer fixed the blanks there due to
keepalive/close), see below.

>
>> 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.

Right, for MPM event we can indeed consider that the reporting of the
previous request for a thread is dead once this thread is given
another connection, but when yet another thread will take care of the
former connection, the previous request could show up in its reporting
(that's how my patch works, faithfully reporting how each MPM works,
without clobbering data unnecessarily).

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).
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).
2. t1 reads/closes c2 and clobbers r1 (blank).
3. t2 reads/closes c1 and clobbers r3 (blank).
=> t1 and t2 scoreboard's request/vhost are blank.

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).
2. t1 reads/closes c2 and restores r2.
3. t2 reads/closes c1 and restores r3.
=> t1 and t2 scoreboard's request/vhost contain relevent (last) info.

>
>> How about the attached patch?
>
> This is the right direction for  - 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.

This would indeed also work in full event mode (i.e. future/trunk),
but it's legitimate in 2.4.x too, IMHO, since requests already cross
threads at keepalive/lingering-close time (with MPM event).

Re: svn commit: r1742794 - /httpd/httpd/branches/2.4.x/STATUS

[William A Rowe Jr]

I believe this is the right patch (or close to correct) for 2.4 branch, and
perhaps
even trunk with additional refactoring.

In the case of resuming a thread, it should be up to the event/advanced MPM
to reflect the correct no-info | [connection-info [request-info]] state of
the conn
at the time the thread is resumed with different work to perform.

On Sun, May 8, 2016 at 5:30 AM,  wrote:

> Author: rjung
> Date: Sun May  8 10:30:38 2016
> New Revision: 1742794
>
> URL: http://svn.apache.org/viewvc?rev=1742794&view=rev
> Log:
> Propose another (small) scoreboard/mod_status fix.
>
> Modified:
> httpd/httpd/branches/2.4.x/STATUS
>
> Modified: httpd/httpd/branches/2.4.x/STATUS
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/STATUS?rev=1742794&r1=1742793&r2=1742794&view=diff
>
> ==
> --- httpd/httpd/branches/2.4.x/STATUS (original)
> +++ httpd/httpd/branches/2.4.x/STATUS Sun May  8 10:30:38 2016
> @@ -181,6 +181,20 @@ PATCHES PROPOSED TO BACKPORT FROM TRUNK:
>   2.4.x patch: trunk works
>   +1: wrowe, rpluem
>
> +   * 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
> + 2.4.x patch: trunk works
> + +1: rjung
> +
>  PATCHES/ISSUES THAT ARE BEING WORKED
>
>*) http: Don't remove the Content-Length of zero from a HEAD response if
>
>
>

Re: svn commit: r1742794 - /httpd/httpd/branches/2.4.x/STATUS

[William A Rowe Jr]

On Sun, May 8, 2016 at 7:29 AM, Yann Ylavic  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  - 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.

Re: svn commit: r1742794 - /httpd/httpd/branches/2.4.x/STATUS

[Rainer Jung]

Am 08.05.2016 um 20:06 schrieb Yann Ylavic:

[top posting reodered]

On Sun, May 8, 2016 at 7:21 PM, Stefan Eissing
 wrote:



Am 08.05.2016 um 16:30 schrieb Rainer Jung :

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. F


Isn't conn_rec->keepalives an indicator?


Yes, !c->keepalives would work equivalently, but the issue is that
with mpm_event, at BUSY_READ time, the previous scoreboard entry is
not necessarily for the same connection, hence we could keep the
request-line/vhost related to another connection (mixup discussed in
PR 59333).

That's why I proposed to restore the previous request's informations
saved in the conn_rec itself, and since at the start of the connection
those would be blank, this also blanks the scoreboard entry when
necessary.


Ah, OK, then your patch is way better.

Regards,

Rainer

Re: svn commit: r1742794 - /httpd/httpd/branches/2.4.x/STATUS

[Yann Ylavic]

[top posting reodered]

On Sun, May 8, 2016 at 7:21 PM, Stefan Eissing
 wrote:
>
>> Am 08.05.2016 um 16:30 schrieb Rainer Jung :
>>
>> 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. F
>
> Isn't conn_rec->keepalives an indicator?

Yes, !c->keepalives would work equivalently, but the issue is that
with mpm_event, at BUSY_READ time, the previous scoreboard entry is
not necessarily for the same connection, hence we could keep the
request-line/vhost related to another connection (mixup discussed in
PR 59333).

That's why I proposed to restore the previous request's informations
saved in the conn_rec itself, and since at the start of the connection
those would be blank, this also blanks the scoreboard entry when
necessary.

Re: svn commit: r1742794 - /httpd/httpd/branches/2.4.x/STATUS

[Stefan Eissing]

Isn't conn_rec->keepalives an indicator?

/Stefan

> Am 08.05.2016 um 16:30 schrieb Rainer Jung :
> 
> 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. F

Re: svn commit: r1742794 - /httpd/httpd/branches/2.4.x/STATUS

[Rainer Jung]

Am 08.05.2016 um 16:30 schrieb Rainer Jung:

Am 08.05.2016 um 14:29 schrieb Yann Ylavic:

On Sun, May 8, 2016 at 12:30 PM,   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?


What about the attached one using a flag? (plus minor MMN change)

Regards,

Rainer

Index: include/httpd.h
===
--- include/httpd.h	(revision 1742822)
+++ include/httpd.h	(working copy)
@@ -1217,6 +1217,9 @@

 /** The minimum level of filter type to allow setaside buckets */
 int async_filter;
+
+/** Did we already see a request? Used by the scoreboard. */
+unsigned short request_seen;
 };

 struct conn_slave_rec {
Index: server/scoreboar

Re: svn commit: r1742794 - /httpd/httpd/branches/2.4.x/STATUS

[Rainer Jung]

Am 08.05.2016 um 14:29 schrieb Yann Ylavic:

On Sun, May 8, 2016 at 12:30 PM,   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

Re: svn commit: r1742794 - /httpd/httpd/branches/2.4.x/STATUS

[Yann Ylavic]

On Sun, May 8, 2016 at 12:30 PM,   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?

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 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).
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).

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?

Regards,
Yann.
Index: include/httpd.h
===
--- include/httpd.h	(revision 1742803)
+++ include/httpd.h	(working copy)
@@ -1095,6 +1095,9 @@ typedef enum {
 AP_CONN_KEEPALIVE
 } ap_conn_keepalive_e;
 
+/* AP_SB_*_SIZE needed by conn_rec */
+#include "scoreboard.h"
+
 /**
  * @brief Structure to store things which are per connection
  */
@@ -1217,6 +1220,10 @@ struct conn_rec {
 
 /** The minimum level of filter type to allow setaside buckets */
 int async_filter;
+
+/** Preserve scoreboard's worker last request infos */
+char sb_lastrline[AP_SB_RLINE_SIZE];
+char sb_lastvhost[AP_SB_VHOST_SIZE];
 };
 
 struct conn_slave_rec {
Index: include/scoreboard.h
===
--- include/scoreboard.h	(revision 1742803)
+++ include/scoreboard.h	(working copy)
@@ -85,6 +85,9 @@ typedef enum {
 SB_SHARED = 2
 } ap_scoreboard_e;
 
+#define AP_SB_RLINE_SIZE 64
+#define AP_SB_VHOST_SIZE 32
+
 /* stuff which is worker specific */
 typedef struct worker_score worker_score;
 struct worker_score {
@@ -113,8 +116,8 @@ struct worker_score {
 struct tms times;
 #endif
 char client[40];/* Keep 'em small... but large enough to hold an IPv6 address */
-char request[64];   /* We just want an idea... */
-char vhost[32]; /* What virtual host is being accessed? */
+char request[AP_SB_RLINE_SIZE]; /* We just want an idea... */
+char vhost[AP_SB_VHOST_SIZE];   /* What virtual host is being accessed? */
 char protocol[16];  /* What protocol is used on the connection? */
 };
 
Index: server/connection.c
===
--- server/connection.c	(revision 1742803)
+++ server/connection.c	(working copy)
@@ -101,7 +101,7 @@ AP_DECLARE(int) ap_prep_lingering_close(conn_rec *
 ap_run_pre_close_connection(c);
 
 if (c->sbh) {
-ap_update_child_status(c->sbh, SERVER_CLOSING, NULL);
+ap_update_child_status_from_conn(c->sbh, SERVER_CLOSING, c);
 }
 return 0;
 }
Index: server/scoreboard.c
===
--- server/scoreboard.c	(revision 1742803)
+++ server/scoreboard.c	(working copy)
@@ -499,9 +499,10 @@ static int update_child_status_internal(int child_
 }
 else if (r) {
 copy_request(ws->request, sizeof(ws->request), r);
+apr_cpystrn(c->sb_lastrline, ws->request, sizeof(c->sb_lastrline));
 }
 else if (c) {
-ws->request[0]='\0';
+apr_cpystrn(ws->request, c->sb_lastrline, sizeof(ws->request));
 }
 
 if (r && r->useragent_ip) {
@@ -522,6 +523,8 @@ static