Skip to content
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

Jump to bottom

timers: fix validation #54404

Merged
merged 1 commit into from
Aug 25, 2024
Merged

timers: fix validation #54404

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 2 additions & 3 deletions doc/api/timers.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -532,9 +532,8 @@ added:
An experimental API defined by the [Scheduling APIs][] draft specification
being developed as a standard Web Platform API.

Calling `timersPromises.scheduler.wait(delay, options)` is roughly equivalent
to calling `timersPromises.setTimeout(delay, undefined, options)` except that
the `ref` option is not supported.
Calling `timersPromises.scheduler.wait(delay, options)` is equivalent
to calling `timersPromises.setTimeout(delay, undefined, options)`.
Comment on lines +535 to +536
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

Copy link
Contributor Author

@ShogunPanda ShogunPanda Aug 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the documentation was incorrect (or not up-to-date). It was not roughly equivalent, it was strictly equivalent since it just wrapping it. I made that explicit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then I guess we should document ref as a valid option


```mjs
import { scheduler } from 'node:timers/promises';
Expand Down
91 changes: 51 additions & 40 deletions lib/timers/promises.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ const {
AbortError,
codes: {
ERR_ILLEGAL_CONSTRUCTOR,
ERR_INVALID_ARG_TYPE,
ERR_INVALID_THIS,
},
} = require('internal/errors');
Expand All @@ -33,6 +32,7 @@ const {
validateAbortSignal,
validateBoolean,
validateObject,
validateNumber,
} = require('internal/validators');

const {
Expand All @@ -50,34 +50,33 @@ function cancelListenerHandler(clear, reject, signal) {
}

function setTimeout(after, value, options = kEmptyObject) {
const args = value !== undefined ? [value] : value;
if (options == null || typeof options !== 'object') {
return PromiseReject(
new ERR_INVALID_ARG_TYPE(
'options',
'Object',
options));
}
const { signal, ref = true } = options;
try {
validateAbortSignal(signal, 'options.signal');
if (typeof after !== 'undefined') {
validateNumber(after, 'delay');
}

validateObject(options, 'options');

if (typeof options?.signal !== 'undefined') {
validateAbortSignal(options.signal, 'options.signal');
}

if (typeof options?.ref !== 'undefined') {
validateBoolean(options.ref, 'options.ref');
}
} catch (err) {
return PromiseReject(err);
}
if (typeof ref !== 'boolean') {
return PromiseReject(
new ERR_INVALID_ARG_TYPE(
'options.ref',
'boolean',
ref));
}

const { signal, ref = true } = options;

if (signal?.aborted) {
return PromiseReject(new AbortError(undefined, { cause: signal.reason }));
}

let oncancel;
const ret = new Promise((resolve, reject) => {
const timeout = new Timeout(resolve, after, args, false, ref);
const timeout = new Timeout(resolve, after, [value], false, ref);
insert(timeout, timeout._idleTimeout);
if (signal) {
oncancel = FunctionPrototypeBind(cancelListenerHandler,
Expand All @@ -93,30 +92,26 @@ function setTimeout(after, value, options = kEmptyObject) {
}

function setImmediate(value, options = kEmptyObject) {
if (options == null || typeof options !== 'object') {
return PromiseReject(
new ERR_INVALID_ARG_TYPE(
'options',
'Object',
options));
}
const { signal, ref = true } = options;
try {
validateAbortSignal(signal, 'options.signal');
validateObject(options, 'options');

if (typeof options?.signal !== 'undefined') {
validateAbortSignal(options.signal, 'options.signal');
}

if (typeof options?.ref !== 'undefined') {
validateBoolean(options.ref, 'options.ref');
}
} catch (err) {
return PromiseReject(err);
}
if (typeof ref !== 'boolean') {
return PromiseReject(
new ERR_INVALID_ARG_TYPE(
'options.ref',
'boolean',
ref));
}

const { signal, ref = true } = options;

if (signal?.aborted) {
return PromiseReject(new AbortError(undefined, { cause: signal.reason }));
}

let oncancel;
const ret = new Promise((resolve, reject) => {
const immediate = new Immediate(resolve, [value]);
Expand All @@ -136,13 +131,29 @@ function setImmediate(value, options = kEmptyObject) {
}

async function* setInterval(after, value, options = kEmptyObject) {
validateObject(options, 'options');
try {
if (typeof after !== 'undefined') {
validateNumber(after, 'delay');
}

validateObject(options, 'options');

if (typeof options?.signal !== 'undefined') {
validateAbortSignal(options.signal, 'options.signal');
}

if (typeof options?.ref !== 'undefined') {
validateBoolean(options.ref, 'options.ref');
}
} catch (err) {
return PromiseReject(err);
}
Comment on lines +148 to +150
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why catch the error in the first place? Isn't returning a promise rejection strictly equivalent in an async function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I copied and pasted from setTimeout (which is not async) and missed that. Do you think we can leave like that or do we need a tiny fixing PR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would have been more "clean" to close that PR about adding an alias for setTimeout, and open a new one for the validation changes. That way, folks watching the repo would have had more chance to notice the change, and our wait time policy would have give enough time for folks to chime in (all the changes happen during a weekend, making it even more likely fewer folks had time to have a look). All in all, I don't really care if you do the follow-up PR with my suggestion, but IMO it's the "right" thing to do.


const { signal, ref = true } = options;
validateAbortSignal(signal, 'options.signal');
validateBoolean(ref, 'options.ref');

if (signal?.aborted)
if (signal?.aborted) {
throw new AbortError(undefined, { cause: signal?.reason });
}

let onCancel;
let interval;
Expand Down Expand Up @@ -216,7 +227,7 @@ class Scheduler {
wait(delay, options) {
if (!this[kScheduler])
throw new ERR_INVALID_THIS('Scheduler');
return setTimeout(delay, undefined, { signal: options?.signal });
return setTimeout(delay, undefined, options);
}
}

Expand Down
38 changes: 15 additions & 23 deletions test/parallel/test-timers-timeout-promisified.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,29 +63,21 @@ process.on('multipleResolves', common.mustNotCall());
}

{
Promise.all(
[1, '', false, Infinity].map(
(i) => assert.rejects(setPromiseTimeout(10, null, i), {
code: 'ERR_INVALID_ARG_TYPE'
})
)
).then(common.mustCall());

Promise.all(
[1, '', false, Infinity, null, {}].map(
(signal) => assert.rejects(setPromiseTimeout(10, null, { signal }), {
code: 'ERR_INVALID_ARG_TYPE'
})
)
).then(common.mustCall());

Promise.all(
[1, '', Infinity, null, {}].map(
(ref) => assert.rejects(setPromiseTimeout(10, null, { ref }), {
code: 'ERR_INVALID_ARG_TYPE'
})
)
).then(common.mustCall());
for (const delay of ['', false]) {
assert.rejects(() => setPromiseTimeout(delay, null, {}), /ERR_INVALID_ARG_TYPE/).then(common.mustCall());
}

for (const options of [1, '', false, Infinity]) {
assert.rejects(() => setPromiseTimeout(10, null, options), /ERR_INVALID_ARG_TYPE/).then(common.mustCall());
}

for (const signal of [1, '', false, Infinity, null, {}]) {
assert.rejects(() => setPromiseTimeout(10, null, { signal }), /ERR_INVALID_ARG_TYPE/).then(common.mustCall());
}

for (const ref of [1, '', Infinity, null, {}]) {
assert.rejects(() => setPromiseTimeout(10, null, { ref }), /ERR_INVALID_ARG_TYPE/).then(common.mustCall());
}
}

{
Expand Down
Loading