fs: validate file mode from cpp

[anonrig]

This is somewhat similar to #49970, with major modifications to input validation to avoid making this a breaking change due to the input validation on Infinity and NaN on the original PR. Credit to @andremralves for the initial push.

The original PR was blocked due to @tniessen's following comment:

Unless I'm missing something, some functions now don't throw synchronously anymore but instead report the validation error to the callback, which seems unusual for argument validation. I am not sure if that's desirable (or if we treat these things as semver-major).

This does not apply because we throw the error synchronously from the C++ side. A similar work was done on getValidatedFd couple of months ago. (Ref: #51027)

In promisified calls the error was thrown inside an async function, and it is thrown the similar way. For non promisified calls, right now the error is thrown still synchronously but inside the C++ file before any callback execution is done.

The only difference is the order of errors. For example, previously for a callback version of access we were checking the validity of mode and later the validity of callback, right now mode is checked later. According to the documentation, and if I'm not mistaken, this is not a breaking change.

cc @nodejs/fs @tniessen @nodejs/performance @nodejs/cpp-reviewers

---

And most importantly, I'm back.

[H4ad]

Good to have you back! Just leave some non-blocking comments.

[nodejs-github-bot]

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

[RafaelGSS]

For example, previously for a callback version of access we were checking the validity of mode and later the validity of callback, right now mode is checked later

Honestly, I have a feeling this should be considered a semver-major. While we do not consider changing the error message to semver-major, changing the order of ERR_CODE should be considered a semver-major.

This, however, should impact only tests (error conditions). So, if everybody agrees, it shouldn't be that bad to land as a non-semver-major PR (mainly on non-LTS lines).

---

Also, I suspect we are only changing it for performance purposes, right? Would you be able to share benchmarks (preferably in a dedicated machine)?

[anonrig]

Honestly, I have a feeling this should be considered a semver-major. While we do not consider changing the error message to semver-major, changing the order of ERR_CODE should be considered a semver-major.

@RafaelGSS I'm -0.5 on making this a semver-major change, because of the past pull-requests. For example, similar PR was landed (#51027) without semver-major. Also, it's extremely easy to make this change since by just moving the order of any validate operation, you can easily break the order.

If we follow this path, calling this major change, we should document this before landing this pull-request. But I believe we shouldn't make a claim to preserve the order of errors since it will stall a lot of optimization opportunities by max 1 year (release of major).

Also, I suspect we are only changing it for performance purposes, right? Would you be able to share benchmarks (preferably in a dedicated machine)?

@RafaelGSS The original PR has some benchmark results. I don't have access to a dedicated machine to back this claim, but the results from original PR stands: #49970

[nodejs-github-bot]

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

[mcollina]

lgtm

[nodejs-github-bot]

Landed in 3ec20f2