Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"changes": [
{
"packageName": "@rushstack/node-core-library",
"comment": "Make FileSystem.deleteFolder, FileSystem.deleteFolderAsync, FileSystem.ensureEmptyFolder, and FileSystem.ensureEmptyFolderAsync resilient to transient Windows EPERM/EBUSY/ENOTEMPTY errors by using native fs.rm with retries.",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"comment": "Make FileSystem.deleteFolder, FileSystem.deleteFolderAsync, FileSystem.ensureEmptyFolder, and FileSystem.ensureEmptyFolderAsync resilient to transient Windows EPERM/EBUSY/ENOTEMPTY errors by using native fs.rm with retries.",
"comment": "Make `FileSystem.deleteFolder`, `FileSystem.deleteFolderAsync`, `FileSystem.ensureEmptyFolder`, and `FileSystem.ensureEmptyFolderAsync` resilient to transient Windows `EPERM`/`EBUSY`/`ENOTEMPTY` errors by using native `fs.rm` with retries.",

"type": "patch"
}
],
"packageName": "@rushstack/node-core-library"
}
44 changes: 37 additions & 7 deletions libraries/node-core-library/src/FileSystem.ts
Original file line number Diff line number Diff line change
Expand Up @@ -722,14 +722,14 @@ export class FileSystem {

/**
* Deletes a folder, including all of its contents.
* Behind the scenes is uses `fs-extra.removeSync()`.
* Behind the scenes it uses `fs.rmSync()`.
* @remarks
* Does not throw if the folderPath does not exist.
* @param folderPath - The absolute or relative path to the folder which should be deleted.
*/
public static deleteFolder(folderPath: string): void {
FileSystem._wrapException(() => {
fsx.removeSync(folderPath);
fs.rmSync(folderPath, { recursive: true, force: true, maxRetries: 3, retryDelay: 100 });

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stick this options object in a shared constant?

});
}

Expand All @@ -738,30 +738,60 @@ export class FileSystem {
*/
public static async deleteFolderAsync(folderPath: string): Promise<void> {
await FileSystem._wrapExceptionAsync(() => {
return fsx.remove(folderPath);
return fsPromises.rm(folderPath, { recursive: true, force: true, maxRetries: 3, retryDelay: 100 });
});
}

/**
* Deletes the content of a folder, but not the folder itself. Also ensures the folder exists.
* Behind the scenes it uses `fs-extra.emptyDirSync()`.
* Each child entry is removed via `fs.rmSync()`.
* @remarks
* This is a workaround for a common race condition, where the virus scanner holds a lock on the folder
* for a brief period after it was deleted, causing EBUSY errors for any code that tries to recreate the folder.
* @param folderPath - The absolute or relative path to the folder which should have its contents deleted.
*/
public static ensureEmptyFolder(folderPath: string): void {
FileSystem._wrapException(() => {
fsx.emptyDirSync(folderPath);
let items: string[];
try {
items = fsx.readdirSync(folderPath);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use FileSystem.readFolderItems and handle subfolders.

} catch {
fsx.ensureDirSync(folderPath);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specifically check for a not-exist error using FileSystem.isNotExistError, and otherwise re-throw

return;
}
for (const item of items) {
fs.rmSync(nodeJsPath.join(folderPath, item), {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forward shashes work fine on Windows.

Suggested change
fs.rmSync(nodeJsPath.join(folderPath, item), {
fs.rmSync(`${folderPath}/${item}`, {

recursive: true,
force: true,
maxRetries: 3,
retryDelay: 100
});
}
});
}

/**
* An async version of {@link FileSystem.ensureEmptyFolder}.
*/
public static async ensureEmptyFolderAsync(folderPath: string): Promise<void> {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same feedback as the sync version applies here.

await FileSystem._wrapExceptionAsync(() => {
return fsx.emptyDir(folderPath);
await FileSystem._wrapExceptionAsync(async () => {
let items: string[];
try {
items = await fsx.readdir(folderPath);
} catch {
await fsx.ensureDir(folderPath);
return;
}
await Promise.all(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Limit concurrency here using Async.forEachAsync. It may be worthwhile to recursive enumerate files and sort them by depth, and delete from deepest-in.

items.map((item) =>
fsPromises.rm(nodeJsPath.join(folderPath, item), {
recursive: true,
force: true,
maxRetries: 3,
retryDelay: 100
})
)
);
});
}

Expand Down
56 changes: 56 additions & 0 deletions libraries/node-core-library/src/test/FileSystem.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,62 @@ describe(FileSystem.name, () => {
});
});

describe(FileSystem.ensureEmptyFolder.name, () => {
const tempDir: string = `${testTempFolder}/ensureEmptyFolder`;

afterEach(() => {
FileSystem.deleteFolder(tempDir);
});

test('empties an existing folder but keeps the folder itself', () => {
FileSystem.ensureFolder(`${tempDir}/sub`);
FileSystem.writeFile(`${tempDir}/a.txt`, 'a');
FileSystem.writeFile(`${tempDir}/sub/b.txt`, 'b');

FileSystem.ensureEmptyFolder(tempDir);

expect(fs.existsSync(tempDir)).toBe(true);
expect(fs.readdirSync(tempDir)).toEqual([]);
});

test('creates the folder when it does not exist', () => {
expect(fs.existsSync(tempDir)).toBe(false);

FileSystem.ensureEmptyFolder(tempDir);

expect(fs.existsSync(tempDir)).toBe(true);
expect(fs.readdirSync(tempDir)).toEqual([]);
});
});

describe(FileSystem.ensureEmptyFolderAsync.name, () => {
const tempDir: string = `${testTempFolder}/ensureEmptyFolderAsync`;

afterEach(async () => {
await FileSystem.deleteFolderAsync(tempDir);
});

test('empties an existing folder but keeps the folder itself', async () => {
await FileSystem.ensureFolderAsync(`${tempDir}/sub`);
await FileSystem.writeFileAsync(`${tempDir}/a.txt`, 'a');
await FileSystem.writeFileAsync(`${tempDir}/sub/b.txt`, 'b');

await FileSystem.ensureEmptyFolderAsync(tempDir);

expect(fs.existsSync(tempDir)).toBe(true);
expect(fs.readdirSync(tempDir)).toEqual([]);
});

test('creates the folder when it does not exist', async () => {
expect(fs.existsSync(tempDir)).toBe(false);

await FileSystem.ensureEmptyFolderAsync(tempDir);

expect(fs.existsSync(tempDir)).toBe(true);
expect(fs.readdirSync(tempDir)).toEqual([]);
});
});

describe(FileSystem.createWriteStreamAsync.name, () => {
const tempDir: string = `${testTempFolder}/createWriteStreamAsync`;

Expand Down