add timeout for closing connections at the graceful shutdown #1108
Conversation
On graceful shutdown, after the second GOAWAY message has been written, it should be possible for use to safely close the connection, without necessarily waiting for all streams to be processed.
|
Both the current behavior and what this PR suggests are within what the RFC permits, i think. Maybe we need a config knob? One downside of the current behavior is that a never ending download might pin an H2O instance forever. Having the connection close makes server stops more predictable in terms of timing. |
|
Thank you for the PR. Even after the second GOAWAY, some responses might still be in-flight (the specification discuss the possibility of losing requests, not responses). In such case, it would be desirable to postpone closing the connection until those responses finish. Otherwise, the clients would be left with partial responses, and since they are partial, they would never get retried. So if this is an issue, I would suggest adding a timer that starts after sending the second GOAWAY and wait until then, or, add a function to server-starter (assuming that you use it to control H2O) that sends SIGKILL when the server fails to stop after some time after SIGTERM has been delivered. |
This gives extra time to clients after we've sent the second GOAWAY frame.
78f7a6b to
00126d7
Compare
|
@deweerdt Thank you for the update!
Sounds reasonable. The PR looks good to me; the only thing I'm a bit worried is that just waiting for one second might not be enough (for example for servers serving large files). Could we make it a configuration variable? |
configure how long H2O should wait for connections to close when doing a graceful shutdown
|
(just in case you missed) CI seems to be failing due to t/00unit.libuv.t receiving a SIGSEGV: |
in libuv because the lib would still reference a free'd context.
| for (node = ctx->http2._conns.next; node != &ctx->http2._conns; node = next) { | ||
| h2o_http2_conn_t *conn = H2O_STRUCT_FROM_MEMBER(h2o_http2_conn_t, _conns, node); | ||
| next = node->next; | ||
| if (conn->state < H2O_HTTP2_CONN_STATE_IS_CLOSING) { |
There was a problem hiding this comment.
This if would mean that if there is a pending write, then closing the socket (and therefore the exit of the server) will be deferred until the write completes.
Is this what we intend, or should we better close a socket with a pending write immediately as well?
There was a problem hiding this comment.
That was indeed my intention, but my assumption was that this would happen relatively fast (next scheduler round), could it be delayed much longer?
There was a problem hiding this comment.
If there is a pending write and the client never sends ACKs, then the write would stall for fairly long time without the write completion callback getting called. Eventually the idle timeout will kick in, but until then, I believe that the connection would be kept open.
There was a problem hiding this comment.
Ah, I see. Fixing this. Thanks!
de7aaf9 to
81cd064
Compare
81cd064 to
d5198ac
Compare
|
Thank you for the changes. Merged to master. |
On graceful shutdown, after the second GOAWAY message has been written,
it should be possible for use to safely close the connection, without
necessarily waiting for all streams to be processed.