On Fri, Dec 22, 2017 at 6:14 PM, Stefan Eissing
<stefan.eiss...@greenbytes.de> 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;

Reply via email to