fix: make HTTP2 invalid headers recoverable (#4356) #4397
Conversation
- Add try-catch blocks around all session.request() calls in writeH2 - Implement automatic retry logic for ERR_HTTP2_INVALID_CONNECTION_HEADERS - Handle expectContinue, regular request, and CONNECT method paths - Destroy session and retry with new connection on first error - Abort gracefully if retry also fails - Add comprehensive test coverage for error handling Fixes nodejs#4356
There was a problem hiding this comment.
Good work implementing this, left few comments.
Overall, I'm a bit over the fence of the approach proposed in the issue itself; my concern with constant retries is that we need to keep the state of the request persisted which is not idea. Each request should be independent and fail or get done, not retried internally.
As well, server might never recover, leading to constant similar failures.
I propose the following, if this issue occurs, undici:
- Wraps the error in an Undici error
- Aborts the request
- Drops the session
- Resumes the queue
In that way, we add possibility for interceptors and callers to decide how and wether or not to retry the request.
test/http2-error-handling-test.js
Outdated
| 'client-h2.js should contain session.request calls' | ||
| ) | ||
|
|
||
| console.log('✅ client-h2.js contains the necessary error handling code') |
|
|
||
| test('undici should recover from invalid HTTP2 headers', async (t) => { | ||
| const server = createInvalidHeaderServer(() => { | ||
| // console.log('Server listening'); |
| callCount++ | ||
| if (callCount === 1) { | ||
| // First request: send invalid HTTP/1 header in HTTP2 response | ||
| console.log('[SERVER] Sending invalid header response') |
| stream.end('hello') | ||
| } else { | ||
| // Second request (retry): send valid response | ||
| console.log('[SERVER] Sending valid response') |
| const { Client } = require('..') | ||
| const pem = require('https-pem') | ||
|
|
||
| const PORT = 5678 |
There was a problem hiding this comment.
No need to hardcode port, when listening without a port, a random port will be picked; we can get it later through server.address method.
| const server = http2.createSecureServer(pem) | ||
| let callCount = 0 | ||
| server.on('stream', (stream, headers) => { | ||
| console.log('[SERVER] Received stream, callCount:', callCount + 1) |
test/http2-error-handling-test.js
Outdated
| assert.ok(errorCaught, 'Error should be catchable') | ||
| assert.strictEqual(errorCode, 'ERR_HTTP2_INVALID_CONNECTION_HEADERS', 'Error code should match') | ||
|
|
||
| console.log('✅ ERR_HTTP2_INVALID_CONNECTION_HEADERS can be caught and handled') |
lib/dispatcher/client-h2.js
Outdated
| client[kHTTP2Session] = null | ||
| } | ||
| // Leave request in queue for retry | ||
| setImmediate(() => { |
There was a problem hiding this comment.
Why did you use setImmediate?
|
@metcoder95 |
Fixes #4356
Status