Re: svn commit: r1741310 - in /httpd/httpd/trunk: modules/http2/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/ server/mpm/winnt/ server/mpm/worker/

[Ruediger Pluem]

On 05/03/2016 05:51 PM, William A Rowe Jr wrote:
> On Mon, May 2, 2016 at 3:09 AM, Ruediger Pluem  > wrote:
> 
> 
> On 04/28/2016 10:19 AM, Ruediger Pluem wrote:
> >
> > On 04/27/2016 08: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=rev
> >> Log:
> >>
> >>   Ensure the useragent_ip is only used in the case
> >>   where it has been initialized, fall back on the
> >>   connection's remote_ip if the status is accidently
> >>   updated from an uninitialized request_rec.
> >>
> >> --- httpd/httpd/trunk/server/scoreboard.c (original)
> >> +++ httpd/httpd/trunk/server/scoreboard.c Wed Apr 27 18:41:49 2016
> >> @@ -501,7 +501,7 @@ static int update_child_status_internal(
> >>  copy_request(ws->request, sizeof(ws->request), r);
> >>  }
> >>
> >> -if (r) {
> >> +if (r && r->useragent_ip) {
> >>  if (!(val = ap_get_useragent_host(r, REMOTE_NOLOOKUP, 
> NULL)))
> >>  apr_cpystrn(ws->client, r->useragent_ip, 
> sizeof(ws->client));
> >
> > Hm, wouldn't it be better to just encapsulate the above line in an if 
> (r->useragent_ip)
> > or do we already know that ap_get_useragent_host(r, REMOTE_NOLOOKUP, 
> NULL) cannot deliver
> > something meaningful if r->useragent_ip == NULL?
> 
> 
> It *could* deliver; but if r->useragent_ip is unset, this is 
> a premature call to set up the score entry, the request
> read hook hasn't completed and will fail to deliver.  But
> I don't know what you mean by "the above line" - you
> mean the apr_cpystrn?  As you point out, the call is
> fairly useless without a completed request rec and 
> known useragent_ip in the first place (you want to
> look up the hostname of null address - but then 
> guard against copying a NULL string once it has
> failed ? :-)
> 
> By falling back on the conn-based lookup, in the code you 
> snipped... we are populating the conn-based hostname, 
> which may be useful datum elsewhere...

Good point. I missed the else if (c) stuff below. Thanks for pointing out.

Regards

Rüdiger

Re: svn commit: r1741310 - in /httpd/httpd/trunk: modules/http2/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/ server/mpm/winnt/ server/mpm/worker/

[William A Rowe Jr]

On Tue, May 3, 2016 at 10:51 AM, William A Rowe Jr 
wrote:

>
> If we want to be more robust, have a look at the current 2.4.x
> implementation of ap_get_useragent_host() in core.c...
>
> if (r->useragent_addr == conn->client_addr) {
> return ap_get_remote_host(conn, r->per_dir_config, type,
> str_is_ip);
> }
>
> Perhaps this would be better...
>
> if (!r->useragent_addr || (r->useragent_addr == conn->client_addr)) {
> return ap_get_remote_host(conn, r->per_dir_config, type,
> str_is_ip);
> }
>

Committed and proposed for backport.

Re: svn commit: r1741310 - in /httpd/httpd/trunk: modules/http2/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/ server/mpm/winnt/ server/mpm/worker/

[William A Rowe Jr]

On Mon, May 2, 2016 at 3:09 AM, Ruediger Pluem  wrote:

>
> On 04/28/2016 10:19 AM, Ruediger Pluem wrote:
> >
> > On 04/27/2016 08: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=rev
> >> Log:
> >>
> >>   Ensure the useragent_ip is only used in the case
> >>   where it has been initialized, fall back on the
> >>   connection's remote_ip if the status is accidently
> >>   updated from an uninitialized request_rec.
> >>
> >> --- httpd/httpd/trunk/server/scoreboard.c (original)
> >> +++ httpd/httpd/trunk/server/scoreboard.c Wed Apr 27 18:41:49 2016
> >> @@ -501,7 +501,7 @@ static int update_child_status_internal(
> >>  copy_request(ws->request, sizeof(ws->request), r);
> >>  }
> >>
> >> -if (r) {
> >> +if (r && r->useragent_ip) {
> >>  if (!(val = ap_get_useragent_host(r, REMOTE_NOLOOKUP,
> NULL)))
> >>  apr_cpystrn(ws->client, r->useragent_ip,
> sizeof(ws->client));
> >
> > Hm, wouldn't it be better to just encapsulate the above line in an if
> (r->useragent_ip)
> > or do we already know that ap_get_useragent_host(r, REMOTE_NOLOOKUP,
> NULL) cannot deliver
> > something meaningful if r->useragent_ip == NULL?
>

It *could* deliver; but if r->useragent_ip is unset, this is
a premature call to set up the score entry, the request
read hook hasn't completed and will fail to deliver.  But
I don't know what you mean by "the above line" - you
mean the apr_cpystrn?  As you point out, the call is
fairly useless without a completed request rec and
known useragent_ip in the first place (you want to
look up the hostname of null address - but then
guard against copying a NULL string once it has
failed ? :-)

By falling back on the conn-based lookup, in the code you
snipped... we are populating the conn-based hostname,
which may be useful datum elsewhere...

[...]
else if (c) {
if (!(val = ap_get_remote_host(c,
c->base_server->lookup_defaults,
   REMOTE_NOLOOKUP, NULL)))
apr_cpystrn(ws->client, c->client_ip, sizeof(ws->client));
else
apr_cpystrn(ws->client, val, sizeof(ws->client));

In all cases we should have the value for (c) where (r) is passed,
whether r was fully populated yet or not.

If we want to be more robust, have a look at the current 2.4.x
implementation of ap_get_useragent_host() in core.c...

if (r->useragent_addr == conn->client_addr) {
return ap_get_remote_host(conn, r->per_dir_config, type, str_is_ip);
}

Perhaps this would be better...

if (!r->useragent_addr || (r->useragent_addr == conn->client_addr)) {
return ap_get_remote_host(conn, r->per_dir_config, type, str_is_ip);
}

Re: svn commit: r1741310 - in /httpd/httpd/trunk: modules/http2/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/ server/mpm/winnt/ server/mpm/worker/

[Ruediger Pluem]

On 04/28/2016 10:19 AM, Ruediger Pluem wrote:
> 
> 
> On 04/27/2016 08: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=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).
>>
>>   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.
>>
>>   Ensure the useragent_ip is only used in the case
>>   where it has been initialized, fall back on the
>>   connection's remote_ip if the status is accidently
>>   updated from an uninitialized request_rec.
>>
>>
>> Modified:
>> httpd/httpd/trunk/modules/http2/h2_conn_io.c
>> httpd/httpd/trunk/modules/http2/h2_task.c
>> httpd/httpd/trunk/server/core.c
>> httpd/httpd/trunk/server/mpm/event/event.c
>> httpd/httpd/trunk/server/mpm/motorz/motorz.c
>> httpd/httpd/trunk/server/mpm/simple/simple_io.c
>> httpd/httpd/trunk/server/mpm/winnt/child.c
>> httpd/httpd/trunk/server/mpm/worker/worker.c
>> httpd/httpd/trunk/server/scoreboard.c
>>
> 
>> Modified: httpd/httpd/trunk/server/scoreboard.c
>> URL: 
>> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/scoreboard.c?rev=1741310=1741309=1741310=diff
>> ==
>> --- httpd/httpd/trunk/server/scoreboard.c (original)
>> +++ httpd/httpd/trunk/server/scoreboard.c Wed Apr 27 18:41:49 2016
>> @@ -501,7 +501,7 @@ static int update_child_status_internal(
>>  copy_request(ws->request, sizeof(ws->request), r);
>>  }
>>  
>> -if (r) {
>> +if (r && r->useragent_ip) {
>>  if (!(val = ap_get_useragent_host(r, REMOTE_NOLOOKUP, NULL)))
>>  apr_cpystrn(ws->client, r->useragent_ip, 
>> sizeof(ws->client));
> 
> Hm, wouldn't it be better to just encapsulate the above line in an if 
> (r->useragent_ip)
> or do we already know that ap_get_useragent_host(r, REMOTE_NOLOOKUP, NULL) 
> cannot deliver
> something meaningful if r->useragent_ip == NULL?
> 

Comments?

Regards

Rüdiger

Re: svn commit: r1741310 - in /httpd/httpd/trunk: modules/http2/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/ server/mpm/winnt/ server/mpm/worker/

[Yann Ylavic]

On Thu, Apr 28, 2016 at 5:22 PM, William A Rowe Jr  wrote:
>
> What I meant is that the request line *MUST* be touched - wiped
> clean - for any status_from_conn(c) request.
>
> When we say we preserve the most recent state of the worker in
> the scoreboard, that old request line is gone.  Every time we are
> doing a conn-based refresh of that worker status, the req-based
> data is no longer relevant and needs to go away.

OK, I'm fine with this, much prefered over mixing (which finally I
introduced in my incomplete revert, understood ;)

>
> See
> http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/server/scoreboard.c?r1=1239125=1741240_format=h
>
> for the delta between 2.4.1 and 2.4.broken (current state).
>
>  else if (c) {
> -apr_cpystrn(ws->client, ap_get_remote_host(c, NULL,
> -REMOTE_NOLOOKUP, NULL), sizeof(ws->client));
> -ws->request[0]='\0';
> -ws->vhost[0]='\0';

Indeed, I missed that, too much influenced by the original report
about blanks introduced in 2.4.20, but that was only about the client
IP, not the vhost/request-line which have always been blank in
SERVER_BUSY_READ...

Your proposed backport looks good to me now, thanks for your patience :)

Re: svn commit: r1741310 - in /httpd/httpd/trunk: modules/http2/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/ server/mpm/winnt/ server/mpm/worker/

[William A Rowe Jr]

On Thu, Apr 28, 2016 at 4:37 AM, Yann Ylavic  wrote:

> On Thu, Apr 28, 2016 at 4:30 AM, William A Rowe Jr 
> wrote:
> > On Wed, Apr 27, 2016 at 6:16 PM, Yann Ylavic 
> wrote:
> >>
> >> On Wed, Apr 27, 2016 at 8:41 PM,   wrote:
> >> >   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...
> >
> >
> > Well, we always want to be considerate of performance.
> >
> > That said, we absolutely must not change the semantic meanings of
> > the server-status results on the maintenance branch.  Change those
> > meanings on trunk for a future 2.6/3.0?  Sure... but 2.4.x needs to
> > behave as 2.4.x has behaved from the start.
>
> Agreed, we can't change it for http1 in 2.4.x, but maybe http2 has
> other considerations WRT reading/writing on the master connection,
> reporting READ or WRITE for http2 is not really a compatibility issue
> IMHO.
>

It is, yes, the server-status has well understood semantics and changing
them on a released branch is never acceptable. The master worker already
has special semantics which are new. No reason to change the slave
workers between http/1 and http/2 as they are doing the same things.


> >> >   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?
> >
> >
> > Given a real conn_rec record, we have no request line.  Therefore the
> result
> > is NULL.  There is no purpose in modifying the conn_rec related fields
> once
> > correctly recorded.  The next chance they have for being modified in any
> > meaningful way is during an ssl renegotiation or the processing of the
> Host:
> > and X-F-F: headers during the read_request phase.
>
> That's more an optimization (not rewrite existing values with the same
> ones), but driven by how http1 in *2.4.x* works currently (this relies
> on the same worker handling the connection for the lifetime of the
> request).
>



> When SUSPENDED is used (trunk does it already at several places) we
> must overwrite the fields with actual values.
>

SUSPENDED within a request cannot be supported on 2.4.x as modules
are allowed to make thread storage and thread-local storage assumptions.
E.g. it breaks modules in an unexpected manner, and it is our responsibility
to inform users of this by bumping the version major.  This wasn't seen as
a big of an issue when we introduced event and launched 2.2.x, because
far more third party modules are request handlers, and are not connection-
based.  But we still bumped the version minor when we allowed the
connection to float between threads. This wasn't changed in maintained
release branch.

On trunk, it will have to become the responsibility of the MPM to restore
the right score slot state before reentering the processing loop on a given
worker thread, multiple times per connection or request, as that MPM
sends requests across threads.


> >> 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.
> >
> > That was incorrect - to the extend that you've changed it since 2.4.1,
> > such a change must be reverted.
>
> I meant ap_update_child_status_from_conn() gives a NULL r to
> update_child_status_internal(), so it won't change the existing
> request line (as opposed to setting it to the NULL value in the
> scoreboard).


What I meant is that the request line *MUST* be touched - wiped
clean - for any status_from_conn(c) request.

When we say we preserve the most recent state of the worker in
the scoreboard, that old request line is gone.  Every time we are
doing a conn-based refresh of that worker status, the req-based
data is no longer relevant and needs to go away.

> At the time we process a new connection (before, and again after we
> > have parsed any SNI host handshake) - there is NO REQUEST.  Any
> > lingering request value must be blanked out at that time.  Once the
> > request URI is parsed we again invoke update_child_status, this time
> > with the request_rec with a now-known request string.
>
> So there is a window where some values need to be blanked or restored
> from elsewhere, so to avoid mixing previous values 

Re: svn commit: r1741310 - in /httpd/httpd/trunk: modules/http2/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/ server/mpm/winnt/ server/mpm/worker/

[Yann Ylavic]

On Thu, Apr 28, 2016 at 11:37 AM, Yann Ylavic  wrote:
>
> OK, so the worker scoreboard entry is up to date with the latest
> connection/request handled.
> Now let's consider the case in KEEAPALIVE state where a new request
> arrives (possibly on another connection for mpm_event), or in
> READY/closed state (for any MPM) where a new connection+request is
> handled by the same worker.
>
> When we transition to READ just before calling ap_read_request(), we
> update the client IP in the scoreboard, but keep the request line
> (from the previous request) untouched, mixed data...
> For mpm_event this is a very transient state since the worker was
> scheduled only because the connection is POLLIN ready, so
> ap_read_request() won't block.
> For mpm_worker though, this may last KeepAliveTimeout.
> Moreover for both MPMs, if the connection is closed by the client the
> worker score will stay as is until next (real) request is read on
> another connection, or for mpm worker if keepalive expires the request
> line will be set to the value NULL (unless patch below is applied).
> That's what is observed in PR 59333, and I don't see where your commit
> change this.
> We need to either accept/document mixing, or blank the request line
> (but it will be very noticeable by users as it is already in 2.4.20),
> or restore the latest request line (and vhost) on the connection
> (hence also store it by connection, in conn_rec).

So maybe for 2.4.x we could add the following to you commit:

Index: modules/http/http_core.c
===
--- modules/http/http_core.c(revision 1741396)
+++ modules/http/http_core.c(working copy)
@@ -141,8 +141,15 @@ static int ap_process_http_async_connection(conn_r
 AP_DEBUG_ASSERT(cs->state == CONN_STATE_READ_REQUEST_LINE);

 while (cs->state == CONN_STATE_READ_REQUEST_LINE) {
-ap_update_child_status_from_conn(c->sbh, SERVER_BUSY_READ, c);
+worker_score *ws = ap_get_scoreboard_worker(c->sbh);

+if (ws->client[0]) {
+ap_update_child_status(c->sbh, SERVER_BUSY_READ, NULL);
+}
+else {
+ap_update_child_status_from_conn(c->sbh, SERVER_BUSY_READ, c);
+}
+
 if ((r = ap_read_request(c))) {

 c->keepalive = AP_CONN_UNKNOWN;
@@ -182,13 +189,20 @@ static int ap_process_http_sync_connection(conn_re
 conn_state_t *cs = c->cs;
 apr_socket_t *csd = NULL;
 int mpm_state = 0;
+worker_score *ws = ap_get_scoreboard_worker(c->sbh);

+if (ws->client[0]) {
+ap_update_child_status(c->sbh, SERVER_BUSY_READ, NULL);
+}
+else {
+ap_update_child_status_from_conn(c->sbh, SERVER_BUSY_READ, c);
+}
+
 /*
  * Read and process each request found on our connection
  * until no requests are left or we decide to close.
  */

-ap_update_child_status_from_conn(c->sbh, SERVER_BUSY_READ, c);
 while ((r = ap_read_request(c)) != NULL) {
 apr_interval_time_t keep_alive_timeout = r->server->keep_alive_timeout;

Index: server/protocol.c
===
--- server/protocol.c(revision 1741396)
+++ 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);
+if (r->the_request && r->the_request[0]) {
+ap_update_child_status(conn->sbh, SERVER_BUSY_LOG, r);
+}
 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);
 if (!r->connection->keepalives)
 ap_run_log_transaction(r);
 apr_brigade_destroy(tmp_bb);
_

Such that we don't update the client IP until a request line is read.
So we'd report WRITE + new-request for a request being handled, and
READ + old-request when waiting for a new request.

Re: svn commit: r1741310 - in /httpd/httpd/trunk: modules/http2/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/ server/mpm/winnt/ server/mpm/worker/

[Yann Ylavic]

On Thu, Apr 28, 2016 at 4:30 AM, William A Rowe Jr  wrote:
> On Wed, Apr 27, 2016 at 6:16 PM, Yann Ylavic  wrote:
>>
>> On Wed, Apr 27, 2016 at 8:41 PM,   wrote:
>> >   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...
>
>
> Well, we always want to be considerate of performance.
>
> That said, we absolutely must not change the semantic meanings of
> the server-status results on the maintenance branch.  Change those
> meanings on trunk for a future 2.6/3.0?  Sure... but 2.4.x needs to
> behave as 2.4.x has behaved from the start.

Agreed, we can't change it for http1 in 2.4.x, but maybe http2 has
other considerations WRT reading/writing on the master connection,
reporting READ or WRITE for http2 is not really a compatibility issue
IMHO.

>
>>
>> >   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?
>
>
> Given a real conn_rec record, we have no request line.  Therefore the result
> is NULL.  There is no purpose in modifying the conn_rec related fields once
> correctly recorded.  The next chance they have for being modified in any
> meaningful way is during an ssl renegotiation or the processing of the Host:
> and X-F-F: headers during the read_request phase.

That's more an optimization (not rewrite existing values with the same
ones), but driven by how http1 in *2.4.x* works currently (this relies
on the same worker handling the connection for the lifetime of the
request).
When SUSPENDED is used (trunk does it already at several places) we
must overwrite the fields with actual values.

>
>>
>> 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.
>
>
> That was incorrect - to the extend that you've changed it since 2.4.1,
> such a change must be reverted.

I meant ap_update_child_status_from_conn() gives a NULL r to
update_child_status_internal(), so it won't change the existing
request line (as opposed to setting it to the NULL value in the
scoreboard).

>
> At the time we process a new connection (before, and again after we
> have parsed any SNI host handshake) - there is NO REQUEST.  Any
> lingering request value must be blanked out at that time.  Once the
> request URI is parsed we again invoke update_child_status, this time
> with the request_rec with a now-known request string.

So there is a window where some values need to be blanked or restored
from elsewhere, so to avoid mixing previous values of the worker with
new ones not yet available (see below).

>
>>
>> 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.
>
>
> You don't understand it.
>
> Envision this sequence, which is how the scoreboard was designed;
>
> Initialization -> entire score record is voided
>
> Connection -> score entry tagged READ, what is known of the
> connecting client and the target vhost is recorded
>
> Connection SNI -> score entry updated with new target vhost
>
> Request read -> score entry again updated with new target vhost,
> the request identifier is updated, score entry tagged WRITE
>
> Request body is read, Response body is prepared
>
> Request complete -> score entry tagged LOGGING, and NO
> other fields need to be updated
>
> Request logged, left in keepalive state -> score entry tagged
> KEEPALIVE, NO other fields need to be updated
>
> Connection closed -> score entry tagged IDLE, NO OTHER
> FIELDS SHOULD BE UPDATED until the worker is reused
> (even in the case of event MPM).

OK, so the worker scoreboard entry is up to date with the latest
connection/request handled.
Now let's consider the case in KEEAPALIVE state where a new request
arrives (possibly on another connection for mpm_event), or in
READY/closed state (for any MPM) where a new connection+request is
handled by the same worker.

When we transition to READ just before calling 

Re: svn commit: r1741310 - in /httpd/httpd/trunk: modules/http2/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/ server/mpm/winnt/ server/mpm/worker/

[Stefan Eissing]

> Am 28.04.2016 um 10:08 schrieb Rainer Jung :
> 
> Am 28.04.2016 um 04:30 schrieb William A Rowe Jr:
>> On Wed, Apr 27, 2016 at 6:16 PM, Yann Ylavic > > wrote:
>> 
>>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,  >> wrote:
>>> Author: wrowe
>>> Date: Wed Apr 27 18:41:49 2016
>>> New Revision: 1741310
>>>
>>> URL:http://svn.apache.org/viewvc?rev=1741310=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...
>> 
>> 
>> Well, we always want to be considerate of performance.
>> 
>> That said, we absolutely must not change the semantic meanings of
>> the server-status results on the maintenance branch.  Change those
>> meanings on trunk for a future 2.6/3.0?  Sure... but 2.4.x needs to
>> behave as 2.4.x has behaved from the start.
> 
> I agree we shouldn't change the semantics of R/W during 2.4. People might 
> monitor the numbers of R and W and will be quite surprised by seeing a 
> noticeable shift from W to R.
> 
> I think it would be a good change for 2.6/3.0 to make R including reading the 
> request body and only after having read it switch to W. We can mention it in 
> CHANGES / migration guide and personally I think those semantics would be 
> more intuitive.

That is how I understood it to be when I made the status updates in mod_http2. 
But it should of course be in line how we report the http/1.x scores.

Re: svn commit: r1741310 - in /httpd/httpd/trunk: modules/http2/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/ server/mpm/winnt/ server/mpm/worker/

[Ruediger Pluem]

On 04/27/2016 08: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=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).
> 
>   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.
> 
>   Ensure the useragent_ip is only used in the case
>   where it has been initialized, fall back on the
>   connection's remote_ip if the status is accidently
>   updated from an uninitialized request_rec.
> 
> 
> Modified:
> httpd/httpd/trunk/modules/http2/h2_conn_io.c
> httpd/httpd/trunk/modules/http2/h2_task.c
> httpd/httpd/trunk/server/core.c
> httpd/httpd/trunk/server/mpm/event/event.c
> httpd/httpd/trunk/server/mpm/motorz/motorz.c
> httpd/httpd/trunk/server/mpm/simple/simple_io.c
> httpd/httpd/trunk/server/mpm/winnt/child.c
> httpd/httpd/trunk/server/mpm/worker/worker.c
> httpd/httpd/trunk/server/scoreboard.c
> 

> Modified: httpd/httpd/trunk/server/scoreboard.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/scoreboard.c?rev=1741310=1741309=1741310=diff
> ==
> --- httpd/httpd/trunk/server/scoreboard.c (original)
> +++ httpd/httpd/trunk/server/scoreboard.c Wed Apr 27 18:41:49 2016
> @@ -501,7 +501,7 @@ static int update_child_status_internal(
>  copy_request(ws->request, sizeof(ws->request), r);
>  }
>  
> -if (r) {
> +if (r && r->useragent_ip) {
>  if (!(val = ap_get_useragent_host(r, REMOTE_NOLOOKUP, NULL)))
>  apr_cpystrn(ws->client, r->useragent_ip, sizeof(ws->client));

Hm, wouldn't it be better to just encapsulate the above line in an if 
(r->useragent_ip)
or do we already know that ap_get_useragent_host(r, REMOTE_NOLOOKUP, NULL) 
cannot deliver
something meaningful if r->useragent_ip == NULL?

Regards

Rüdiger

Re: svn commit: r1741310 - in /httpd/httpd/trunk: modules/http2/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/ server/mpm/winnt/ server/mpm/worker/

[Rainer Jung]

Am 28.04.2016 um 04:30 schrieb William A Rowe Jr:

On Wed, Apr 27, 2016 at 6:16 PM, Yann Ylavic > wrote:

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,  > wrote:
> Author: wrowe
> Date: Wed Apr 27 18:41:49 2016
> New Revision: 1741310
>
> URL:http://svn.apache.org/viewvc?rev=1741310=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...


Well, we always want to be considerate of performance.

That said, we absolutely must not change the semantic meanings of
the server-status results on the maintenance branch.  Change those
meanings on trunk for a future 2.6/3.0?  Sure... but 2.4.x needs to
behave as 2.4.x has behaved from the start.


I agree we shouldn't change the semantics of R/W during 2.4. People 
might monitor the numbers of R and W and will be quite surprised by 
seeing a noticeable shift from W to R.


I think it would be a good change for 2.6/3.0 to make R including 
reading the request body and only after having read it switch to W. We 
can mention it in CHANGES / migration guide and personally I think those 
semantics would be more intuitive.


Regards,

Rainer

Re: svn commit: r1741310 - in /httpd/httpd/trunk: modules/http2/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/ server/mpm/winnt/ server/mpm/worker/

[William A Rowe Jr]

On Wed, Apr 27, 2016 at 9:30 PM, William A Rowe Jr 
wrote:

> On Wed, Apr 27, 2016 at 6:16 PM, Yann Ylavic  wrote:
>
>>
>> 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).
>>
>
> If that slot does other work, it should be recorded.  But while that slot
> sits unused, the last activity for that score slot must be preserved.
>

Just to clarify the event case... our server is not free-threaded.  Once we
are in a request, we are threadbound.  The following request on that same
worker is likely to jump threads, and it's fine, my brief testing today
seems
to confirm, but I presented the patch to the followers of PR 59333 for
further
review and testing.

This will likely change in 3.0 (no, we aren't going to make requests
free-threaded without bumping the version major - that's too drastic
a behavior change.)  Once a *request* is jumping from thread to
thread, as opposed to the underlying connection, we are going to
have to rethink how the scoreboard presents that information and
how it is refreshed.

Re: svn commit: r1741310 - in /httpd/httpd/trunk: modules/http2/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/ server/mpm/winnt/ server/mpm/worker/

[William A Rowe Jr]

On Wed, Apr 27, 2016 at 6:16 PM, Yann Ylavic  wrote:

> 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,   wrote:
> > Author: wrowe
> > Date: Wed Apr 27 18:41:49 2016
> > New Revision: 1741310
> >
> > URL: http://svn.apache.org/viewvc?rev=1741310=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...
>

Well, we always want to be considerate of performance.

That said, we absolutely must not change the semantic meanings of
the server-status results on the maintenance branch.  Change those
meanings on trunk for a future 2.6/3.0?  Sure... but 2.4.x needs to
behave as 2.4.x has behaved from the start.


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

Given a real conn_rec record, we have no request line.  Therefore the result
is NULL.  There is no purpose in modifying the conn_rec related fields once
correctly recorded.  The next chance they have for being modified in any
meaningful way is during an ssl renegotiation or the processing of the
Host:
and X-F-F: headers during the read_request phase.


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

That was incorrect - to the extend that you've changed it since 2.4.1,
such a change must be reverted.

At the time we process a new connection (before, and again after we
have parsed any SNI host handshake) - there is NO REQUEST.  Any
lingering request value must be blanked out at that time.  Once the
request URI is parsed we again invoke update_child_status, this time
with the request_rec with a now-known request string.


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

You don't understand it.

Envision this sequence, which is how the scoreboard was designed;

Initialization -> entire score record is voided

Connection -> score entry tagged READ, what is known of the
connecting client and the target vhost is recorded

Connection SNI -> score entry updated with new target vhost

Request read -> score entry again updated with new target vhost,
the request identifier is updated, score entry tagged WRITE

Request body is read, Response body is prepared

Request complete -> score entry tagged LOGGING, and NO
other fields need to be updated

Request logged, left in keepalive state -> score entry tagged
KEEPALIVE, NO other fields need to be updated

Connection closed -> score entry tagged IDLE, NO OTHER
FIELDS SHOULD BE UPDATED until the worker is reused
(even in the case of event MPM).




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

That could be updated, or we could equally pass NULL always, but it doesn't
impact the correctness of the proposed backport.


>  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 

Re: svn commit: r1741310 - in /httpd/httpd/trunk: modules/http2/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/ server/mpm/winnt/ server/mpm/worker/

[Yann Ylavic]

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,   wrote:
> Author: wrowe
> Date: Wed Apr 27 18:41:49 2016
> New Revision: 1741310
>
> URL: http://svn.apache.org/viewvc?rev=1741310=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.