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. <wmr...@gmail.com>:
>
> 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, &bblen);
> 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.
