Skip to content
This repository was archived by the owner on Feb 16, 2026. It is now read-only.

Commit 092f497

Browse files
drididridi
committed
h2: Make requests walk away from the waiting list
When a session goes to the cleanup step, it will indefinitely try to kill all the streams until those stuck in a waiting list reembark a thread. This can lead to clients timing out, closing the h2 session, and opening a new h2 session for the exact same requests. This can artifically inflate the number of dead tasks in a waiting list with generous backend timeouts. To avoid that, an h2 stream will keep a list of requests currently in a waiting list and make them walk away quietly during cleanup. An h2 stream would normally only have one request running at any time but this may no longer be true with modules like vmod_pesi. Better diff with the --ignore-all-space option.
1 parent a469275 commit 092f497

4 files changed

Lines changed: 197 additions & 14 deletions

File tree

‎bin/varnishd/http2/cache_http2_proto.c‎

Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -209,9 +209,13 @@ void
209209
h2_kill_req(struct worker *wrk, struct h2_sess *h2,
210210
struct h2_req *r2, h2_error h2e)
211211
{
212+
VTAILQ_HEAD(, req) wl;
213+
struct req *req, *t;
214+
struct objhead *oh;
212215

213216
ASSERT_RXTHR(h2);
214217
AN(h2e);
218+
VTAILQ_INIT(&wl);
215219
Lck_Lock(&h2->sess->mtx);
216220
VSLb(h2->vsl, SLT_Debug, "KILL st=%u state=%d sched=%d",
217221
r2->stream, r2->state, r2->scheduled);
@@ -223,16 +227,34 @@ h2_kill_req(struct worker *wrk, struct h2_sess *h2,
223227
if (r2->error == NULL)
224228
r2->error = h2e;
225229
if (r2->scheduled) {
226-
if (r2->cond != NULL)
227-
AZ(pthread_cond_signal(r2->cond));
228-
r2 = NULL;
230+
VTAILQ_FOREACH_SAFE(req, &r2->waitinglist, t_list, t) {
231+
CHECK_OBJ(req, REQ_MAGIC);
232+
VTAILQ_REMOVE(&r2->waitinglist, req, t_list);
233+
VTAILQ_INSERT_TAIL(&wl, req, t_list);
234+
}
235+
if (VTAILQ_EMPTY(&wl)) {
236+
if (r2->cond != NULL)
237+
AZ(pthread_cond_signal(r2->cond));
238+
r2 = NULL;
239+
}
229240
} else {
230241
if (r2->state == H2_S_OPEN && h2->new_req == r2->req)
231242
(void)h2h_decode_fini(h2);
232243
}
233244
Lck_Unlock(&h2->sess->mtx);
234-
if (r2 != NULL)
245+
if (VTAILQ_EMPTY(&wl) && r2 != NULL) {
235246
h2_del_req(wrk, r2);
247+
return;
248+
}
249+
VTAILQ_FOREACH_SAFE(req, &wl, t_list, t) {
250+
Lck_Lock(&h2->sess->mtx);
251+
VTAILQ_REMOVE(&wl, req, t_list);
252+
oh = req->transport_objhead;
253+
req->transport_objhead = NULL;
254+
Lck_Unlock(&h2->sess->mtx);
255+
HSH_WalkAway(wrk, &oh, req);
256+
AZ(oh);
257+
}
236258
}
237259

238260
/**********************************************************************/

‎bin/varnishd/http2/cache_http2_session.c‎

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -485,19 +485,24 @@ h2_top_reembark(struct worker *wrk, struct req *req)
485485
VSLb(req->vsl, SLT_Debug, "off h2 waiting list <%p>", r2);
486486

487487
Lck_Lock(&r2->h2sess->sess->mtx);
488-
VTAILQ_REMOVE(&r2->waitinglist, req, t_list);
489488
oh = req->transport_objhead;
490-
req->transport_objhead = NULL;
489+
CHECK_OBJ_ORNULL(oh, OBJHEAD_MAGIC);
490+
if (oh != NULL) {
491+
VTAILQ_REMOVE(&r2->waitinglist, req, t_list);
492+
req->transport_objhead = NULL;
493+
}
491494
Lck_Unlock(&r2->h2sess->sess->mtx);
492495

493-
CHECK_OBJ_NOTNULL(oh, OBJHEAD_MAGIC);
494-
Lck_Lock(&oh->mtx);
495-
AN(req->hash_objhead);
496-
AZ(req->waitinglist);
497-
AZ(req->wrk);
498-
assert(oh->refcnt > 1);
499-
oh->refcnt--;
500-
Lck_Unlock(&oh->mtx);
496+
if (oh != NULL) {
497+
Lck_Lock(&oh->mtx);
498+
AN(req->hash_objhead);
499+
AZ(req->waitinglist);
500+
AZ(req->wrk);
501+
AZ(req->walkaway);
502+
assert(oh->refcnt > 1);
503+
oh->refcnt--;
504+
Lck_Unlock(&oh->mtx);
505+
}
501506
}
502507

503508
struct transport HTTP2_transport = {

‎bin/varnishtest/tests/t02023.vtc‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
varnishtest "Dying h2 session with stream on waiting list"
2+
3+
barrier b1 cond 2
4+
barrier b2 cond 2
5+
barrier b3 cond 2
6+
7+
server s1 {
8+
rxreq
9+
barrier b1 sync
10+
barrier b3 sync
11+
txresp
12+
} -start
13+
14+
varnish v1 -cliok "param.set thread_pools 1"
15+
varnish v1 -cliok "param.set feature +http2"
16+
varnish v1 -cliok "param.set debug +waitinglist"
17+
varnish v1 -cliok "param.set debug +syncvsl"
18+
varnish v1 -vcl+backend {} -start
19+
20+
logexpect l1 -v v1 -g raw {
21+
expect * 1003 Debug "on waiting list"
22+
} -start
23+
24+
logexpect l2 -v v1 {
25+
expect * 1003 Debug "walking away"
26+
expect 0 = Debug "off waiting list"
27+
expect * = Timestamp "^Waitinglist: "
28+
expect 0 = Error "The client is going away"
29+
} -start
30+
31+
client c1 {
32+
stream 1 {
33+
txreq
34+
barrier b1 sync
35+
} -run
36+
stream 3 {
37+
txreq
38+
} -run
39+
stream 5 {
40+
barrier b2 sync
41+
txsettings
42+
} -run
43+
stream 0 {
44+
rxgoaway
45+
expect goaway.laststream == 3
46+
expect goaway.err == PROTOCOL_ERROR
47+
} -run
48+
} -start
49+
50+
logexpect l1 -wait
51+
barrier b2 sync
52+
53+
logexpect l2 -wait
54+
barrier b3 sync
55+
56+
server s1 -wait
57+
58+
varnish v1 -expect busy_killed == 1
59+
60+
client c2 {
61+
txreq
62+
rxresp
63+
expect resp.status == 200
64+
} -run
65+
66+
varnish v1 -expect cache_hit == 1

‎bin/varnishtest/tests/t02024.vtc‎

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
varnishtest "Dying h2 session with ESI stream on waiting list"
2+
3+
barrier b1 cond 2
4+
barrier b2 cond 2
5+
barrier b3 cond 2
6+
7+
server s1 {
8+
rxreq
9+
expect req.url == "/esi"
10+
barrier b1 sync
11+
barrier b3 sync
12+
txresp -hdr "included: esi" -body hello
13+
} -start
14+
15+
server s2 {
16+
rxreq
17+
expect req.url == "/"
18+
txresp -body {<esi:include src="/esi"/><esi:include src="/esi"/><esi:include src="/esi"/>}
19+
} -start
20+
21+
varnish v1 -cliok "param.set thread_pools 1"
22+
varnish v1 -cliok "param.set feature +http2"
23+
varnish v1 -cliok "param.set debug +waitinglist"
24+
varnish v1 -cliok "param.set debug +syncvsl"
25+
varnish v1 -vcl+backend {
26+
sub vcl_recv {
27+
if (req.url == "/") {
28+
set req.backend_hint = s2;
29+
}
30+
}
31+
sub vcl_backend_response {
32+
set beresp.do_esi = !beresp.http.included;
33+
}
34+
} -start
35+
36+
logexpect l1 -v v1 -g raw {
37+
expect * 1006 Debug "on waiting list"
38+
} -start
39+
40+
logexpect l2 -v v1 -g raw {
41+
expect * 1006 Debug "walking away"
42+
expect 0 1006 Debug "off waiting list"
43+
} -start
44+
45+
# c1 will block during a backend fetch
46+
client c1 {
47+
txreq -url "/esi"
48+
rxresp
49+
expect resp.body == hello
50+
} -start
51+
52+
barrier b1 sync
53+
54+
client c2 {
55+
stream 1 {
56+
txreq
57+
} -run
58+
stream 3 {
59+
barrier b2 sync
60+
# bork the session
61+
txsettings
62+
} -run
63+
stream 0 {
64+
rxgoaway
65+
expect goaway.laststream == 1
66+
expect goaway.err == PROTOCOL_ERROR
67+
} -run
68+
} -start
69+
70+
logexpect l1 -wait
71+
barrier b2 sync
72+
73+
logexpect l2 -wait
74+
barrier b3 sync
75+
76+
client c1 -wait
77+
client c2 -wait
78+
server s1 -wait
79+
server s2 -wait
80+
81+
varnish v1 -expect busy_killed == 1
82+
varnish v1 -expect cache_hit == 0
83+
84+
client c3 {
85+
txreq
86+
rxresp
87+
expect resp.body == hellohellohello
88+
} -run
89+
90+
varnish v1 -expect cache_hit == 4

0 commit comments

Comments
 (0)