Skip to content

Commit a1c783c

Browse files
bpaserobpasero
authored
fs - retry rename on Windows and adopt (microsoft#188899)
* fs - retry `rename` on Windows and adopt * return early if target is not a file * proper throw error * align move and rename * fix compile * tests
1 parent 17ad316 commit a1c783c

File tree

13 files changed

+109
-54
lines changed

13 files changed

+109
-54
lines changed

‎src/bootstrap.js

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
const Module = typeof require === 'function' ? require('module') : undefined;
2424
const path = typeof require === 'function' ? require('path') : undefined;
2525
const fs = typeof require === 'function' ? require('fs') : undefined;
26+
const util = typeof require === 'function' ? require('util') : undefined;
2627

2728
//#region global bootstrapping
2829

@@ -222,8 +223,8 @@
222223
return ipcRenderer.invoke('vscode:readNlsFile', ...pathSegments);
223224
}
224225

225-
if (fs && path) {
226-
return (await fs.promises.readFile(path.join(...pathSegments))).toString();
226+
if (fs && path && util) {
227+
return (await util.promisify(fs.readFile)(path.join(...pathSegments))).toString();
227228
}
228229

229230
throw new Error('Unsupported operation (read NLS files)');
@@ -240,8 +241,8 @@
240241
return ipcRenderer.invoke('vscode:writeNlsFile', path, content);
241242
}
242243

243-
if (fs) {
244-
return fs.promises.writeFile(path, content);
244+
if (fs && util) {
245+
return util.promisify(fs.writeFile)(path, content);
245246
}
246247

247248
throw new Error('Unsupported operation (write NLS files)');

‎src/vs/base/node/pfs.ts

Lines changed: 51 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
import * as fs from 'fs';
77
import { tmpdir } from 'os';
88
import { promisify } from 'util';
9-
import { ResourceQueue } from 'vs/base/common/async';
9+
import { ResourceQueue, timeout } from 'vs/base/common/async';
1010
import { isEqualOrParent, isRootOrDriveLetter, randomPath } from 'vs/base/common/extpath';
1111
import { normalizeNFC } from 'vs/base/common/normalization';
1212
import { join } from 'vs/base/common/path';
@@ -491,16 +491,24 @@ function ensureWriteOptions(options?: IWriteFileOptions): IEnsuredWriteFileOptio
491491
/**
492492
* A drop-in replacement for `fs.rename` that:
493493
* - allows to move across multiple disks
494+
* - attempts to retry the operation for certain error codes on Windows
494495
*/
495-
async function move(source: string, target: string): Promise<void> {
496+
async function rename(source: string, target: string, windowsRetryTimeout: number | false = 60000 /* matches graceful-fs */): Promise<void> {
496497
if (source === target) {
497498
return; // simulate node.js behaviour here and do a no-op if paths match
498499
}
499500

500501
try {
501-
await Promises.rename(source, target);
502+
if (isWindows && typeof windowsRetryTimeout === 'number') {
503+
// On Windows, a rename can fail when either source or target
504+
// is locked by AV software. We do leverage graceful-fs to iron
505+
// out these issues, however in case the target file exists,
506+
// graceful-fs will immediately return without retry for fs.rename().
507+
await renameWithRetry(source, target, Date.now(), windowsRetryTimeout);
508+
} else {
509+
await promisify(fs.rename)(source, target);
510+
}
502511
} catch (error) {
503-
504512
// In two cases we fallback to classic copy and delete:
505513
//
506514
// 1.) The EXDEV error indicates that source and target are on different devices
@@ -518,6 +526,44 @@ async function move(source: string, target: string): Promise<void> {
518526
}
519527
}
520528

529+
async function renameWithRetry(source: string, target: string, startTime: number, retryTimeout: number, attempt = 0): Promise<void> {
530+
try {
531+
return await promisify(fs.rename)(source, target);
532+
} catch (error) {
533+
if (error.code !== 'EACCES' && error.code !== 'EPERM' && error.code !== 'EBUSY') {
534+
throw error; // only for errors we think are temporary
535+
}
536+
537+
if (Date.now() - startTime >= retryTimeout) {
538+
console.error(`[node.js fs] rename failed after ${attempt} retries with error: ${error}`);
539+
540+
throw error; // give up after configurable timeout
541+
}
542+
543+
if (attempt === 0) {
544+
let abortRetry = false;
545+
try {
546+
const { stat } = await SymlinkSupport.stat(target);
547+
if (!stat.isFile()) {
548+
abortRetry = true; // if target is not a file, EPERM error may be raised and we should not attempt to retry
549+
}
550+
} catch (error) {
551+
// Ignore
552+
}
553+
554+
if (abortRetry) {
555+
throw error;
556+
}
557+
}
558+
559+
// Delay with incremental backoff up to 100ms
560+
await timeout(Math.min(100, attempt * 10));
561+
562+
// Attempt again
563+
return renameWithRetry(source, target, startTime, retryTimeout, attempt + 1);
564+
}
565+
}
566+
521567
interface ICopyPayload {
522568
readonly root: { source: string; target: string };
523569
readonly options: { preserveSymlinks: boolean };
@@ -694,7 +740,6 @@ export const Promises = new class {
694740
get fdatasync() { return promisify(fs.fdatasync); }
695741
get truncate() { return promisify(fs.truncate); }
696742

697-
get rename() { return promisify(fs.rename); }
698743
get copyFile() { return promisify(fs.copyFile); }
699744

700745
get open() { return promisify(fs.open); }
@@ -733,7 +778,7 @@ export const Promises = new class {
733778

734779
get rm() { return rimraf; }
735780

736-
get move() { return move; }
781+
get rename() { return rename; }
737782
get copy() { return copy; }
738783

739784
//#endregion

‎src/vs/base/parts/storage/node/storage.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -274,7 +274,7 @@ export class SQLiteStorageDatabase implements IStorageDatabase {
274274
try {
275275
await Promises.unlink(path);
276276
try {
277-
await Promises.rename(this.toBackupPath(path), path);
277+
await Promises.rename(this.toBackupPath(path), path, false /* no retry */);
278278
} catch (error) {
279279
// ignore
280280
}

‎src/vs/base/test/node/pfs/pfs.test.ts

Lines changed: 30 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -173,7 +173,7 @@ flakySuite('PFS', function () {
173173
assert.ok(!fs.existsSync(testDir));
174174
});
175175

176-
test('copy, move and delete', async () => {
176+
test('copy, rename and delete', async () => {
177177
const sourceDir = FileAccess.asFileUri('vs/base/test/node/pfs/fixtures').fsPath;
178178
const parentDir = join(tmpdir(), 'vsctests', 'pfs');
179179
const targetDir = randomPath(parentDir);
@@ -188,7 +188,7 @@ flakySuite('PFS', function () {
188188
assert.ok(fs.statSync(join(targetDir, 'examples')).isDirectory());
189189
assert.ok(fs.existsSync(join(targetDir, 'examples', 'small.jxs')));
190190

191-
await Promises.move(targetDir, targetDir2);
191+
await Promises.rename(targetDir, targetDir2);
192192

193193
assert.ok(!fs.existsSync(targetDir));
194194
assert.ok(fs.existsSync(targetDir2));
@@ -198,7 +198,34 @@ flakySuite('PFS', function () {
198198
assert.ok(fs.statSync(join(targetDir2, 'examples')).isDirectory());
199199
assert.ok(fs.existsSync(join(targetDir2, 'examples', 'small.jxs')));
200200

201-
await Promises.move(join(targetDir2, 'index.html'), join(targetDir2, 'index_moved.html'));
201+
await Promises.rename(join(targetDir2, 'index.html'), join(targetDir2, 'index_moved.html'));
202+
203+
assert.ok(!fs.existsSync(join(targetDir2, 'index.html')));
204+
assert.ok(fs.existsSync(join(targetDir2, 'index_moved.html')));
205+
206+
await Promises.rm(parentDir);
207+
208+
assert.ok(!fs.existsSync(parentDir));
209+
});
210+
211+
test('rename without retry', async () => {
212+
const sourceDir = FileAccess.asFileUri('vs/base/test/node/pfs/fixtures').fsPath;
213+
const parentDir = join(tmpdir(), 'vsctests', 'pfs');
214+
const targetDir = randomPath(parentDir);
215+
const targetDir2 = randomPath(parentDir);
216+
217+
await Promises.copy(sourceDir, targetDir, { preserveSymlinks: true });
218+
await Promises.rename(targetDir, targetDir2, false);
219+
220+
assert.ok(!fs.existsSync(targetDir));
221+
assert.ok(fs.existsSync(targetDir2));
222+
assert.ok(fs.existsSync(join(targetDir2, 'index.html')));
223+
assert.ok(fs.existsSync(join(targetDir2, 'site.css')));
224+
assert.ok(fs.existsSync(join(targetDir2, 'examples')));
225+
assert.ok(fs.statSync(join(targetDir2, 'examples')).isDirectory());
226+
assert.ok(fs.existsSync(join(targetDir2, 'examples', 'small.jxs')));
227+
228+
await Promises.rename(join(targetDir2, 'index.html'), join(targetDir2, 'index_moved.html'), false);
202229

203230
assert.ok(!fs.existsSync(join(targetDir2, 'index.html')));
204231
assert.ok(fs.existsSync(join(targetDir2, 'index_moved.html')));

‎src/vs/platform/backup/electron-main/backupMainService.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,7 @@ export class BackupMainService implements IBackupMainService {
130130
// When we have data to migrate from, move it over to the target location
131131
if (await Promises.exists(moveFromPath)) {
132132
try {
133-
await Promises.rename(moveFromPath, backupPath);
133+
await Promises.rename(moveFromPath, backupPath, false /* no retry */);
134134
} catch (error) {
135135
this.logService.error(`Backup: Could not move backup folder to new location: ${error.toString()}`);
136136
}
@@ -285,7 +285,7 @@ export class BackupMainService implements IBackupMainService {
285285
// Rename backupPath to new empty window backup path
286286
const newEmptyWindowBackupPath = join(this.backupHome, newEmptyWindowBackupInfo.backupFolder);
287287
try {
288-
await Promises.rename(backupPath, newEmptyWindowBackupPath);
288+
await Promises.rename(backupPath, newEmptyWindowBackupPath, false /* no retry */);
289289
} catch (error) {
290290
this.logService.error(`Backup: Could not rename backup folder: ${error.toString()}`);
291291
return false;

‎src/vs/platform/extensionManagement/node/extensionDownloader.ts

Lines changed: 1 addition & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import { Promises } from 'vs/base/common/async';
77
import { getErrorMessage } from 'vs/base/common/errors';
88
import { Disposable } from 'vs/base/common/lifecycle';
99
import { Schemas } from 'vs/base/common/network';
10-
import { isWindows } from 'vs/base/common/platform';
1110
import { joinPath } from 'vs/base/common/resources';
1211
import * as semver from 'vs/base/common/semver/semver';
1312
import { isBoolean } from 'vs/base/common/types';
@@ -129,7 +128,7 @@ export class ExtensionsDownloader extends Disposable {
129128

130129
try {
131130
// Rename temp location to original
132-
await this.rename(tempLocation, location, Date.now() + (2 * 60 * 1000) /* Retry for 2 minutes */);
131+
await FSPromises.rename(tempLocation.fsPath, location.fsPath, 2 * 60 * 1000 /* Retry for 2 minutes */);
133132
} catch (error) {
134133
try {
135134
await this.fileService.del(tempLocation);
@@ -148,18 +147,6 @@ export class ExtensionsDownloader extends Disposable {
148147
await this.fileService.del(location);
149148
}
150149

151-
private async rename(from: URI, to: URI, retryUntil: number): Promise<void> {
152-
try {
153-
await FSPromises.rename(from.fsPath, to.fsPath);
154-
} catch (error) {
155-
if (isWindows && error && error.code === 'EPERM' && Date.now() < retryUntil) {
156-
this.logService.info(`Failed renaming ${from} to ${to} with 'EPERM' error. Trying again...`);
157-
return this.rename(from, to, retryUntil);
158-
}
159-
throw error;
160-
}
161-
}
162-
163150
private async cleanUp(): Promise<void> {
164151
try {
165152
if (!(await this.fileService.exists(this.extensionsDownloadDir))) {

‎src/vs/platform/extensionManagement/node/extensionManagementService.ts

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@ import { Disposable } from 'vs/base/common/lifecycle';
1515
import { ResourceSet } from 'vs/base/common/map';
1616
import { Schemas } from 'vs/base/common/network';
1717
import * as path from 'vs/base/common/path';
18-
import { isWindows } from 'vs/base/common/platform';
1918
import { joinPath } from 'vs/base/common/resources';
2019
import * as semver from 'vs/base/common/semver/semver';
2120
import { isBoolean, isUndefined } from 'vs/base/common/types';
@@ -508,7 +507,7 @@ export class ExtensionsScanner extends Disposable {
508507
// Rename
509508
try {
510509
this.logService.trace(`Started renaming the extension from ${tempLocation.fsPath} to ${extensionLocation.fsPath}`);
511-
await this.rename(extensionKey, tempLocation.fsPath, extensionLocation.fsPath, Date.now() + (2 * 60 * 1000) /* Retry for 2 minutes */);
510+
await this.rename(tempLocation.fsPath, extensionLocation.fsPath);
512511
this.logService.info('Renamed to', extensionLocation.fsPath);
513512
} catch (error) {
514513
if (error.code === 'ENOTEMPTY') {
@@ -606,7 +605,7 @@ export class ExtensionsScanner extends Disposable {
606605
private async deleteExtensionFromLocation(id: string, location: URI, type: string): Promise<void> {
607606
this.logService.trace(`Deleting ${type} extension from disk`, id, location.fsPath);
608607
const renamedLocation = this.uriIdentityService.extUri.joinPath(this.uriIdentityService.extUri.dirname(location), `${this.uriIdentityService.extUri.basename(location)}.${hash(generateUuid()).toString(16)}${DELETED_FOLDER_POSTFIX}`);
609-
await this.rename({ id }, location.fsPath, renamedLocation.fsPath, Date.now() + (2 * 60 * 1000) /* Retry for 2 minutes */);
608+
await this.rename(location.fsPath, renamedLocation.fsPath);
610609
await this.fileService.del(renamedLocation, { recursive: true });
611610
this.logService.info(`Deleted ${type} extension from disk`, id, location.fsPath);
612611
}
@@ -643,14 +642,10 @@ export class ExtensionsScanner extends Disposable {
643642
});
644643
}
645644

646-
private async rename(identifier: IExtensionIdentifier, extractPath: string, renamePath: string, retryUntil: number): Promise<void> {
645+
private async rename(extractPath: string, renamePath: string): Promise<void> {
647646
try {
648-
await pfs.Promises.rename(extractPath, renamePath);
647+
await pfs.Promises.rename(extractPath, renamePath, 2 * 60 * 1000 /* Retry for 2 minutes */);
649648
} catch (error) {
650-
if (isWindows && error && error.code === 'EPERM' && Date.now() < retryUntil) {
651-
this.logService.info(`Failed renaming ${extractPath} to ${renamePath} with 'EPERM' error. Trying again...`, identifier.id);
652-
return this.rename(identifier, extractPath, renamePath, retryUntil);
653-
}
654649
throw new ExtensionManagementError(error.message || nls.localize('renameError', "Unknown error while renaming {0} to {1}", extractPath, renamePath), error.code || ExtensionManagementErrorCode.Rename);
655650
}
656651
}

‎src/vs/platform/files/node/diskFileSystemProvider.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -640,8 +640,8 @@ export class DiskFileSystemProvider extends AbstractDiskFileSystemProvider imple
640640
// Validate the move operation can perform
641641
await this.validateMoveCopy(from, to, 'move', opts.overwrite);
642642

643-
// Move
644-
await Promises.move(fromFilePath, toFilePath);
643+
// Rename
644+
await Promises.rename(fromFilePath, toFilePath);
645645
} catch (error) {
646646

647647
// Rewrite some typical errors that can happen especially around symlinks

‎src/vs/platform/files/test/node/nodejsWatcher.integrationTest.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -252,7 +252,7 @@ import { FileAccess } from 'vs/base/common/network';
252252

253253
// Move file
254254
changeFuture = awaitEvent(watcher, filePath, FileChangeType.DELETED);
255-
await Promises.move(filePath, `${filePath}-moved`);
255+
await Promises.rename(filePath, `${filePath}-moved`);
256256
await changeFuture;
257257
});
258258

‎src/vs/platform/remote/node/wsl.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55

66
import * as os from 'os';
77
import * as cp from 'child_process';
8-
import { promises as fs } from 'fs';
8+
import { Promises } from 'vs/base/node/pfs';
99
import * as path from 'path';
1010

1111
let hasWSLFeaturePromise: Promise<boolean> | undefined;
@@ -33,7 +33,7 @@ async function testWSLFeatureInstalled(): Promise<boolean> {
3333
const dllPath = getLxssManagerDllPath();
3434
if (dllPath) {
3535
try {
36-
if ((await fs.stat(dllPath)).isFile()) {
36+
if ((await Promises.stat(dllPath)).isFile()) {
3737
return true;
3838
}
3939
} catch (e) {

0 commit comments

Comments
 (0)