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

[William A Rowe Jr]

So just to summarize how in h2 there may be issues...

modules/http2/h2_conn_io.c:ap_update_child_status_from_conn(c->sbh,
SERVER_BUSY_WRITE, c);
  - this will trash any request line info

modules/http2/h2_task.c:ap_update_child_status(c->sbh,
SERVER_BUSY_READ, r);
  - is only appropriate if this is reading the request body... if we are
still in the read_request hook then the request headers and values such as
r->useragent_ip etc aren't set up yet.

modules/http2/h2_request.c:ap_update_child_status(c->sbh,
SERVER_BUSY_LOG, r);
  - good

modules/http2/h2_session.c:
 ap_update_child_status_descr(session->c->sbh, status, session->status);
  - looks fine, provided that session->status isn't "\0"

modules/http2/h2_session.c:
 ap_update_child_status_from_conn(c->sbh, SERVER_BUSY_READ, c);
  - this is the correct logic if we are reading the request headers

modules/http2/h2_session.c:
 ap_update_child_status(session->c->sbh, SERVER_BUSY_READ, NULL);
  - this looks a bit odd, note that the request / descr is not updated here
so hopefully it isn't ready/was updated already

modules/http2/h2_session.c:
 ap_update_child_status(session->c->sbh, SERVER_BUSY_WRITE, NULL);
  - note that the request / descr is not updated here so hopefully it was
updated elsewhere already


On Wed, Apr 27, 2016 at 9:15 AM, William A Rowe Jr 
wrote:

> For example... in event.c;
>
> if (cs->pub.state == CONN_STATE_WRITE_COMPLETION) {
> int not_complete_yet;
>
> ap_update_child_status_from_conn(sbh, SERVER_BUSY_WRITE, c);
>
> Yuck, that just trashed the score for that connection.
>
> We do it again here;
>
> if (cs->pub.state == CONN_STATE_LINGER) {
> start_lingering_close_blocking(cs);
> }
> else if (cs->pub.state == CONN_STATE_CHECK_REQUEST_LINE_READABLE) {
> ap_update_child_status_from_conn(sbh, SERVER_BUSY_KEEPALIVE, c);
>
> I'm sure we can find similar examples, I'm still going through a pretty
> broad grep of the trunk sources.
>
>
>
>
>
> On Wed, Apr 27, 2016 at 8:43 AM, William A Rowe Jr 
> wrote:
>
>> I'm not certain the error is -in- scoreboard.c.
>>
>> Suspecting that we used to call ap_update_child_status[_xxx] with the
>> NULL req/conn rec pointer when we were idling for keepalive or ending the
>> request.
>> It seems we may now be calling it in these situations with a now-NULL
>> req_rec
>> and the dying/idling conn_rec pointer, causing the request value to null
>> out (or
>> calling it with a descr pointer to "\0").
>>
>>
>> On Wed, Apr 27, 2016 at 7:54 AM, Jim Jagielski  wrote:
>>
>>> +1...
>>>
>>> I had just applied the patch in STATUS w/o seeing this thread.
>>> Best to just revert this all and get back to 2.4.18 behavior
>>> (and code)
>>>
>>> > On Apr 27, 2016, at 12:40 AM, Stefan Eissing <
>>> stefan.eiss...@greenbytes.de> wrote:
>>> >
>>> >
>>> >
>>> >> Am 27.04.2016 um 03:53 schrieb William A Rowe Jr >> >:
>>> >>
>>> >>
>>> >> My gut instinct is to propose scoreboard.c for a full svn revert back
>>> >> to a working state,
>>> >
>>> > I did not realize that it is that deep of a mess now. Please revert,
>>> uncomment the new calls use in http2 and I'll find another approach there.
>>> >
>>> > It seems that there are a lot of expectations on how this part of the
>>> server behaves, but no way to verify any changes/additions against them for
>>> a newbie like me.
>>> >
>>> > Since this whole h2 thing is experimental and needs to evolve based on
>>> user feedback, it better either use the extension possibilities of
>>> mod_status or come up with a new, separate approach that allows frequent
>>> changes.
>>> >
>>> > -Stefan
>>>
>>>
>>
>

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

[William A Rowe Jr]

For example... in event.c;

if (cs->pub.state == CONN_STATE_WRITE_COMPLETION) {
int not_complete_yet;

ap_update_child_status_from_conn(sbh, SERVER_BUSY_WRITE, c);

Yuck, that just trashed the score for that connection.

We do it again here;

if (cs->pub.state == CONN_STATE_LINGER) {
start_lingering_close_blocking(cs);
}
else if (cs->pub.state == CONN_STATE_CHECK_REQUEST_LINE_READABLE) {
ap_update_child_status_from_conn(sbh, SERVER_BUSY_KEEPALIVE, c);

I'm sure we can find similar examples, I'm still going through a pretty
broad grep of the trunk sources.





On Wed, Apr 27, 2016 at 8:43 AM, William A Rowe Jr 
wrote:

> I'm not certain the error is -in- scoreboard.c.
>
> Suspecting that we used to call ap_update_child_status[_xxx] with the
> NULL req/conn rec pointer when we were idling for keepalive or ending the
> request.
> It seems we may now be calling it in these situations with a now-NULL
> req_rec
> and the dying/idling conn_rec pointer, causing the request value to null
> out (or
> calling it with a descr pointer to "\0").
>
>
> On Wed, Apr 27, 2016 at 7:54 AM, Jim Jagielski  wrote:
>
>> +1...
>>
>> I had just applied the patch in STATUS w/o seeing this thread.
>> Best to just revert this all and get back to 2.4.18 behavior
>> (and code)
>>
>> > On Apr 27, 2016, at 12:40 AM, Stefan Eissing <
>> stefan.eiss...@greenbytes.de> wrote:
>> >
>> >
>> >
>> >> Am 27.04.2016 um 03:53 schrieb William A Rowe Jr > >:
>> >>
>> >>
>> >> My gut instinct is to propose scoreboard.c for a full svn revert back
>> >> to a working state,
>> >
>> > I did not realize that it is that deep of a mess now. Please revert,
>> uncomment the new calls use in http2 and I'll find another approach there.
>> >
>> > It seems that there are a lot of expectations on how this part of the
>> server behaves, but no way to verify any changes/additions against them for
>> a newbie like me.
>> >
>> > Since this whole h2 thing is experimental and needs to evolve based on
>> user feedback, it better either use the extension possibilities of
>> mod_status or come up with a new, separate approach that allows frequent
>> changes.
>> >
>> > -Stefan
>>
>>
>

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

[William A Rowe Jr]

I'm not certain the error is -in- scoreboard.c.

Suspecting that we used to call ap_update_child_status[_xxx] with the
NULL req/conn rec pointer when we were idling for keepalive or ending the
request.
It seems we may now be calling it in these situations with a now-NULL
req_rec
and the dying/idling conn_rec pointer, causing the request value to null
out (or
calling it with a descr pointer to "\0").


On Wed, Apr 27, 2016 at 7:54 AM, Jim Jagielski  wrote:

> +1...
>
> I had just applied the patch in STATUS w/o seeing this thread.
> Best to just revert this all and get back to 2.4.18 behavior
> (and code)
>
> > On Apr 27, 2016, at 12:40 AM, Stefan Eissing <
> stefan.eiss...@greenbytes.de> wrote:
> >
> >
> >
> >> Am 27.04.2016 um 03:53 schrieb William A Rowe Jr :
> >>
> >>
> >> My gut instinct is to propose scoreboard.c for a full svn revert back
> >> to a working state,
> >
> > I did not realize that it is that deep of a mess now. Please revert,
> uncomment the new calls use in http2 and I'll find another approach there.
> >
> > It seems that there are a lot of expectations on how this part of the
> server behaves, but no way to verify any changes/additions against them for
> a newbie like me.
> >
> > Since this whole h2 thing is experimental and needs to evolve based on
> user feedback, it better either use the extension possibilities of
> mod_status or come up with a new, separate approach that allows frequent
> changes.
> >
> > -Stefan
>
>

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

[Jim Jagielski]

+1...

I had just applied the patch in STATUS w/o seeing this thread.
Best to just revert this all and get back to 2.4.18 behavior
(and code)

> On Apr 27, 2016, at 12:40 AM, Stefan Eissing  
> wrote:
> 
> 
> 
>> Am 27.04.2016 um 03:53 schrieb William A Rowe Jr :
>> 
>> 
>> My gut instinct is to propose scoreboard.c for a full svn revert back
>> to a working state,
> 
> I did not realize that it is that deep of a mess now. Please revert, 
> uncomment the new calls use in http2 and I'll find another approach there.
> 
> It seems that there are a lot of expectations on how this part of the server 
> behaves, but no way to verify any changes/additions against them for a newbie 
> like me.
> 
> Since this whole h2 thing is experimental and needs to evolve based on user 
> feedback, it better either use the extension possibilities of mod_status or 
> come up with a new, separate approach that allows frequent changes.
> 
> -Stefan

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

[Stefan Eissing]

> Am 27.04.2016 um 03:53 schrieb William A Rowe Jr :
> 
> 
> My gut instinct is to propose scoreboard.c for a full svn revert back
> to a working state,

I did not realize that it is that deep of a mess now. Please revert, uncomment 
the new calls use in http2 and I'll find another approach there.

It seems that there are a lot of expectations on how this part of the server 
behaves, but no way to verify any changes/additions against them for a newbie 
like me.

Since this whole h2 thing is experimental and needs to evolve based on user 
feedback, it better either use the extension possibilities of mod_status or 
come up with a new, separate approach that allows frequent changes.

-Stefan

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

[William A Rowe Jr]

On Tue, Apr 26, 2016 at 5:01 PM, Yann Ylavic  wrote:

> On Tue, Apr 26, 2016 at 10:48 PM, William A Rowe Jr 
> wrote:
> > On Tue, Apr 26, 2016 at 3:47 PM, William A Rowe Jr 
> > wrote:
> >>
> >> Reviewing the applicable patch, the client, request and protocol now
> >> appear
> >
> > ... the client, vhost and protocol now appear ...
> >
> >> to persist for idle connections, but the request is still NULL?
> >>
> >> Is this by design? Or is something missing from the patch?
>
> Is it different behaviour than 2.4.18 (it shouldn't)?
>
> We are discussing different issues/solutions in PR 59333...
>

AIUI the previous behavior was to retain the prior request until the
new keep-alive request began processing the post-read-request
phase, until that point the previous request on that pipeline should
still appear in the connection's status.

I'm now already following that thread since this afternoon and have
to compare notes against 2.4.17-.18-.20-post-patch behaviors.

My gut instinct is to propose scoreboard.c for a full svn revert back
to a working state, but will help if folks demonstrate some effort
toward preserving ABI and functional state compatibility... at least
over the next few days.

Cheers,

Bill

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

[Yann Ylavic]

On Tue, Apr 26, 2016 at 10:48 PM, William A Rowe Jr  wrote:
> On Tue, Apr 26, 2016 at 3:47 PM, William A Rowe Jr 
> wrote:
>>
>> Reviewing the applicable patch, the client, request and protocol now
>> appear
>
>
> ... the client, vhost and protocol now appear ...
>
>>
>> to persist for idle connections, but the request is still NULL?
>>
>> Is this by design? Or is something missing from the patch?

Is it different behaviour than 2.4.18 (it shouldn't)?

We are discussing different issues/solutions in PR 59333...

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

[William A Rowe Jr]

On Tue, Apr 26, 2016 at 3:47 PM, William A Rowe Jr 
wrote:

> Reviewing the applicable patch, the client, request and protocol now appear
>

... the client, vhost and protocol now appear ...


> to persist for idle connections, but the request is still NULL?
>
> Is this by design? Or is something missing from the patch?
>
> On Tue, Apr 19, 2016 at 4:19 AM,  wrote:
>
>> Author: rpluem
>> Date: Tue Apr 19 09:19:44 2016
>> New Revision: 1739876
>>
>> URL: http://svn.apache.org/viewvc?rev=1739876=rev
>> Log:
>> * Vote
>>
>> 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=1739876=1739875=1739876=diff
>>
>> ==
>> --- httpd/httpd/branches/2.4.x/STATUS (original)
>> +++ httpd/httpd/branches/2.4.x/STATUS Tue Apr 19 09:19:44 2016
>> @@ -191,7 +191,7 @@ PATCHES PROPOSED TO BACKPORT FROM TRUNK:
>>   trunk patch: http://svn.apache.org/r1739193
>>   2.4.x patch: trunk works (modulo CHANGES) or
>>
>> http://home.apache.org/~ylavic/patches/httpd-2.4.x-scoreboard_preserve-v2.patch
>> - +1: ylavic
>> + +1: ylavic, rpluem
>>   ylavic: diff with 2.4.18 after this merge:
>>
>> http://home.apache.org/~ylavic/patches/scoreboard-2.4.18.diff
>>
>>
>>
>>
>

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

[William A Rowe Jr]

Reviewing the applicable patch, the client, request and protocol now appear
to persist for idle connections, but the request is still NULL?

Is this by design? Or is something missing from the patch?

On Tue, Apr 19, 2016 at 4:19 AM,  wrote:

> Author: rpluem
> Date: Tue Apr 19 09:19:44 2016
> New Revision: 1739876
>
> URL: http://svn.apache.org/viewvc?rev=1739876=rev
> Log:
> * Vote
>
> 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=1739876=1739875=1739876=diff
>
> ==
> --- httpd/httpd/branches/2.4.x/STATUS (original)
> +++ httpd/httpd/branches/2.4.x/STATUS Tue Apr 19 09:19:44 2016
> @@ -191,7 +191,7 @@ PATCHES PROPOSED TO BACKPORT FROM TRUNK:
>   trunk patch: http://svn.apache.org/r1739193
>   2.4.x patch: trunk works (modulo CHANGES) or
>
> http://home.apache.org/~ylavic/patches/httpd-2.4.x-scoreboard_preserve-v2.patch
> - +1: ylavic
> + +1: ylavic, rpluem
>   ylavic: diff with 2.4.18 after this merge:
>
> http://home.apache.org/~ylavic/patches/scoreboard-2.4.18.diff
>
>
>
>