RepoMirrors
/
haproxy
mirror of http://git.haproxy.org/git/haproxy.git/
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

BUG/MEDIUM: mux-h2: make sure control frames do not refresh the idle timeout 

Christopher found as part of the analysis of Tim's issue #1891 that commit
15a4733d5 ("BUG/MEDIUM: mux-h2: make use of http-request and keep-alive
timeouts") introduced in 2.6 incompletely addressed a timeout issue in the
H2 mux. The problem was that the http-keepalive and http-request timeouts
were not applied before it. With that commit they are now considered, but
if a GOAWAY is sent (or even attempted to be sent), then they are not used
anymore again, because the way the code is arranged consists in applying
the client-fin timeout (if set) to the current date, and falling back to
the client timeout, without considering the idle_start period. This means
that a config having a "timeout http-keepalive" would still not close the
connection quickly when facing a client that periodically sends PING,
PRIORITY or whatever other frame types.

In addition, after the GOAWAY was attempted to be sent, there was no check
for pending data in the output buffer, meaning that it would be possible
to truncate some responses in configs involving a very short client-fin
timeout.

Finally the spreading of the closures during the soft-stop brought in 2.6
by commit b5d968d9b ("MEDIUM: global: Add a "close-spread-time" option to
spread soft-stop on time window") didn't consider the particular case of
an idle "pre-connect" connection, which would also live long if a browser
failed to deliver a valid request for a long time.

All of this indicates that the conditions must be reworked so as not to
have that level of exclusion between conditions, but rather stick to the
rules from the doc that are already enforced on other muxes:
  - timeout client always applies if there are data pending, and is
    relative to each new I/O ;
  - timeout http-request applies before the first complete request and
    is relative to the entry in idle state ;
  - timeout http-keepalive applies between idle and the next complete
    request and is relative to the entry in idle state ;
  - timeout client-fin applies when in idle after a shut was sent (here
    the shut is the GOAWAY). The shut may only be considered as sent if
    the buffer is empty and the flags indicate that it was successfully
    sent (or failed) but not if it's still waiting for some room in the
    output buffer for example. This implies that this timeout may then
    lower the http-keepalive/http-request ones.

This is what this patch implements. Of course the client timeout still
applies as a fallback when all the ones above are not set or when their
conditions are not met.

It would seem reasoanble to backport this to 2.7 first, then only after
one or two releases to 2.6.
This commit is contained in:
Willy Tarreau 2023-05-15 11:28:48 +02:00
parent df97f472fa
commit d38d8c6ccb
1 changed files with 31 additions and 24 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

55
src/mux_h2.c
View File

@ -603,34 +603,41 @@ static void h2c_update_timeout(struct h2c *h2c)
if (h2c_may_expire(h2c)) {
/* no more streams attached */
if (h2c->last_sid >= 0) {
/* GOAWAY sent, closing in progress */
h2c->task->expire = tick_add_ifset(now_ms, h2c->shut_timeout);
is_idle_conn = 1;
} else if (br_data(h2c->mbuf)) {
if (br_data(h2c->mbuf)) {
/* pending output data: always the regular data timeout */
h2c->task->expire = tick_add_ifset(now_ms, h2c->timeout);
} else if (!(h2c->flags & H2_CF_IS_BACK) && h2c->max_id > 0 && !b_data(&h2c->dbuf)) {
/* idle after having seen one stream => keep-alive */
int to;
if (tick_isset(h2c->proxy->timeout.httpka))
to = h2c->proxy->timeout.httpka;
else
to = h2c->proxy->timeout.httpreq;
h2c->task->expire = tick_add_ifset(h2c->idle_start, to);
is_idle_conn = 1;
} else {
/* before first request, or started to deserialize a
* new req => http-request, but only set, not refresh.
*/
int exp = (h2c->flags & H2_CF_IS_BACK) ? TICK_ETERNITY : h2c->proxy->timeout.httpreq;
h2c->task->expire = tick_add_ifset(h2c->idle_start, exp);
/* no stream, no output data */
if (!(h2c->flags & H2_CF_IS_BACK)) {
int to;
if (h2c->max_id > 0 && !b_data(&h2c->dbuf) &&
tick_isset(h2c->proxy->timeout.httpka)) {
/* idle after having seen one stream => keep-alive */
to = h2c->proxy->timeout.httpka;
} else {
/* before first request, or started to deserialize a
* new req => http-request.
*/
to = h2c->proxy->timeout.httpreq;
}
h2c->task->expire = tick_add_ifset(h2c->idle_start, to);
is_idle_conn = 1;
}
if (h2c->flags & (H2_CF_GOAWAY_SENT|H2_CF_GOAWAY_FAILED)) {
/* GOAWAY sent (or failed), closing in progress */
int exp = tick_add_ifset(now_ms, h2c->shut_timeout);
h2c->task->expire = tick_first(h2c->task->expire, exp);
is_idle_conn = 1;
}
/* if a timeout above was not set, fall back to the default one */
if (!tick_isset(h2c->task->expire))
h2c->task->expire = tick_add_ifset(now_ms, h2c->timeout);
}
/* if a timeout above was not set, fall back to the default one */
if (!tick_isset(h2c->task->expire))
h2c->task->expire = tick_add_ifset(now_ms, h2c->timeout);
if ((h2c->proxy->flags & (PR_FL_DISABLED|PR_FL_STOPPED)) &&
is_idle_conn && tick_isset(global.close_spread_end)) {