-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
lib: add aborted() utility function #46494
Changes from all commits
6e797f5
3834e9f
1cab0ca
2e41a14
0a58ffc
f3621fd
3bfeec0
a2f2c56
f076729
6334317
7ff30c8
f1ca1f7
812c723
b209fe1
2f633c6
b03ab4e
bae2765
b1a93a5
8dfe5b7
a5b43fe
2401897
347c252
2446138
2b6929c
a4a7bef
f106f87
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,59 @@ | ||
// Flags: --expose-gc | ||
'use strict'; | ||
|
||
const common = require('../common'); | ||
const { aborted } = require('util'); | ||
const assert = require('assert'); | ||
const { getEventListeners } = require('events'); | ||
const { spawn } = require('child_process'); | ||
|
||
{ | ||
// Test aborted works when provided a resource | ||
const ac = new AbortController(); | ||
aborted(ac.signal, {}).then(common.mustCall()); | ||
ac.abort(); | ||
assert.strictEqual(ac.signal.aborted, true); | ||
assert.strictEqual(getEventListeners(ac.signal, 'abort').length, 0); | ||
} | ||
|
||
{ | ||
// Test aborted with gc cleanup | ||
const ac = new AbortController(); | ||
aborted(ac.signal, {}).then(common.mustNotCall()); | ||
setImmediate(() => { | ||
global.gc(); | ||
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. What does GC accomplish here? That the aborted() promise is collected before abort() is called? I don't know if that kind of GC observability is a good thing. Maybe it works now but GC changes can break such patterns. Looks fragile. 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. basically want to cleanup the empty object to check if the listener is removed when the resource is cleaned by the gc, unsure of any other ways to test this 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. If it's just for testing, then it's probably fine. I mean, it could still break in the future but we can revisit if and when that happens. I'd advise against documenting that behavior though. It's simply not very robust. |
||
ac.abort(); | ||
assert.strictEqual(ac.signal.aborted, true); | ||
assert.strictEqual(getEventListeners(ac.signal, 'abort').length, 0); | ||
}); | ||
} | ||
|
||
{ | ||
// Fails with error if not provided abort signal | ||
Promise.all([{}, null, undefined, Symbol(), [], 1, 0, 1n, true, false, 'a', () => {}].map((sig) => | ||
assert.rejects(aborted(sig, {}), { | ||
code: 'ERR_INVALID_ARG_TYPE', | ||
}) | ||
)).then(common.mustCall()); | ||
} | ||
|
||
{ | ||
// Fails if not provided a resource | ||
const ac = new AbortController(); | ||
Promise.all([null, undefined, 0, 1, 0n, 1n, Symbol(), '', 'a'].map((resource) => | ||
assert.rejects(aborted(ac.signal, resource), { | ||
code: 'ERR_INVALID_ARG_TYPE', | ||
}) | ||
)).then(common.mustCall()); | ||
} | ||
|
||
{ | ||
const childProcess = spawn(process.execPath, ['--input-type=module']); | ||
childProcess.on('exit', common.mustCall((code) => { | ||
assert.strictEqual(code, 13); | ||
})); | ||
childProcess.stdin.end(` | ||
import { aborted } from 'node:util'; | ||
await aborted(new AbortController().signal, {}); | ||
`); | ||
} | ||
debadree25 marked this conversation as resolved.
Show resolved
Hide resolved
|
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.
Have added this check since turns out changing the condition in
validateAbortSignal()
immediately results in the build breaking, so i assume its probably not a bug but would like a review hereThere 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.
It seems like a common pattern across to the codebase to check for undefined and then validate the signal so i think its probably fine?