-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
lib: reuse default DOMException in AbortController #47908
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
Changes from 1 commit
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 |
---|---|---|
@@ -0,0 +1,16 @@ | ||
'use strict'; | ||
const common = require('../common.js'); | ||
|
||
const bench = common.createBenchmark(main, { | ||
n: [1e6], | ||
}); | ||
|
||
function main({ n }) { | ||
bench.start(); | ||
for (let i = 0; i < n; i++) { | ||
const ac = new AbortController(); | ||
ac.signal.addEventListener('abort', () => {}); | ||
ac.abort(); | ||
} | ||
bench.end(n); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,6 +30,7 @@ const { | |
customInspectSymbol, | ||
kEmptyObject, | ||
kEnumerableProperty, | ||
SideEffectFreeRegExpPrototypeSymbolReplace, | ||
} = require('internal/util'); | ||
const { inspect } = require('internal/util/inspect'); | ||
const { | ||
|
@@ -63,11 +64,14 @@ const { | |
|
||
let _MessageChannel; | ||
let makeTransferable; | ||
let defaultDOMException; | ||
|
||
// Loading the MessageChannel and makeTransferable have to be done lazily | ||
// because otherwise we'll end up with a require cycle that ends up with | ||
// an incomplete initialization of abort_controller. | ||
|
||
const userModuleRegExp = /^ {4}at (?:[^/\\(]+ \()(?!node:(.+):\d+:\d+\)$).*/gm; | ||
|
||
function lazyMessageChannel() { | ||
_MessageChannel ??= require('internal/worker/io').MessageChannel; | ||
return new _MessageChannel(); | ||
|
@@ -79,6 +83,21 @@ function lazyMakeTransferable(obj) { | |
return makeTransferable(obj); | ||
} | ||
|
||
function lazyDOMException() { | ||
if (defaultDOMException) { | ||
return defaultDOMException; | ||
} | ||
|
||
defaultDOMException = new DOMException('This operation was aborted', 'AbortError'); | ||
|
||
// Avoid V8 leak and remove userland stackstrace | ||
defaultDOMException.stack = SideEffectFreeRegExpPrototypeSymbolReplace( | ||
userModuleRegExp, | ||
defaultDOMException.stack, | ||
''); | ||
return defaultDOMException; | ||
} | ||
|
||
const clearTimeoutRegistry = new SafeFinalizationRegistry(clearTimeout); | ||
const timeOutSignals = new SafeSet(); | ||
|
||
|
@@ -166,8 +185,10 @@ class AbortSignal extends EventTarget { | |
* @param {any} [reason] | ||
* @returns {AbortSignal} | ||
*/ | ||
static abort( | ||
reason = new DOMException('This operation was aborted', 'AbortError')) { | ||
static abort(reason) { | ||
if (reason === undefined) { | ||
reason = lazyDOMException(); | ||
} | ||
return createAbortSignal({ aborted: true, reason }); | ||
} | ||
|
||
|
@@ -328,7 +349,10 @@ class AbortController { | |
/** | ||
* @param {any} [reason] | ||
*/ | ||
abort(reason = new DOMException('This operation was aborted', 'AbortError')) { | ||
abort(reason) { | ||
if (reason === undefined) { | ||
reason = lazyDOMException(); | ||
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. Since we are not creating a new error everytime abort is called, 2 different usages in the code throwing this error, might have a different stack trace. Is this compliant with the spec? 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. ah good point, not sure about that, one thing we could do maybe is move this lazy loading inside the functions themselves. that should ensure correct trace? |
||
} | ||
abortSignal(this.#signal ??= createAbortSignal(), reason); | ||
} | ||
|
||
|
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.
Would
reason ??= lazyDOMException();
also work in here, and in the next usage?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.
I think so yes! updating this (since benchmark ci is running would pushing a commit interfere with it?)
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.
No, it won't interfere with it. 👍
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.
Done!
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.
I don’t think this is correct, users should be able to supply
null
as a reasonThere 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.
Oh ok yes reverting