Skip to content

Commit e5b3364

Browse files
staylorstaylor
authored
fix(h2): guard onResponse against a 'response' event delivered after completion (#5440)
* fix(h2): guard onResponse against a 'response' event delivered after completion The HTTP/2 'response' handler (onResponse) only guarded request.aborted before calling request.onResponseStart, while its sibling handlers onEnd and onTrailers also guard request.completed. A 'response' frame delivered after the request has already completed (a stream-teardown race that shows up under load on shared h2 sessions with GOAWAY / refused-stream churn) therefore calls onResponseStart post-completion, tripping its assert(!this.completed). Because it throws on the http2 stream's event tick, it surfaces as an uncaught exception and crashes the process. Add the same request.completed guard the other handlers already use. Signed-off-by: Scott Taylor <[email protected]> Assisted-By: devx/26cd7e09-2a13-4e79-9cd0-8191fff4dc7a * test(h2): cover 'response' delivered after request completion Regression test for the onResponse completed-guard. Drives the real onResponse handler (via connectH2) against a fake stream — mirroring test/http2-late-data.js — and emits a 'response' event after the request has completed. Without the guard this invokes request.onResponseStart, whose assert(!this.completed) throws on the http2 event tick and crashes the process; with the guard the stream is released and the event ignored. Fails against the pre-fix handler with the exact assert(!this.completed) AssertionError and passes with the fix. Refs: #5440 Signed-off-by: Scott Taylor <[email protected]> Assisted-By: devx/26cd7e09-2a13-4e79-9cd0-8191fff4dc7a --------- Signed-off-by: Scott Taylor <[email protected]>
1 parent c0007f4 commit e5b3364

2 files changed

Lines changed: 193 additions & 4 deletions

File tree

‎lib/dispatcher/client-h2.js‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1247,10 +1247,13 @@ function onResponse (headers) {
12471247

12481248
// Due to the stream nature, it is possible we face a race condition
12491249
// where the stream has been assigned, but the request has been aborted
1250-
// the request remains in-flight and headers hasn't been received yet
1251-
// for those scenarios, best effort is to destroy the stream immediately
1252-
// as there's no value to keep it open.
1253-
if (request.aborted) {
1250+
// or already completed and headers hasn't been received yet. A late
1251+
// 'response' delivered after completion would call request.onResponseStart
1252+
// post-completion, tripping its `assert(!this.completed)` (an uncatchable
1253+
// throw on the http2 event tick). Guard `completed` here as onEnd/onTrailers
1254+
// already do; best effort is to release the stream immediately as there's
1255+
// no value to keep it open.
1256+
if (request.aborted || request.completed) {
12541257
releaseRequestStream(stream)
12551258
return
12561259
}

‎test/http2-response-after-completion.js‎

Lines changed: 186 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,186 @@
1+
'use strict'
2+
3+
// Regression test for nodejs/undici#5440.
4+
//
5+
// The HTTP/2 'response' handler (onResponse in client-h2.js) only guarded
6+
// request.aborted before calling request.onResponseStart, while its sibling
7+
// handlers onEnd/onTrailers also guard request.completed. A 'response' frame
8+
// delivered to a still-live stream *after* the request has completed (a
9+
// stream-teardown race seen under load on shared h2 sessions with GOAWAY /
10+
// refused-stream churn) therefore called onResponseStart post-completion,
11+
// tripping its assert(!this.completed). Because that throws on the http2
12+
// stream's event tick — outside any caller's try — it escaped as an uncaught
13+
// exception and crashed the process.
14+
//
15+
// This drives the real onResponse handler (registered by connectH2) against a
16+
// fake stream, mirroring test/http2-late-data.js, and asserts that a
17+
// post-completion 'response' is ignored instead of asserting.
18+
19+
const { test, after } = require('node:test')
20+
const { EventEmitter } = require('node:events')
21+
const { tspl } = require('@matteo.collina/tspl')
22+
23+
const connectH2 = require('../lib/dispatcher/client-h2')
24+
const Request = require('../lib/core/request')
25+
const {
26+
kUrl,
27+
kSocket,
28+
kMaxConcurrentStreams,
29+
kHTTP2InitialWindowSize,
30+
kHTTP2ConnectionWindowSize,
31+
kBodyTimeout,
32+
kStrictContentLength,
33+
kQueue,
34+
kRunningIdx,
35+
kPendingIdx,
36+
kOnError,
37+
kResume,
38+
kRunning,
39+
kPingInterval
40+
} = require('../lib/core/symbols')
41+
42+
class FakeSocket extends EventEmitter {
43+
constructor () {
44+
super()
45+
this.destroyed = false
46+
}
47+
48+
destroy () {
49+
this.destroyed = true
50+
return this
51+
}
52+
53+
ref () {}
54+
unref () {}
55+
}
56+
57+
class FakeStream extends EventEmitter {
58+
setTimeout () {}
59+
pause () {}
60+
resume () {}
61+
close () {}
62+
write () { return true }
63+
end () {}
64+
cork () {}
65+
uncork () {}
66+
}
67+
68+
class FakeSession extends EventEmitter {
69+
constructor (stream) {
70+
super()
71+
this.stream = stream
72+
this.closed = false
73+
this.destroyed = false
74+
}
75+
76+
request () {
77+
return this.stream
78+
}
79+
80+
close () {
81+
this.closed = true
82+
}
83+
84+
destroy () {
85+
this.destroyed = true
86+
}
87+
88+
ref () {}
89+
unref () {}
90+
ping (_, cb) {
91+
cb(null, 0)
92+
}
93+
}
94+
95+
test('Should ignore a late http2 "response" delivered after request completion', async (t) => {
96+
t = tspl(t, { plan: 6 })
97+
98+
const http2 = require('node:http2')
99+
const originalConnect = http2.connect
100+
101+
const stream = new FakeStream()
102+
const session = new FakeSession(stream)
103+
104+
http2.connect = function connectStub () {
105+
return session
106+
}
107+
108+
after(() => {
109+
http2.connect = originalConnect
110+
})
111+
112+
const client = {
113+
[kUrl]: new URL('https://localhost'),
114+
[kSocket]: null,
115+
[kMaxConcurrentStreams]: 100,
116+
[kHTTP2InitialWindowSize]: null,
117+
[kHTTP2ConnectionWindowSize]: null,
118+
[kBodyTimeout]: 30_000,
119+
[kStrictContentLength]: true,
120+
[kQueue]: [],
121+
[kRunningIdx]: 0,
122+
[kPendingIdx]: 0,
123+
[kRunning]: 1,
124+
[kPingInterval]: 0,
125+
[kOnError] (err) {
126+
t.ifError(err)
127+
},
128+
[kResume] () {},
129+
emit () {},
130+
destroyed: false
131+
}
132+
133+
const context = connectH2(client, new FakeSocket())
134+
135+
let onResponseStartedCalls = 0
136+
let onResponseStartCalls = 0
137+
138+
const request = new Request('https://localhost', {
139+
path: '/',
140+
method: 'GET',
141+
headers: {}
142+
}, {
143+
onRequestStart () {},
144+
onResponseStarted () {
145+
onResponseStartedCalls++
146+
},
147+
onResponseStart () {
148+
onResponseStartCalls++
149+
},
150+
onResponseData () {},
151+
onResponseEnd () {},
152+
onResponseError (_controller, err) {
153+
t.ifError(err)
154+
}
155+
})
156+
157+
client[kQueue].push(request)
158+
159+
t.ok(context.write(request))
160+
161+
// Mark the request completed directly: the real completion path
162+
// (onRequestStreamClose) also clears the stream's request state and removes
163+
// its listeners, which is the opposite of the race being reproduced here —
164+
// a 'response' frame buffered before teardown and delivered to the still-live
165+
// stream *after* completion. aborted stays false so this exercises the
166+
// completed guard specifically, not the pre-existing aborted one.
167+
request.completed = true
168+
t.strictEqual(request.aborted, false)
169+
170+
// Without the completed guard, onResponse calls request.onResponseStart,
171+
// whose assert(!this.completed) throws synchronously on the stream's
172+
// 'response' tick (an uncaught exception that crashes the process). With the
173+
// guard, onResponse releases the stream and returns.
174+
t.doesNotThrow(() => {
175+
stream.emit('response', { ':status': 200 })
176+
})
177+
178+
// onResponse ran (onResponseStarted fires before the guard)...
179+
t.strictEqual(onResponseStartedCalls, 1)
180+
// ...but the guard skipped the post-completion onResponseStart...
181+
t.strictEqual(onResponseStartCalls, 0)
182+
// ...and released the stream (releaseRequestStream removes its listeners).
183+
t.strictEqual(stream.listenerCount('trailers'), 0)
184+
185+
await t.completed
186+
})

0 commit comments

Comments
 (0)