process existing streams after goaway#1556
Conversation
…RITY frames never increments max_open
i110
changed the title
| @@ -681,8 +691,7 @@ static int handle_headers_frame(h2o_http2_conn_t *conn, h2o_http2_frame_t *frame | |||
| if ((frame->flags & H2O_HTTP2_FRAME_FLAG_END_HEADERS) == 0) | |||
| goto PREPARE_FOR_CONTINUATION; | |||
| return handle_trailing_headers(conn, stream, payload.headers, payload.headers_len, err_desc); | |||
| } else if (!stream || stream->state != H2O_HTTP2_STREAM_STATE_IDLE) { | |||
There was a problem hiding this comment.
Why isn't this needed any more? This check was added so that we don't send an error back if the stream was opened by a PRIORITY frame and thus is in the IDLE state.
There was a problem hiding this comment.
@deweerdt
I think You mean #1043, but after that change, max_open came to be not increased by PRIORITY frame (#1136, #1138). By that fact, a stream at here must have the id which is lesser or equal than max_open even if it received prior PRIORITY frames, so I think this condition is no longer needed. Please correct me if i'm wrong.
lib/http2/connection.c
Outdated
| @@ -405,6 +411,7 @@ static int handle_incoming_request(h2o_http2_conn_t *conn, h2o_http2_stream_t *s | |||
| { | |||
| int ret, header_exists_map; | |||
|
|
|||
| assert(conn->state != H2O_HTTP2_CONN_STATE_IS_CLOSING); | |||
There was a problem hiding this comment.
Do we need this assertion? I believe that we guarantee that in parse_input.
lib/http2/connection.c
Outdated
| @@ -471,16 +478,16 @@ static ssize_t expect_continuation_of_headers(h2o_http2_conn_t *conn, const uint | |||
| h2o_http2_stream_t *stream; | |||
| int hret; | |||
|
|
|||
| if (conn->state == H2O_HTTP2_CONN_STATE_IS_CLOSING) | |||
| return 0; | |||
|
|
|||
lib/http2/connection.c
Outdated
| @@ -628,12 +635,12 @@ static int handle_data_frame(h2o_http2_conn_t *conn, h2o_http2_frame_t *frame, c | |||
| h2o_http2_stream_t *stream; | |||
| int ret; | |||
|
|
|||
| if (conn->state == H2O_HTTP2_CONN_STATE_IS_CLOSING) | |||
| return 0; | |||
|
|
|||
lib/http2/connection.c
Outdated
| @@ -662,6 +669,9 @@ static int handle_headers_frame(h2o_http2_conn_t *conn, h2o_http2_frame_t *frame | |||
| h2o_http2_stream_t *stream; | |||
| int ret; | |||
|
|
|||
| if (conn->state == H2O_HTTP2_CONN_STATE_IS_CLOSING) | |||
| return 0; | |||
|
|
|||
| @@ -1170,7 +1176,7 @@ void do_emit_writereq(h2o_http2_conn_t *conn) | |||
| case H2O_HTTP2_CONN_STATE_OPEN: | |||
| break; | |||
| case H2O_HTTP2_CONN_STATE_HALF_CLOSED: | |||
| if (conn->num_streams.pull.half_closed + conn->num_streams.push.half_closed != 0) | |||
| if (conn->num_streams.pull.open + conn->num_streams.push.open != 0) | |||
There was a problem hiding this comment.
Could you please explain the intent of the change? My understanding is that the connection needs to kept open until all the streams deliver their response to the client.
There was a problem hiding this comment.
Yes but we also should keep the connections which doesn't start responses yet. The lifetime of num_streams.*.open includes conn->num_streams.*.half_closed.
https://github.com/h2o/h2o/blob/master/include/h2o/http2_internal.h#L432
https://github.com/h2o/h2o/blob/master/include/h2o/http2_internal.h#L445
There was a problem hiding this comment.
Thank you for the clarification. I understand now.
lib/http2/connection.c
Outdated
| @@ -1353,6 +1359,9 @@ static void push_path(h2o_req_t *src_req, const char *abspath, size_t abspath_le | |||
| h2o_http2_conn_t *conn = (void *)src_req->conn; | |||
| h2o_http2_stream_t *src_stream = H2O_STRUCT_FROM_MEMBER(h2o_http2_stream_t, req, src_req); | |||
|
|
|||
| if (conn->state == H2O_HTTP2_CONN_STATE_IS_CLOSING) | |||
| return; | |||
|
|
|||
There was a problem hiding this comment.
Generally speaking, do we do these checks in the code that sends data?
Assuming that we do not, I think we should omit making such checks unless it's required for other reasons.
|
@kazuho I addressed the issues other than the one I replied. |
|
The code looks good to me. Please feel free to merge the PR once the CI succeeds. |
Under current master, when graceful shutdown is requested, h2o possibly stops processing existing streams (i.e. ignore HEADERS and DATA frames) while second GOAWAY has been sent and waiting the clients to close the connections (or http2-graceful-shutdown-timeout is invoked if set). This PR fixes that issue.