You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by "William A. Rowe Jr." <wm...@gmail.com> on 2016/04/27 20:25:59 UTC
h2 request processing score behavior...
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, &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.)
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: h2 request processing score behavior...
Posted by Stefan Eissing <st...@greenbytes.de>.
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. <wm...@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.