On Fri, Dec 22, 2017 at 6:14 PM, Stefan Eissing
<[email protected]> wrote:
>
> The changes in h2_h2.c and h2_switch.c work find in my test. However, the
> change in h2_conn.c which sets CONN_STATE_LINGER does not. It prevents the
> last GOAWAY frame from the server to be sent out, in some tests.
>
> The current flow, when the client sends a GOAWAY is that the session
> transits to H2_SESSION_ST_DONE, the mpm calls the pre_linger hook and
> that writes the server's GOAWAY (as it does in situations where the connection
> is shut down after an idle timeout by the mpm itself).
Argh, forgot about the pre_close_connection hook..
>
> mod_http2 also returns from connection procession when the connection
> can only progress when more frames from the client arrive. For example,
> when the session is H2_SESSION_ST_DONE, which means that all streams have
> been processed. This is analog to a request being done on a h1 connection.
>
> But the h2session and all its state is still alive. So c->aborted should
> not be set. And it is definitely not in LINGER.
Yes, the issue is probably not about CONN_STATE_LINGER, but c->aborted.
I wanted to close the connection as quickly as possible, that was not
a good idea.
I could replace ap_flush_conn() by ap_lingering_close() in my patch,
but the simpler is to let the MPM do it, v2 attached simply sets
CONN_STATE_LINGER (it may be turned to CONN_STATE_WRITE_COMPLETION in
the Upgrade case, still the MPM will do the right thing with
c->keepalive = AP_CONN_CLOSE).
My main concern was returning anything else than
CONN_STATE_WRITE_COMPLETION or CONN_STATE_LINGER in h2_conn_run(),
like CONN_STATE_HANDLER.
>
> I am all for aligning h2 connection states more with the connection state
> model that mpm_event has (and vice versa). Maybe we have to lock ourselves
> into a room with a whiteboard and beers and figure this out one day...
Would be a pleasure :)
>
> Do we have good documentation about the CONN_STATE engine somewhere?
I updated some comments in event.c lately...
Index: modules/http2/h2_conn.c
===================================================================
--- modules/http2/h2_conn.c (revision 1819027)
+++ modules/http2/h2_conn.c (working copy)
@@ -253,25 +253,12 @@ apr_status_t h2_conn_run(struct h2_ctx *ctx, conn_
} while (!async_mpm
&& c->keepalive == AP_CONN_KEEPALIVE
&& mpm_state != AP_MPMQ_STOPPING);
-
+
if (c->cs) {
- switch (session->state) {
- case H2_SESSION_ST_INIT:
- case H2_SESSION_ST_CLEANUP:
- case H2_SESSION_ST_DONE:
- case H2_SESSION_ST_IDLE:
- c->cs->state = CONN_STATE_WRITE_COMPLETION;
- break;
- case H2_SESSION_ST_BUSY:
- case H2_SESSION_ST_WAIT:
- default:
- c->cs->state = CONN_STATE_HANDLER;
- break;
-
- }
+ c->cs->state = CONN_STATE_LINGER;
}
-
- return DONE;
+
+ return APR_SUCCESS;
}
apr_status_t h2_conn_pre_close(struct h2_ctx *ctx, conn_rec *c)
Index: modules/http2/h2_h2.c
===================================================================
--- modules/http2/h2_h2.c (revision 1819027)
+++ modules/http2/h2_h2.c (working copy)
@@ -669,10 +669,11 @@ int h2_h2_process_conn(conn_rec* c)
ap_log_cerror(APLOG_MARK, APLOG_TRACE1, status, c, "conn_setup");
if (status != APR_SUCCESS) {
h2_ctx_clear(c);
- return status;
+ return !OK;
}
}
- return h2_conn_run(ctx, c);
+ h2_conn_run(ctx, c);
+ return OK;
}
ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, c, "h2_h2, declined");
Index: modules/http2/h2_switch.c
===================================================================
--- modules/http2/h2_switch.c (revision 1819027)
+++ modules/http2/h2_switch.c (working copy)
@@ -185,13 +185,12 @@ static int h2_protocol_switch(conn_rec *c, request
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, status, r, APLOGNO(03088)
"session setup");
h2_ctx_clear(c);
- return status;
+ return !OK;
}
h2_conn_run(ctx, c);
- return DONE;
}
- return DONE;
+ return OK;
}
return DECLINED;
