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: TIL

[Yann Ylavic]

On Thu, Apr 28, 2016 at 2:40 AM, Yann Ylavic  wrote:
>>
>> What I saw was memory nullified, by assumedly, new apr_pcallocs, that should 
>> not have been freed already. With more likelihood the more load and the less 
>> logging, of course.
>
> So I'm rather thinking of a pool lifetime issue in h2 buckets
> handling, did you see the comment in ap_bucket_eor_create(), the case
> does not seem to be handled in h2_beam_bucket_make() (if that's the
> bucket you are talking about).

Hmm, the beam bucket is DATA (not META), so the issue would instead be
in h2_bucket_{eos,eoc}_destroy(), since h2_bucket_{eos,eoc}_make()
don't seem to take care of the h2_{stream,session} being destroyed
outside the bucket handling...
Is that possible?

Re: TIL

[Yann Ylavic]

On Wed, Apr 27, 2016 at 7:28 AM, Stefan Eissing
 wrote:
>
>
>> Am 26.04.2016 um 23:57 schrieb Yann Ylavic :
>>
>> On Tue, Apr 26, 2016 at 4:49 PM, Stefan Eissing
>>  wrote:
>>> Today I Learned the difference between writing
>>>  DATA + META
>>> and
>>>  DATA + META + FLUSH
>>> to the core output filters. I am astonished that
>>> my code ever worked.
>>>
>>> Seriously, what is the reason for this kind of
>>> implementation?
>>
>> Do you mean why META could be destroyed before (setaside-)DATA, in
>> remove_empty_buckets()?
>
> Yes. That was unexpected. My understanding of the pass_brigade contract was 
> that buckets get destroyed in order of appearance (might be after the call).

Actually after re-reading the code, the core output filter looks good to me.
remove_empty_buckets() will only remove buckets up to DATA buckets,
and indeed httpd couldn't work otherwise.

>
> The h2 use case is to pass a meta bucket that, when destroyed, signals the 
> end of life for data belonging to one stream. So all buckets before that one 
> should have been either destroyed or setaside already.

That's looks very similar to how EOR works.

>
> With http/1.x and EOR, my current reading of the code, this does not happen 
> since EORs are always FLUSHed.

No, EOR is never flushed explicitly (sent alone in the brigade by
ap_process_request_after_handler), the flush occurs if/when pipelining
stops (or MAX_REQUESTS_IN_PIPELINE is reached).
While requests are pipelined, so are responses (the usual case is not
pipelining though, so EOR may look flushed, but not always...).

> And even if, releasing the request pool early will probably go unnoticed as 
> WRITE_COMPLETION will read all memory *before* a new pool is opened on the 
> conn pool.

I debugged pipeling a few time ago and would have noticed it, accurate
ap_run_log_transaction() depends on this too.
This is not something that crashes httpd at least :)

>
> This (again, if my reading is correct) does no longer hold in h2. request 
> (stream) starting and ending is happening all the time, conn child pools are 
> created/destroyed all the time.
>
> What I saw was memory nullified, by assumedly, new apr_pcallocs, that should 
> not have been freed already. With more likelihood the more load and the less 
> logging, of course.

So I'm rather thinking of a pool lifetime issue in h2 buckets
handling, did you see the comment in ap_bucket_eor_create(), the case
does not seem to be handled in h2_beam_bucket_make() (if that's the
bucket you are talking about).
I'm not familiar with your recent Beam changes, and don't know what
lifetime the bucket is supposed to have, but couldn't your issue be
that beam_bucket_destroy() is called too late?

Regards,
Yann.

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.

h2 request processing score behavior...

[William A. Rowe Jr.]

Within h2_task.c, we have

static apr_status_t h2_task_process_request(h2_task *task, conn_rec *c)
{
const h2_request *req = task->request;
conn_state_t *cs = c->cs;
request_rec *r;

ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, c,
  "h2_task(%s): create request_rec", task->id);
r = h2_request_create_rec(req, c);
if (r && (r->status == HTTP_OK)) {
ap_update_child_status(c->sbh, SERVER_BUSY_READ, r);

if (cs) {
cs->state = CONN_STATE_HANDLER;
}
ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, c,
  "h2_task(%s): start process_request", task->id);
ap_process_request(r);

Contrast this to http_core.c...

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;

/* To preserve legacy behaviour, use the keepalive timeout from the
 * base server (first on this IP:port) when none is explicitly
 * configured on this server.
 */
if (!r->server->keep_alive_timeout_set) {
keep_alive_timeout = c->base_server->keep_alive_timeout;
}

c->keepalive = AP_CONN_UNKNOWN;
/* process the request if it was read without error */

ap_update_child_status(c->sbh, SERVER_BUSY_WRITE, r);
if (r->status == HTTP_OK) {
if (cs)
cs->state = CONN_STATE_HANDLER;
ap_process_request(r);

It certainly seems this slave worker needs to be in
SERVER_BUSY_WRITE at the time we run the
process_request logic to correspond to the expected
behavior of status analysis tools, so will update this
logic accordingly.

The next issue I do not understand, this seems broken
whether it is happening in the master or slave worker...

static apr_status_t pass_out(apr_bucket_brigade *bb, void *ctx)
{
pass_out_ctx *pctx = ctx;
conn_rec *c = pctx->c;
apr_status_t status;
apr_off_t bblen;

if (APR_BRIGADE_EMPTY(bb)) {
return APR_SUCCESS;
}

ap_update_child_status_from_conn(c->sbh, SERVER_BUSY_WRITE, c);
apr_brigade_length(bb, 0, );
h2_conn_io_bb_log(c, 0, APLOG_TRACE2, "master conn pass", bb);
status = ap_pass_brigade(c->output_filters, bb);

This does whack the request line so I've corrected the logic
accordingly by not passing the conn_rec, but if this is only
invoked in the slave worker, correcting the first issue above
seems to make this redundant (and offers a small optimization
to simply remove it.)

The only other issue I see is in h2_session.c...

ap_update_child_status_from_conn(c->sbh, SERVER_BUSY_READ,
c);
if (!h2_is_acceptable_connection(c, 1)) {
update_child_status(session, SERVER_BUSY_READ,
"inadequate security");
h2_session_shutdown(session,
NGHTTP2_INADEQUATE_SECURITY, NULL, 1);
}
else {
update_child_status(session, SERVER_BUSY_READ, "init");

Can h2_is_acceptable_connection update the connection's
virtual host?  If it does, we want to repeat the _from_conn
status refresh to correct the vhost before setting the status
to "init".  I have not touched this code.

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: TIL

[Stefan Eissing]

Change was done in http://svn.apache.org/viewvc?view=revision=327872 
by brianp in 2005 with message: 

"New version of ap_core_output_filter that does nonblocking writes
(backport from async-dev branch to 2.3 trunk)"

Since Brian put his name in CHANGES, it was probably him...

-Stefan


> Am 27.04.2016 um 14:52 schrieb Graham Leggett :
> 
> On 27 Apr 2016, at 2:49 PM, Stefan Eissing  
> wrote:
> 
>> I had a look into 2.4.x this morning and while there are changes in filter 
>> brigade setaside code, the basic "cleanup" of empty and meta buckets before 
>> the setaside is there already.
>> 
>> I think this has not bween noticed before as 
>> 1. EOR is always followed by a FLUSH
>> 2. most bucket types survive the destruction of r->pool quite well, even 
>> pool buckets silently morph
>> 3. even if transient buckets would reference pool memory, settings those 
>> aside after destruction of r->pool, but before filter return would access 
>> freed, but not re-used memory from the connection allocator - I think.
>> 
>> So far, I have seen no reasons why meta buckets should not just be setaside 
>> in core filter. 0 length data buckets should also stay, IMHO, just ignored 
>> in the actual write.
>> 
>> I can only guess the original intent: 0-length data buckets sometimes happen 
>> during some brigade read modes and if there are several FLUSH buckets in the 
>> brigade, it makes sense to get rid of them also. But I think the space 
>> savings are minimal.
>> 
>> But there could be reasons for the current behavior, I overlooked. So, I am 
>> asking around.
> 
> I don’t see any obvious reason for the current behaviour either - is it 
> possible to go back in history and confirm the log entry when it was 
> introduced?
> 
> Regards,
> Graham
> —
> 

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: TIL

[Graham Leggett]

On 27 Apr 2016, at 2:49 PM, Stefan Eissing  wrote:

> I had a look into 2.4.x this morning and while there are changes in filter 
> brigade setaside code, the basic "cleanup" of empty and meta buckets before 
> the setaside is there already.
> 
> I think this has not bween noticed before as 
> 1. EOR is always followed by a FLUSH
> 2. most bucket types survive the destruction of r->pool quite well, even pool 
> buckets silently morph
> 3. even if transient buckets would reference pool memory, settings those 
> aside after destruction of r->pool, but before filter return would access 
> freed, but not re-used memory from the connection allocator - I think.
> 
> So far, I have seen no reasons why meta buckets should not just be setaside 
> in core filter. 0 length data buckets should also stay, IMHO, just ignored in 
> the actual write.
> 
> I can only guess the original intent: 0-length data buckets sometimes happen 
> during some brigade read modes and if there are several FLUSH buckets in the 
> brigade, it makes sense to get rid of them also. But I think the space 
> savings are minimal.
> 
> But there could be reasons for the current behavior, I overlooked. So, I am 
> asking around.

I don’t see any obvious reason for the current behaviour either - is it 
possible to go back in history and confirm the log entry when it was introduced?

Regards,
Graham
—

Re: TIL

[Stefan Eissing]

> Am 27.04.2016 um 14:10 schrieb Jim Jagielski :
> 
> Grrr... w/o looking too deeply into this, this seems very
> wrong. Is that a long-standing bug or something recently
> "optimized" away?

I had a look into 2.4.x this morning and while there are changes in filter 
brigade setaside code, the basic "cleanup" of empty and meta buckets before the 
setaside is there already.

I think this has not bween noticed before as 
1. EOR is always followed by a FLUSH
2. most bucket types survive the destruction of r->pool quite well, even pool 
buckets silently morph
3. even if transient buckets would reference pool memory, settings those aside 
after destruction of r->pool, but before filter return would access freed, but 
not re-used memory from the connection allocator - I think.

So far, I have seen no reasons why meta buckets should not just be setaside in 
core filter. 0 length data buckets should also stay, IMHO, just ignored in the 
actual write.

I can only guess the original intent: 0-length data buckets sometimes happen 
during some brigade read modes and if there are several FLUSH buckets in the 
brigade, it makes sense to get rid of them also. But I think the space savings 
are minimal.

But there could be reasons for the current behavior, I overlooked. So, I am 
asking around.

-Stefan

> 
>> On Apr 26, 2016, at 10:49 AM, Stefan Eissing  
>> wrote:
>> 
>> Today I Learned the difference between writing 
>> DATA + META 
>> and 
>> DATA + META + FLUSH
>> to the core output filters. I am astonished that
>> my code ever worked.
>> 
>> Seriously, what is the reason for this kind of
>> implementation? I would like to pass META buckets
>> in non-blocking way and *not* lose the order of 
>> bucket destruction. Any explanation or advice is
>> appreciated!
>> 
>> Cheers,
>> 
>> Stefan
> 

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

[William A Rowe Jr]

On Wed, Apr 27, 2016 at 7:20 AM,  wrote:

> Author: jim
> Date: Wed Apr 27 12:20:57 2016
> New Revision: 1741239
>
> URL: http://svn.apache.org/viewvc?rev=1741239=rev
>
> 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=1741239=1741238=1741239=diff
>
> ==
> --- httpd/httpd/branches/2.4.x/STATUS (original)
> +++ httpd/httpd/branches/2.4.x/STATUS Wed Apr 27 12:20:57 2016
> @@ -114,6 +114,18 @@ RELEASE SHOWSTOPPERS:
>  PATCHES ACCEPTED TO BACKPORT FROM TRUNK:
>[ start all new proposals below, under PATCHES PROPOSED. ]
>
> +  *) scoreboard/status: Keep previous worker connection/request data when
> idle as
> + prior to 2.4.20.
> + trunk patch: http://svn.apache.org/r1739008
> + trunk patch: http://svn.apache.org/r1739146
> + trunk patch: http://svn.apache.org/r1739151
> + 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, rpluem, jim
> + ylavic: diff with 2.4.18 after this merge:
> +
> http://home.apache.org/~ylavic/patches/scoreboard-2.4.18.diff


Hi Jim, see https://bz.apache.org/bugzilla/show_bug.cgi?id=59333

This doesn't seem to be 'fixed' yet.  I don't mind incremental patches
moving forward, but we still appear to be zeroing out the request element
during keep-alive activity, which is the usual precursor to closing the
connection.

Request for eyes and tests

[Jim Jagielski]

In the 2.4 STATUS file there are 2 backports which, I think,
would be exceptional if we could get in 2.4 asap. They are
the dynamic health check and the http/2 proxy stuff.

These are some killer features and would be great to have
httpd 2.4 be the web server/reverse proxy that brings 'em
to the people.

Re: TIL

[Jim Jagielski]

Grrr... w/o looking too deeply into this, this seems very
wrong. Is that a long-standing bug or something recently
"optimized" away?

> On Apr 26, 2016, at 10:49 AM, Stefan Eissing  
> wrote:
> 
> Today I Learned the difference between writing 
>  DATA + META 
> and 
>  DATA + META + FLUSH
> to the core output filters. I am astonished that
> my code ever worked.
> 
> Seriously, what is the reason for this kind of
> implementation? I would like to pass META buckets
> in non-blocking way and *not* lose the order of 
> bucket destruction. Any explanation or advice is
> appreciated!
> 
> Cheers,
> 
> Stefan

Re: TIL

[Graham Leggett]

On 26 Apr 2016, at 4:49 PM, Stefan Eissing  wrote:

> Today I Learned the difference between writing 
>  DATA + META 
> and 
>  DATA + META + FLUSH
> to the core output filters. I am astonished that
> my code ever worked.
> 
> Seriously, what is the reason for this kind of
> implementation? I would like to pass META buckets
> in non-blocking way and *not* lose the order of 
> bucket destruction. Any explanation or advice is
> appreciated!

Definitely looks like a bug - if we’re deleting buckets out of order things 
will definitely break. I say we must fix the core.

Regards,
Graham
—

Re: Recent updates to httpd-trunk

[NormW]

On 27/04/2016 8:56 AM, Yann Ylavic wrote:

Thanks, done in r1741112 and r1741115.

Regards,
Yann.


Excellent! All httpd-trunk now builds nicely with NetWare.

Re: TIL

[Niklas Edmundsson]

On Wed, 27 Apr 2016, Stefan Eissing wrote:



The h2 use case is to pass a meta bucket that, when destroyed, 
signals the end of life for data belonging to one stream. So all 
buckets before that one should have been either destroyed or 
setaside already.


One workaround could be a custom (data) bucket type, and pass a 0 
length bucket of it on the brigade where appropriate...


However, I don't know if the remove-empty-buckets logic here and there 
wrecks this method as well?



/Nikke
--
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
 Niklas Edmundsson, Admin @ {acc,hpc2n}.umu.se  | ni...@acc.umu.se
---
 The paper is always strongest at the perforations.
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=