Hi Lukas,
On Wed, Nov 22, 2017 at 01:43:32AM +0100, Lukas Tribus wrote:
> Since af1e4f5167 ("MEDIUM: h2: perform a graceful shutdown on "Connection:
> close"") we send GOAWAY with last stream identifier set to 2^31-1, as per
> the RFC suggestion [1]. However that is only part of what the RFC suggests
> if we want to close the connection gracefully; after at least 1 RTT we
> would have to send another GOAWAY with a proper and valid last stream ID.
> Just the "2^31-1" GOAWAY is not enough.
>
> In fact this confuses Chrome and leads to a hung connection that clears
> only by "timeout client" or "timeout server" (whatever strikes first).
This is annoying. I'm still wondering whether it's a good thing to try
to close the connection when we have a close. I've been wanting to ensure
we can close a connection so that all users having "http-request deny"
and similar rules can continue to use them to fight attacks, but a graceful
shutdown doesn't appear 100% efficient either :-/
Also I'd like to change all the error messages to support keep-alive (at
least in the announce) so that we don't close on 401 etc. But once we
stop closing on any such codes, we can wonder whether it still makes
sense to close when "connection: close" appears in the response :-/
Maybe we should have a "close" action on the request to explicitly close
a connection, regardless of headers. We already have such an action on
tcp-response to close the server connection. It can make sense.
> When all 3 connections slots to the HTTP2 server are filled, Chrome is
> unable to communicate with the server at all.
Then at least we're not the only ones to have a bug in this chain :-)
> Trivial fix (to be discussed) is to not set the last stream id to 2^31-1.
We must really not do this, that's what we used to do before the patch
you mentionned above. It invites the browser to *safely* retry all
streams with a higher ID than the one mentionned there, but in practice
some clients are not able to retry so they report errors as their pending
requests are simply lost.
I suspect we could act differently : when the last stream has been closed,
we currently re-arm a timer on the H2 connection to close it if it's idle.
What we could do would be to check if we've already sent a graceful shutdown
and in this case arm a much shorter timer (eg: 1s) so that the final GOAWAY
is sent and the connection closed. Could you please try the attached patch,
which tries to do this ?
With that said, there is an issue with Chrome if it refrains from using
these connections an refrains from creating a new one as well, because
this means that it can be limited to a request rate of only 3 per RTT
if each connection handles a single request.
Thanks,
Willy
diff --git a/src/mux_h2.c b/src/mux_h2.c
index eb8dd0e..4d71c95 100644
--- a/src/mux_h2.c
+++ b/src/mux_h2.c
@@ -2134,7 +2134,7 @@ static int h2_wake(struct connection *conn)
if (h2c->task) {
if (eb_is_empty(&h2c->streams_by_id)) {
- h2c->task->expire = tick_add(now_ms, h2c->timeout);
+ h2c->task->expire = tick_add(now_ms, h2c->last_sid < 0
? h2c->timeout : MS_TO_TICKS(1000));
task_queue(h2c->task);
}
else
@@ -2166,6 +2166,7 @@ static struct task *h2_timeout_task(struct task *t)
}
/* try to send but no need to insist */
+ h2c->last_sid = h2c->max_id;
if (h2c_send_goaway_error(h2c, NULL) <= 0)
h2c->flags |= H2_CF_GOAWAY_FAILED;
@@ -2298,7 +2299,7 @@ static void h2_detach(struct conn_stream *cs)
}
else if (h2c->task) {
if (eb_is_empty(&h2c->streams_by_id)) {
- h2c->task->expire = tick_add(now_ms,
h2c->timeout);
+ h2c->task->expire = tick_add(now_ms,
h2c->last_sid < 0 ? h2c->timeout : MS_TO_TICKS(1000));
task_queue(h2c->task);
}
else
