| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
In the thrown exception, `std::errc::permission_denied` is already treated as an `EPERM` error, but it is not included in one of the omittable errors when retrying the `RmSync` operation. This commit includes it. This also fixes the `retryDelay` calculation on Windows where the `retryDelay` is divided by `1000` but the win32's `Sleep` function takes the argument as an `ms` unit, dividing the supplied `ms` unit further to a much smaller delay. Signed-off-by: louiellan <louie.lou.llaneta@gmail.com>
|
This change seems OK, but there should be a regression test to confirm that and keep us safe in the future. |
Sorry, something went wrong.
|
@louiellan You can lock the file with JS: const fs = require('fs');
const UV_FS_O_EXLOCK = 0x10000000;
fs.openSync(process.argv[1], fs.constants.O_RDWR | UV_FS_O_EXLOCK);
process.stdout.write('locked');
setInterval(() => {}, 60_000);This test should cover your changes: 'use strict';
const common = require('../common');
if (!common.isWindows)
common.skip('Windows-specific: EPERM sharing-violation retry in rmSync');
const tmpdir = require('../common/tmpdir');
const assert = require('assert');
const { once } = require('events');
const { spawn } = require('child_process');
const fs = require('fs');
const path = require('path');
tmpdir.refresh();
// UV_FS_O_EXLOCK opens with share mode 0, so deletion fails with EPERM
// until this process kills the child.
const lockerScript = `
const fs = require('fs');
const UV_FS_O_EXLOCK = 0x10000000;
fs.openSync(process.argv[1], fs.constants.O_RDWR | UV_FS_O_EXLOCK);
process.stdout.write('locked');
setInterval(() => {}, 60_000);
`;
async function spawnLocker(file) {
const child = spawn(process.execPath, ['-e', lockerScript, file],
{ stdio: ['ignore', 'pipe', 'inherit'] });
const [data] = await once(child.stdout, 'data');
assert.strictEqual(data.toString(), 'locked');
return child;
}
// Sleep before retry i is i * retryDelay ms, so all retries take at least
// retryDelay * (1 + 2 + ... + maxRetries) ms.
function minRetryTime({ maxRetries, retryDelay }) {
return retryDelay * maxRetries * (maxRetries + 1) / 2;
}
function timedRmThrowsEPERM(dir, options) {
const start = Date.now();
assert.throws(() => {
fs.rmSync(dir, { recursive: true, ...options });
}, {
code: 'EPERM',
name: 'Error',
syscall: 'rm',
});
return Date.now() - start;
}
(async () => {
const dir = tmpdir.resolve('rm-eperm-retries');
const file = path.join(dir, 'locked.txt');
fs.mkdirSync(dir);
fs.writeFileSync(file, 'hello');
const child = await spawnLocker(file);
try {
// Proves the lock is effective: no retries means an immediate EPERM.
timedRmThrowsEPERM(dir, { maxRetries: 0, retryDelay: 0 });
assert.strictEqual(fs.existsSync(file), true);
const options = { maxRetries: 4, retryDelay: 100 };
const expected = minRetryTime(options); // 100+200+300+400 = 1000 ms.
const elapsed = timedRmThrowsEPERM(dir, options);
// Windows timer granularity may shave a few ms off each Sleep() call.
const slack = 16 * options.maxRetries;
assert.ok(elapsed >= expected - slack,
`rmSync() gave up after ${elapsed}ms; expected it to spend at ` +
`least ~${expected}ms on ${options.maxRetries} retries of ` +
`${options.retryDelay}ms escalating delay`);
// Catches unit confusion (e.g. seconds vs. milliseconds) in the delay.
assert.ok(elapsed < common.platformTimeout(expected * 10),
`rmSync() gave up after ${elapsed}ms; expected roughly ` +
`${expected}ms for ${options.maxRetries} retries`);
} finally {
child.kill();
}
await once(child, 'exit');
assert.strictEqual(fs.existsSync(file), true);
})().then(common.mustCall()); |
Sorry, something went wrong.
|
Thanks for the help❤️ @PickBas hadn't had the time for this yet |
Sorry, something went wrong.
In the thrown exception, `std::errc::permission_denied` is already treated as an `EPERM` error, but it is not included in one of the omittable errors when retrying the `RmSync` operation. This commit includes it. This also fixes the `retryDelay` calculation on Windows where the `retryDelay` is divided by `1000` but the win32's `Sleep` function takes the argument as an `ms` unit, dividing the supplied `ms` unit further to a much smaller delay. Signed-off-by: louiellan <louie.lou.llaneta@gmail.com> PR-URL: #64698 Fixes: #64016 Refs: #64017 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Stefan Stojanovic <stefan.stojanovic@janeasystems.com>
In the thrown exception, `std::errc::permission_denied` is already treated as an `EPERM` error, but it is not included in one of the omittable errors when retrying the `RmSync` operation. This commit includes it. This also fixes the `retryDelay` calculation on Windows where the `retryDelay` is divided by `1000` but the win32's `Sleep` function takes the argument as an `ms` unit, dividing the supplied `ms` unit further to a much smaller delay. Signed-off-by: louiellan <louie.lou.llaneta@gmail.com> PR-URL: #64698 Fixes: #64016 Refs: #64017 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Stefan Stojanovic <stefan.stojanovic@janeasystems.com>
In the thrown exception, `std::errc::permission_denied` is already treated as an `EPERM` error, but it is not included in one of the omittable errors when retrying the `RmSync` operation. This commit includes it. This also fixes the `retryDelay` calculation on Windows where the `retryDelay` is divided by `1000` but the win32's `Sleep` function takes the argument as an `ms` unit, dividing the supplied `ms` unit further to a much smaller delay. Signed-off-by: louiellan <louie.lou.llaneta@gmail.com> PR-URL: #64698 Fixes: #64016 Refs: #64017 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Stefan Stojanovic <stefan.stojanovic@janeasystems.com>
| Back | FazBrowse Home | New Git URL |
Fixes #64016
In the thrown exception, std::errc::permission_denied is already treated as an EPERM error, but it is not included in one of the omittable errors when retrying the RmSync operation. This commit includes it.
This also fixes the retryDelay calculation on Windows where the retryDelay is divided by 1000 but the win32's Sleep function takes the argument as an ms unit, dividing the supplied ms unit further to a much smaller delay.
I tried writing a test for this fix but node doesn't do file lock when opening a file, even if it's ran as another process, and running python as a child process in the test file yields race conditions (i.e., python does create the folder and the file to be locked but the js test fails because it can't do a rmSync because the file and the folder does not exist)
But I validated the reproducible steps mentioned in the issue with this fix, the rmSync now delays the retries and is around 15000ms, the same as the asynchronous fs.rm