Hi Lukas!
On Wed, Nov 22, 2017 at 11:12:04AM +0100, Lukas Tribus wrote:
> > 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.
>
> That makes absolutely sense. In fact, with a couple of 401 exchanges
> between the browser and the client and when closing the connection
> each time, HTTP2 seems very inefficient. I'm all for allowing
> keep-alive with "normal" HTTP errors.
>
> > 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 :-/
>
> Yes, I'd say lets eliminate both adding the close header on http
> errors and parsing the header for a connection close/GOAWAY, unless
> there are good reason to keep those behaviors.
>
> > 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.
>
> Yes, I'd like that. Keep-alive with normal http headers and have
> config actions available for DoS mitigation, etc.
So in the end here's what I've done :
- implemented a new "reject" HTTP action. I initially started with
"close" and while documenting it I noticed it does exactly the same
as the tcp-request "reject" action, and we already have a different
http-response "close" action which enforces a close without killing
the request so I preferred to stay consistent and switch to "reject".
- remove the special case on the "connection: close" header in the H2
mux. Now I can authenticate using 401 over the same connection.
- figured I didn't need to remove "connection: close" from our default
responses anymore so I left them intact (we can't serve them using
keep-alive on H1 anyway)
- made use of "timeout client-fin" for H2 connections on which we've
already sent a GOAWAY frame, as it's very similar to the closing
shutdown(SHUT_WR) on H1.
So with all this I think we're good.
> >> 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.
>
> If we are talking about Chrome, this was fixed in Chrome 60 (see [1] and [2]).
Great!
> This was done as nginx limits the number of request to 1000 by default
> and then sends GOAWAY [3]. I believe nginx does not set last stream id
> to 2^31-1 [4].
I don't know if it does, though it's a special case in the RFC and I noticed
one agent (I don't remember which one) which was unhappy with GOAWAY sent for
a stream ID it didn't send which was not 2^31-1. That was the reason for using
the specified ID.
> Maybe combine this fix with your 2 suggestions above?
> - don't add Connection: close to http errors
> - don't close the connection/send GOAWAY when detecting Connection: close
The latter was what it used to do :-) But I've never been very sure that it
was a good idea. Also the "Connection: close" in HTTP errors is just one
aspect of the problem, but we can have them in error pages or even in Lua.
> > 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.
>
> Well Chrome can certainly not open new streams on a connection where
> it already received GOAWAY with last stream id 2^31-1, as there are no
> id's left to use for Chrome. Its should close the connection though,
> instead of having it linger around.
No, that's the opposite, when sending 2^31-1, we're telling it "this is
the last one I'm willing to consider", ie "you can still send as much as
you want".
> Whatever the specific GOAWAY fix, I think its important we don't
> unnecessarily close or send GOAWAY messages.
I agree, but I wanted to be able to close them when necessary, and given
that we (normally) don't emit "connection: close" and that it's used to
indicate a willingness to close the connection on HTTP/1, it makes sense
to do the same for HTTP/2. But when looking at our concrete real world
cases, we only emit this one with error messages, only one of which we're
really interested in for the close (403 for deny). So better reverse the
problem and consider that we need an action to close the connection and
let all other messages keep the connection open.
> When I look at my HTTP
> Basic Auth scenario, I can see that HTTP2 is more chatty and
> inefficient due to all the closing (and since there is actually a
> connection setup as opposed to HTTP/1.1 where we just have
> transactions).
Not surprised, which is why I think the current situation is much better
now.
Thanks for the discussion!
Willy
