-
Notifications
You must be signed in to change notification settings - Fork 539
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: resolve event-loop blocking when aborting a fetch #2660
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -313,16 +313,14 @@ function finalizeAndReportTiming (response, initiatorType = 'other') { | |
} | ||
|
||
// https://w3c.github.io/resource-timing/#dfn-mark-resource-timing | ||
function markResourceTiming (timingInfo, originalURL, initiatorType, globalThis, cacheState) { | ||
if (nodeMajor > 18 || (nodeMajor === 18 && nodeMinor >= 2)) { | ||
performance.markResourceTiming(timingInfo, originalURL.href, initiatorType, globalThis, cacheState) | ||
} | ||
} | ||
const markResourceTiming = (nodeMajor > 18 || (nodeMajor === 18 && nodeMinor >= 2)) | ||
? performance.markResourceTiming | ||
: () => {} | ||
Comment on lines
+316
to
+318
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This showed hot in the flame graphs, low hanging fruit. |
||
|
||
// https://fetch.spec.whatwg.org/#abort-fetch | ||
function abortFetch (p, request, responseObject, error) { | ||
// 1. Reject promise with error. | ||
p.reject(error) | ||
setImmediate(() => p.reject(error)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This shouldn't be necessary according to spec. Are we sure this is a spec bug and not something else? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Something is clogging the event loop. As I see other tests fail. So this is not the solution for the issue. calling setImmediate resulted in declogging it. Still investigating. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is an issue in v8, I vaguely recall an issue in node core where promises weren't being GC'ed in a loop There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
// 2. If request’s body is not null and is readable, then cancel request’s | ||
// body with error. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -76,18 +76,17 @@ class Pool extends PoolBase { | |
} | ||
|
||
[kGetDispatcher] () { | ||
let dispatcher = this[kClients].find(dispatcher => !dispatcher[kNeedDrain]) | ||
|
||
if (dispatcher) { | ||
return dispatcher | ||
for (const client of this[kClients]) { | ||
if (!client[kNeedDrain]) { | ||
return client | ||
} | ||
} | ||
|
||
if (!this[kConnections] || this[kClients].length < this[kConnections]) { | ||
dispatcher = this[kFactory](this[kUrl], this[kOptions]) | ||
const dispatcher = this[kFactory](this[kUrl], this[kOptions]) | ||
this[kAddClient](dispatcher) | ||
return dispatcher | ||
Comment on lines
+79
to
+88
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this was also kind of slow while benchmarking. Low hanging fruit |
||
} | ||
|
||
return dispatcher | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
originalURL is being passed instead of the stringified url