httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Yann Ylavic <ylavic....@gmail.com>
Subject 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/
Date Thu, 28 Apr 2016 09:37:56 GMT
On Thu, Apr 28, 2016 at 4:30 AM, William A Rowe Jr <wrowe@rowe-clan.net> wrote:
> On Wed, Apr 27, 2016 at 6:16 PM, Yann Ylavic <ylavic.dev@gmail.com> wrote:
>>
>> On Wed, Apr 27, 2016 at 8:41 PM,  <wrowe@apache.org> 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 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 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.

Or even do not update the status at all if !r->the_request (and/or
!r->the_request[0]?) since it will pass to CLOSING very soon.

Mime
View raw message