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.

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

[release-3.9] Pass throwIfNoEntry to fs.statSync #44582

Merged
merged 1 commit into from
Jun 15, 2021

Conversation

DanielRosenwasser
Copy link
Member

@DanielRosenwasser DanielRosenwasser commented Jun 14, 2021

Ports #41604 to TypeScript 3.9

See nodejs/node#35644 for why we have to do this to avoid potential regressions when users upgrade to Node 16.

Future versions of node will be able to return undefined, rather than
allocating and throwing an exception, when a file is not found.

See nodejs/node#33716
@typescript-bot
Copy link
Collaborator

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Jun 14, 2021
@fatcerberus
Copy link

fatcerberus commented Jun 14, 2021

See nodejs/node#35644 for why we have to do this to avoid potential regressions when users upgrade to Node 16.

I read the associated Node PR for fs.statSync but unless I missed something, it looks like it's an explicit flag that has to be passed in to get the new behavior? Otherwise you get the original (throwing) behavior. So I don't understand why a backport is necessary.

@DanielRosenwasser
Copy link
Member Author

@fatcerberus I don't think that's the case. My understanding is that if you don't pass in the flag, exceptions will be thrown with an arbitrarily large call stack capture. @amcasey understands it way better than I do though.

@amcasey
Copy link
Member

amcasey commented Jun 14, 2021

@fatcerberus, yes, the new flag is the mitigation. Prior to adding the flag, we were using a different mitigation (suggested by node) - we set the call stack depth to zero for the duration of the stat call. Node PR 35644 circumvents this mitigation by resetting the call stack depth to infinity while construction uvExceptions (like the one for non-existent files). More details: nodejs/node#35644 (comment)

@amcasey
Copy link
Member

amcasey commented Jun 14, 2021

The change looks good, but strictly speaking, we need to review all the statSync calls in case 3.9 had some that were dropped by the time I made the change in 4.2 (unlikely).

Edit: confirmed for 3.9, 4.0, and 4.1.

@fatcerberus
Copy link

fatcerberus commented Jun 14, 2021

@amcasey Thanks, I was assuming this was a mitigation for a breaking change in Node 16, then I got confused because it didn't seem like throwIfNoEntry was a breaking change (in the Node API). I didn't notice there was a different change causing a perf regression at the time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants