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

[node] fs.existsSync throws when path is not present #19021

Closed
marvinhagemeister opened this issue May 7, 2023 · 10 comments · Fixed by #19104, denoland/rusty_v8#1229 or #19116
Closed

[node] fs.existsSync throws when path is not present #19021

marvinhagemeister opened this issue May 7, 2023 · 10 comments · Fixed by #19104, denoland/rusty_v8#1229 or #19116
Assignees
Labels
bug Something isn't working correctly needs investigation requires further investigation before determining if it is an issue or not node compat

Comments

@marvinhagemeister
Copy link
Contributor

Was playing around with vite to see what's missing to get it working. Ran into an issue where fs.existsSync unexpectedly throws when the passed path does not exist. This is particularly odd because our implementation contains a try/catch statement.

I'm unable to reproduce that in a smaller test case, so something must be different when more node modules are loaded.

Steps to reproduce:

  1. Clone https://github.com/marvinhagemeister/deno-exists-sync-bug
  2. Run deno task dev

Error output:

error: Uncaught NotFound: No such file or directory (os error 2): lstat '/project/node_modules/.vite'
    at Object.lstatSync (ext:deno_fs/30_fs.js:339:7)
    at Object.existsSync (ext:deno_node/_fs/_fs_exists.ts:28:14)
    at cleanupDepsCacheStaleDirs (file:///project/node_modules/.deno/vite@4.3.4/node_modules/vite/dist/node/chunks/dep-f7d05e3f.js:44801:18)
    at Timeout._onTimeout (file:///project/node_modules/.deno/vite@4.3.4/node_modules/vite/dist/node/chunks/dep-f7d05e3f.js:44103:26)
    at cb (ext:deno_node/internal/timers.mjs:37:46)
    at Object.action (ext:deno_web/02_timers.js:149:11)
    at handleTimerMacrotask (ext:deno_web/02_timers.js:66:10)
    at eventLoopTick (ext:core/01_core.js:187:21)

The error originates here https://github.com/vitejs/vite/blob/c63ba3fa08a64d75bfffa6885dc4c44875b9c5ba/packages/vite/src/node/optimizer/index.ts#L1335 .

@bartlomieju
Copy link
Member

I hit this problem earlier this week and I'm flabbergasted as to what is happening since that error is caught. I will try to fix it.

@aapoalas
Copy link
Collaborator

aapoalas commented May 7, 2023

I wonder if this isn't about the uncatchable fast errors again?

@bartlomieju
Copy link
Member

I wonder if this isn't about the uncatchable fast errors again?

@aapoalas that's a good pointer - changed lstatSync to get rid of fast op but it appears the problem still persist. It makes no sense to me what's going on here.

@aapoalas
Copy link
Collaborator

aapoalas commented May 7, 2023

I tried turning #[op] into #[op(fast)] for existsSync's op and the build still went off without a hitch. I'm not sure if that's the fast-attribute not correctly asserting that fast optimisation happened or if the op is successfully being optimised.

If it is, then likely the ops macro is doing it automatically even when not asked to: The macro is after all set to always create fast ops if possible.

@bartlomieju
Copy link
Member

@aapoalas I just changed return type of op_lstat_sync to be Result<(i32, SerializableStat), AnyError> - this should definitely kick us off the fast op because this return type is not fast call compatible.

That said, your observation is valid and I already raised it to @littledivy.

@bartlomieju bartlomieju added bug Something isn't working correctly node compat needs investigation requires further investigation before determining if it is an issue or not labels May 7, 2023
@bartlomieju bartlomieju self-assigned this May 9, 2023
@bartlomieju
Copy link
Member

I've been debugging this thing for the past 3 days.

I can reliably reproduce the problem and managed to trace it. The problem is that I cannot reproduce it with either deno release, I cannot reproduce it with a local release build, I can only reproduce it with local debug build.

@marvinhagemeister could you give more info how you reproduced it? Did you use official deno release or did you build it yourself? What OS and arch did this happen?

@bartlomieju
Copy link
Member

The root cause appears to be that the synchronous op_lstat_sync is called during responding from an async op to JavaScript in JsRuntime::do_single_realm_js_event_loop_tick.

op_lstat_sync throws an error which calls scope.throw_exception. The exception is caught in JavaScript, but somehow a TryCatchScope we have in do_single_realm_js_event_loop_tick reports that it has caught an error. This in turn causes to create a Rust error which bubbles up all the way to the main function and thus erroring the whole process.

I don't yet understand why TryCatchScope would do that and why is it only manifesting in debug builds for me.

@marvinhagemeister
Copy link
Contributor Author

marvinhagemeister commented May 10, 2023

@bartlomieju Can confirm that it only happens with debug builds for me. Release builds compiled from git as well as the official 1.33.2 are fine. I'm on macOS Ventura with an M1 chip. I can reproduce the error in debug builds on both M1 and M1 Pro chips.

@bartlomieju
Copy link
Member

Ooof, that's good to hear though not ideal! I'll try to chase down this problem, but happy to hear that's it's not the highest priority.

bartlomieju added a commit to denoland/rusty_v8 that referenced this issue May 12, 2023
…1227)

Reproduces #1226 and denoland/deno#19021

```
// Fails
$ V8_FROM_SOURCE=1 cargo test exception
// Passes
$ V8_FROM_SOURCE=1 cargo test --release exception
```

We bisected this and this problem first appeared with V8 v11.2 upgrade. After further
bisects we established that v8/v8@1f349da#diff-de75fc3e7b84373d94e18b4531827e8b749f9bbe05b59707e894e4e0ce2a1535
is the first V8 commit that causes this failure. However after more investigation we can't
find any reason why that particular commit might cause this problem.

It is only reproducible in debug build, but not release build. Current working theory
is that it is a Rust compiler bug as changing the optimization level for this code
makes the bug go away. This commit should be considered a band-aid that works
around the problem, but doesn't fix it completely. We are gonna go with it as it
unblocks un on day-to-day work, but ultimately we should track it down (or wait
for another Rust upgrade which might fix it?).

---------

Co-authored-by: Bert Belder <bertbelder@gmail.com>
bartlomieju added a commit that referenced this issue May 12, 2023
bartlomieju added a commit that referenced this issue May 12, 2023
@bartlomieju bartlomieju reopened this May 12, 2023
@bartlomieju
Copy link
Member

Unfortunately the above rusty_v8 upgrade didn't fix the problem, however I identified the problem and will cut another rusty_v8 release that will fix it later tonight.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working correctly needs investigation requires further investigation before determining if it is an issue or not node compat
Projects
None yet
3 participants