fix(H2 Client): bind stream 'data' listener only after received 'response' event#2985
Conversation
ronag
left a comment
There was a problem hiding this comment.
This need to be fixed somewhere else. Can we defer the onData in the h2 wrapper?
|
@ronag like doing this? in client-h2.js let isResponseStarted = false
stream.once('response', headers => {
isResponseStarted = true
const { [HTTP2_HEADER_STATUS]: statusCode, ...realHeaders } = headers
request.onResponseStarted()
if (request.onHeaders(Number(statusCode), realHeaders, stream.resume.bind(stream), '') === false) {
stream.pause()
}
})
stream.on('data', (chunk) => {
if (!isResponseStarted || request.onData(chunk) === false) {
stream.pause()
}
}) |
mcollina
left a comment
There was a problem hiding this comment.
Thanks for opening a PR! Can you please add a unit test?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2985 +/- ##
=======================================
Coverage 93.49% 93.49%
=======================================
Files 89 89
Lines 24214 24216 +2
=======================================
+ Hits 22638 22640 +2
Misses 1576 1576 ☔ View full report in Codecov by Sentry. |
|
@mcollina this would maybe be a long term test, because the error can happen at any time, immediately or not, it would be like using the script reported in the issue and wait indefinitely if the scenario occurs |
…n HTTP/2" This reverts commit cd2eaf4.
|
|
||
| stream.on('data', (chunk) => { | ||
| if (request.onData(chunk) === false) { | ||
| if (!isStreamResponseStarted || request.onData(chunk) === false) { |
There was a problem hiding this comment.
this will throw away the first chunk?
There was a problem hiding this comment.
@ronag thaks, good point, I was focused on seeing that the error no longer occurs.
Is it ok to declare a temp chunks array and if isStreamResponseStarted is false in 'data' event push the chunk and then in 'response' event if the temp chunks array has length > 0 call the request.onData method after the request.onHeaders?
There was a problem hiding this comment.
This does kind of feel like an error in node core http, it doesn't make sense to me that data is emitted before response. Do you think you could report it in the node core repo in addition to this workaround PR.
There was a problem hiding this comment.
@ronag before report,
i was checking nodejs documentation about http2
there's one example here: https://nodejs.org/api/http2.html#clienthttp2sessionrequestheaders-options
const http2 = require('node:http2');
const clientSession = http2.connect('https://localhost:1234');
const {
HTTP2_HEADER_PATH,
HTTP2_HEADER_STATUS,
} = http2.constants;
const req = clientSession.request({ [HTTP2_HEADER_PATH]: '/' });
req.on('response', (headers) => {
console.log(headers[HTTP2_HEADER_STATUS]);
req.on('data', (chunk) => { /* .. */ });
req.on('end', () => { /* .. */ });
});the data event listener is binded inside the response listener callback.
So i have moved client-h2.js stream on data event inside stream on response (at the and of the callback) and it seems to be ok now
stream.once('response', headers => {
const { [HTTP2_HEADER_STATUS]: statusCode, ...realHeaders } = headers
request.onResponseStarted()
if (request.onHeaders(Number(statusCode), realHeaders, stream.resume.bind(stream), '') === false) {
stream.pause()
}
stream.on('data', (chunk) => {
if (request.onData(chunk) === false) {
stream.pause()
}
})
})my test script is:
const Agent = require("./lib/dispatcher/agent");
const { fetch } = require("./lib/web/fetch");
const dispatcher = new Agent({ allowH2: true });
(async () => {
while (true) {
//console.log("fetching 1000 requests");
//console.time();
await Promise.all(
Array.from({ length: 1000 }).map(async () => {
try {
const response = await fetch("https://http2.pro/api/v1", {
dispatcher,
});
const text = await response.text();
//console.log(text);
if(!text) throw new Error("NOT GOOD")
} catch (e) {
console.error(e);
}
}),
);
//console.timeEnd();
}
})();This make more sense to you?
Thanks
There was a problem hiding this comment.
Make sense, but yea, please open an issue because this seems wrong.
…response event" This reverts commit 9219ee3.
| stream.pause() | ||
| } | ||
|
|
||
| stream.on('data', (chunk) => { |
There was a problem hiding this comment.
Can we add the link to the issue as comment?
This relates to...
#2808
Rationale
Sporadically when doing massive h2 requests for a certain period of time a breaking assertion is triggered "assert(!this.aborted)" in onHeaders Method [undici/lib/core/request.js] causing application crash
Changes
Features
N/A
Bug Fixes
Breaking Changes and Deprecations
Status
[cert]: https://github.com/nodejs/undici/blob/main/CONTRIBUTING.md #