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

refactor: move to native fs.promises #314

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

pmmmwh
Copy link

@pmmmwh pmmmwh commented Jul 14, 2024

This is basically #284 but rebased with the latest changes.

While working on sindresorhus/del#161, I came across #303 - and can consistently reproduce that on Windows (there's a test in del to try 200 deletions in quick succession). From testing, it seemed to me this issue is related to timings around readdir, and seemingly node:fs/promises don't suffer from the same issue (tests consistently passes with this patch applied).

If it's acceptable, this would be a useful patch to v5 as well since Node.js v18 is still in maintenance mode.

@mark-wiemer
Copy link

@isaacs are you able to review this? Just want to make sure it doesn't fall through the cracks

@lukekarrys
Copy link

lukekarrys commented Oct 4, 2024

Here are the benchmarks (run from #319) before and after this change. Run in Node 22.4.0 on a MacBook Pro with an M3 Pro chip.

before

┌──────────────────┬──────────┬──────────┬────────┬──────────┬──────────┐
│ (index)          │ mean     │ median   │ stddev │ max      │ min      │
├──────────────────┼──────────┼──────────┼────────┼──────────┼──────────┤
│ native async     │ 291.019  │ 293.294  │ 38.554 │ 363.886  │ 234.82   │
│ windows async    │ 326.285  │ 309.799  │ 50.292 │ 447.157  │ 286.467  │
│ posix async      │ 328.639  │ 317.438  │ 20.119 │ 361.977  │ 306.322  │
│ native sync      │ 376.376  │ 377.487  │ 27.502 │ 422.319  │ 323.366  │
│ posix sync       │ 547.561  │ 537.363  │ 22.099 │ 586.83   │ 523.725  │
│ windows sync     │ 605.838  │ 607.56   │ 21.321 │ 637.648  │ 571.23   │
│ windows parallel │ 2515.488 │ 2515.486 │ 0.06   │ 2515.596 │ 2515.379 │
│ posix parallel   │ 2555.075 │ 2555.094 │ 0.083  │ 2555.208 │ 2554.954 │
│ native parallel  │ 2577.091 │ 2577.113 │ 0.072  │ 2577.149 │ 2576.922 │
└──────────────────┴──────────┴──────────┴────────┴──────────┴──────────┘

after

┌──────────────────┬──────────┬──────────┬────────┬──────────┬──────────┐
│ (index)          │ mean     │ median   │ stddev │ max      │ min      │
├──────────────────┼──────────┼──────────┼────────┼──────────┼──────────┤
│ native async     │ 325.626  │ 304.59   │ 49.495 │ 440.007  │ 286.464  │
│ posix async      │ 333.135  │ 330.679  │ 37.2   │ 415.842  │ 288.934  │
│ windows async    │ 337.623  │ 337.874  │ 47.999 │ 423.304  │ 254.826  │
│ native sync      │ 413.48   │ 412.375  │ 32.405 │ 472.207  │ 368.516  │
│ posix sync       │ 557.625  │ 565.108  │ 26.888 │ 595.113  │ 510.367  │
│ windows sync     │ 610.146  │ 612.455  │ 31.379 │ 642.311  │ 555.389  │
│ windows parallel │ 2351.348 │ 2351.39  │ 0.076  │ 2351.456 │ 2351.229 │
│ posix parallel   │ 2569.589 │ 2569.631 │ 0.076  │ 2569.645 │ 2569.43  │
│ native parallel  │ 2620.708 │ 2620.797 │ 0.177  │ 2620.926 │ 2620.417 │
└──────────────────┴──────────┴──────────┴────────┴──────────┴──────────┘

@lukekarrys
Copy link

lukekarrys commented Oct 6, 2024

I was able to adapt the test from del into an integration test for rimraf that throws EPERM errors on Windows CI consistently enough. Interestingly, I can still get the test to fail even with the patch from this PR applied: lukekarrys@fd7327d (actions run).

I think it happens less often but that's just anecdotal from running it in CI a bunch of times until it fails eventually.

So I still think the solution proposed in #303 will need to be applied to make this more bulletproof.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants