Re: h2 request processing score behavior...

[Stefan Eissing]

Thanks a lot for going over this. Additionally to your changes, I also reduced
the overall connection update frequency, so that last request information stays
visible longer and scoreboard updates are reduced.

> Am 27.04.2016 um 20:25 schrieb 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.

Thanks!

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

It's called only on master.

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

It does not change the vhost.

Thanks for fixing this up.

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.