isaacs / rimraf

A `rm -rf` util for nodejs
ISC License
5.66k stars 252 forks source link

refactor: move to native fs.promises #314

Open pmmmwh opened 3 months ago

pmmmwh commented 3 months ago

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 commented 2 months ago

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

lukekarrys commented 1 month ago

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 commented 1 month ago

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: https://github.com/lukekarrys/rimraf/pull/1/commits/fd7327d88979d45af8b7f4005aeea65db612c9ac (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.