lib: make validateAbortSignal stricter

[VoltrexKeyva]

We should not force-check if the passed signal is NOT undefined and then proceed to check whether the value is an abort signal or not as the validator won't throw an error if the value is undefined as we straight up validate the values instead of checking if they're not undefined first. This unnecessary check can be done explicitly outside of the validator to only pass the value to the validator if its not undefined.

[targos]

SGTM, but I'm surprised that no other code needs to be changed.

[Linkgoron]

I'm not sure I see how this is an improvement, it adds a lot of code duplication to most places as usually abort signal is an optional parameter. Note that there are at least 5-10 more places you'd need to change.

[VoltrexKeyva]

I'm not sure I see how this is an improvement, it adds a lot of code duplication to most places as usually abort signal is an optional parameter. Note that there are at least 5-10 more places you'd need to change.

The main goal isn't really to apply any improvements as there's not much to improve, these changes are made to achieve consistency with the other validators and also allow the validator to throw an error if the value is not exactly the expected value instead of letting a specific value as undefined slip through the validation.

[Linkgoron]

I'm not sure I see how this is an improvement, it adds a lot of code duplication to most places as usually abort signal is an optional parameter. Note that there are at least 5-10 more places you'd need to change.

The main goal isn't really to apply any improvements as there's not much to improve, these changes are made to achieve consistency with the other validators and also allow the validator to throw an error if the value is not exactly the expected value instead of letting a specific value as undefined slip through the validation.

There are quite a few validators that allow null/undefined e.g. in child_process - validateTimeout, validateMaxBuffer and sanitizeKillSignal, or in dns validateHints - and there are others.

IMO a better solution would be to add a second parameter named required with a default value of false like validateBitLength in crypto - although I'm not sure if there are any places that use this specific method and require an abort-signal.

[VoltrexKeyva]

I'm not sure I see how this is an improvement, it adds a lot of code duplication to most places as usually abort signal is an optional parameter. Note that there are at least 5-10 more places you'd need to change.

The main goal isn't really to apply any improvements as there's not much to improve, these changes are made to achieve consistency with the other validators and also allow the validator to throw an error if the value is not exactly the expected value instead of letting a specific value as undefined slip through the validation.

There are quite a few validators that allow null/undefined e.g. in child_process - validateTimeout, validateMaxBuffer and sanitizeKillSignal, or in dns validateHints - and there are others.

IMO a better solution would be to add a second parameter named required with a default value of false like validateBitLength in crypto - although I'm not sure if there are any places that use this specific method and require an abort-signal.

Those custom validators are made and mostly used specifically for the subsystem they're created in, thus not making them a part of them main validators that are used all across the code base of node. The main goal here is to keep consistency of the main validators, and yes, there's a few places where they require an abort signal, such as the following; as this one creates it's own validateAbortSignal() validator instead of using the main one just for the sake of the main validateAbortSignal() validator not throwing an error for an undefined value and ignoring it:

node/lib/internal/streams/add-abort-signal.js

Lines 14 to 19 in e937662

	const validateAbortSignal = (signal, name) => {
	if (typeof signal !== 'object' ||
	!('aborted' in signal)) {
	throw new ERR_INVALID_ARG_TYPE(name, 'AbortSignal', signal);
	}
	};

[Linkgoron]

such as the following; as this one creates it's own validateAbortSignal() validator instead of using the main one just for the sake of the main validateAbortSignal() validator not throwing an error for an undefined value and ignoring it:

node/lib/internal/streams/add-abort-signal.js

Lines 14 to 19 in e937662

	const validateAbortSignal = (signal, name) => {
	if (typeof signal !== 'object' ||
	!('aborted' in signal)) {
	throw new ERR_INVALID_ARG_TYPE(name, 'AbortSignal', signal);
	}
	};

That is not the reason that it was copied. The reason that it's copied there is for inlining, which is stated above the method. There's a discussion about it here: #36061 (comment)

[VoltrexKeyva]

such as the following; as this one creates it's own validateAbortSignal() validator instead of using the main one just for the sake of the main validateAbortSignal() validator not throwing an error for an undefined value and ignoring it:

node/lib/internal/streams/add-abort-signal.js

Lines 14 to 19 in e937662

	const validateAbortSignal = (signal, name) => {
	if (typeof signal !== 'object' ||
	!('aborted' in signal)) {
	throw new ERR_INVALID_ARG_TYPE(name, 'AbortSignal', signal);
	}
	};

That is untrue, the reason that it's copied there is for inlining, which is stated above the method. There's a discussion about it here: #36061 (comment)

Oh, that is interesting; although the goal still stands for consistency with all the main validators, your idea of the inclusion of an optional parameter to gate the logic of whether or not the value is required is not entirely a bad approach, but it breaks the consistency with the other main validators until we change them all as well, which just changing this one seems to accomplish the goal, wdyt?

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/40583/