fix(http2): Fixed HTTP2 stream closure handling #2710
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2710 +/- ##
==========================================
- Coverage 85.54% 85.27% -0.27%
==========================================
Files 76 84 +8
Lines 6858 7595 +737
==========================================
+ Hits 5867 6477 +610
- Misses 991 1118 +127 ☔ View full report in Codecov by Sentry. |
lib/fetch/index.js
Outdated
| if (err.name === 'NGHTTP2_INTERNAL_ERROR') { | ||
| // Reject pending Promise when HTTP2 stream is closed | ||
| return Promise.reject(new Error('HTTP2 stream closed with NGHTTP2_INTERNAL_ERROR')) | ||
| } |
There was a problem hiding this comment.
This is not part of spec. I think this issue should be resolved in undici core. Not fetch.
greetings I have taken some steps, how can I use this in chore, I would be very happy if you have any suggestions @ronag |
metcoder95
changed the title
|
Take a look at Line 1639 in 0a069ab http2
|
|
@metcoder95 tried a few steps thanks for your support |
lib/client.js
Outdated
| @@ -1840,7 +1841,9 @@ function writeH2 (client, session, request) { | |||
| }) | |||
|
|
|||
| stream.once('frameError', (type, code) => { | |||
| const err = new InformationalError(`HTTP/2: "frameError" received - type ${type}, code ${code}`) | |||
| const errorMessage = `HTTP/2: Frame error received - type: ${type}, code: ${code}.` | |||
There was a problem hiding this comment.
My understanding was that the 'error' event a few lines above was unhandled, and that 'frameError' is already being handled decently albeit with a different message
As far as I can tell the current changeset doesnt alter behavior other than to change the error message.
However, when I naively attempted to add errorRequest to the 'error' event handler, it resulted in a test like this failing (@metcoder95 this was the MRE you requested),
it additionally requires adding errorRequest(client, request, err) in the above stream.once('error', function (err) { handler
test('Client closing completed request should not error', async (t) => {
const server = createSecureServer(pem, async (req, res) => {
let body = ''
req.setEncoding('utf8')
res.writeHead(200, {
'content-type': 'text/plain; charset=utf-8',
})
for await (const chunk of req) {
body += chunk
}
res.end('hello h2')
})
server.listen()
await once(server, 'listening')
const client = new Client(`https://localhost:${server.address().port}`, {
connect: {
rejectUnauthorized: false,
},
allowH2: true,
})
const resp = await client.request({
path: '/',
method: 'GET',
})
await client.close()
await server.close()
})There was a problem hiding this comment.
that would fit our purpose? @jackschu
stream.once('error', function (err) {
if (client[kHTTP2Session] && !client[kHTTP2Session].destroyed && !this.closed && !this.destroyed) {
errorRequest(client, request, err);
h2State.streams -= 1;
util.destroy(stream, err);
}
});
stream.once('frameError', (type, code) => {
const errorMessage = `HTTP/2: Frame error received - type: ${type}, code: ${code}.`;
const err = new HTTP2Error(errorMessage);
errorRequest(client, request, err);
if (client[kHTTP2Session] && !client[kHTTP2Session].destroyed && !this.closed && !this.destroyed) {
h2State.streams -= 1;
util.destroy(stream, err);
}
});There was a problem hiding this comment.
I'm not sure what the right patch is to be honest, when I saw this in prod this.closed was true and the promise was left unresolved
There was a problem hiding this comment.
Oh wow, sorry. Too late to the party
Let's give it a try, the frameError should also error the request that is tight to the given stream that received that error; can you also add some testing around it @mertcanaltin ?
|
@metcoder95 @ronag @KhafraDev I would like to update this place, I would appreciate your comments 🙏 |
|
This is outside of my expertise. I trust @metcoder95 here. |
test/http2.js
Outdated
| @@ -1265,6 +1265,53 @@ test('The h2 pseudo-headers is not included in the headers', async t => { | |||
| t.strictEqual(response.headers[':status'], undefined) | |||
| }) | |||
|
|
|||
| test('HTTP/2 stream closed with NGHTTP2_INTERNAL_ERROR', async (t) => { | |||
There was a problem hiding this comment.
you wrote a tap test. we switched to node native test runner
|
@mertcanaltin can you rebase this on top of main? I think there are quite a few spurious changes in there that make reviewing this quite hard via gh. |
|
I'll tidy up here, I blew my codes while synchronizing 😢 |
This PR addresses an issue related to handling the closure of HTTP2 streams with the NGHTTP2_INTERNAL_ERROR code. Now, when HTTP2 streams are closed with the NGHTTP2_INTERNAL_ERROR code, the promise is rejected instead of being left unresolved.
#2675