Skip to content

lib: add aborted() utility function #46494

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.

Sign up for GitHub

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

Merged
nodejs-github-bot merged 26 commits into from
Feb 7, 2023
Merged

lib: add aborted() utility function #46494

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
6e797f5
lib: add aborted() utility function
debadree25 Feb 4, 2023
3834e9f
fixup! update param validation
debadree25 Feb 4, 2023
1cab0ca
fixup! make async func
debadree25 Feb 4, 2023
2e41a14
fixup! make resource compulsory
debadree25 Feb 4, 2023
0a58ffc
fixup! add doc
debadree25 Feb 4, 2023
f3621fd
fixup! doc fixup
debadree25 Feb 4, 2023
3bfeec0
fixup! doc fixup
debadree25 Feb 4, 2023
a2f2c56
fixup! doc
debadree25 Feb 4, 2023
f076729
Update doc/api/util.md
debadree25 Feb 5, 2023
6334317
Update doc/api/util.md
debadree25 Feb 5, 2023
7ff30c8
Update test/parallel/test-aborted-util.js
debadree25 Feb 5, 2023
f1ca1f7
Update test/parallel/test-aborted-util.js
debadree25 Feb 5, 2023
812c723
Update test/parallel/test-aborted-util.js
debadree25 Feb 5, 2023
b209fe1
fixup
debadree25 Feb 5, 2023
2f633c6
fixup
debadree25 Feb 5, 2023
b03ab4e
fixup! doc
debadree25 Feb 6, 2023
bae2765
fixup! doc
debadree25 Feb 6, 2023
b1a93a5
fixup! ordering
debadree25 Feb 6, 2023
8dfe5b7
fixup! ordering
debadree25 Feb 6, 2023
a5b43fe
fixup! ordering
debadree25 Feb 6, 2023
2401897
fixup! ordering
debadree25 Feb 6, 2023
347c252
fixup! ordering
debadree25 Feb 6, 2023
2446138
fixup! refactor function
debadree25 Feb 6, 2023
2b6929c
Update test/parallel/test-aborted-util.js
debadree25 Feb 6, 2023
a4a7bef
Update test/parallel/test-aborted-util.js
debadree25 Feb 6, 2023
f106f87
fixup! add undefined check
debadree25 Feb 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions lib/internal/abort_controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ const {
Symbol,
SymbolToStringTag,
WeakRef,
PromiseResolve,
Comment thread
aduh95 marked this conversation as resolved.
Outdated
} = primordials;

const {
Expand All @@ -22,11 +23,13 @@ const {
kTrustEvent,
kNewListener,
kRemoveListener,
kWeakHandler,
} = require('internal/event_target');
const {
customInspectSymbol,
kEnumerableProperty,
kEmptyObject,
createDeferredPromise,
Comment thread
debadree25 marked this conversation as resolved.
Outdated
} = require('internal/util');
const { inspect } = require('internal/util/inspect');
const {
Expand Down Expand Up @@ -357,6 +360,20 @@ function transferableAbortController() {
return AbortController[kMakeTransferable]();
}

/**
* @param {AbortSignal} signal
* @param {any} resource
* @returns {Promise<void>}
*/
function aborted(signal, resource = null) {
Comment thread
aduh95 marked this conversation as resolved.
Outdated
validateAbortSignal(signal, 'signal');
if (signal.aborted)
return PromiseResolve();
const abortPromise = createDeferredPromise();
signal.addEventListener('abort', abortPromise.resolve, { [kWeakHandler]: resource, once: true });
Comment thread
aduh95 marked this conversation as resolved.
Comment thread
aduh95 marked this conversation as resolved.
return abortPromise.promise;
}

ObjectDefineProperties(AbortController.prototype, {
signal: kEnumerableProperty,
abort: kEnumerableProperty,
Expand All @@ -377,4 +394,5 @@ module.exports = {
ClonedAbortSignal,
transferableAbortSignal,
transferableAbortController,
aborted,
Comment thread
debadree25 marked this conversation as resolved.
Outdated
};
3 changes: 3 additions & 0 deletions lib/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,6 +393,9 @@ module.exports = {
get transferableAbortController() {
return lazyAbortController().transferableAbortController;
},
get aborted() {
return lazyAbortController().aborted;
},
types
};

Expand Down
37 changes: 37 additions & 0 deletions test/parallel/test-aborted-util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
// Flags: --expose-gc
'use strict';

const common = require('../common');
const { aborted } = require('util');
const assert = require('assert');
const { getEventListeners } = require('events');

{
// Test aborted works
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 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();
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis Feb 4, 2023

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Member Author

@debadree25 debadree25 Feb 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The 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

Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis Feb 4, 2023

Choose a reason for hiding this comment

The 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);
});
}