Hello Willy,
2017-11-22 6:50 GMT+01:00 Willy Tarreau <[email protected]>: > Hi Lukas, > > On Wed, Nov 22, 2017 at 01:43:32AM +0100, Lukas Tribus wrote: >> 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. 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. >> 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. If we are talking about Chrome, this was fixed in Chrome 60 (see [1] and [2]). 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 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 ? Yes, it does limit the stall to 1 second, but I'm must say I'm not completely happy with this fix alone. 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 > 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. Whatever the specific GOAWAY fix, I think its important we don't unnecessarily close or send GOAWAY messages. 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). Regards, Lukas [1] https://chromium.googlesource.com/chromium/src.git/+/c4769d412d1b89724db8eb42577cd975fd85f559 [2] https://bugs.chromium.org/p/chromium/issues/detail?id=681477 [3] http://nginx.org/en/docs/http/ngx_http_v2_module.html#http2_max_requests [4] http://hg.nginx.org/nginx/file/tip/src/http/v2/ngx_http_v2.c#l4250
