From 7f838f9d6f78ffed33f104d965c168c1dc33d0e9 Mon Sep 17 00:00:00 2001 From: Ian Clanton-Thuon Date: Thu, 12 Sep 2024 16:50:20 -0700 Subject: [PATCH] [node-core-library] Fix an issue where attempting to acquire multiple `LockFile`s at the same time on POSIX would cause the second to immediately be acquired without releasing the first. (#4920) * Introduce a test that's broken on mac and linux for acquiring two locks on the same resource at the same time. * Rename LockFile.acquire to Lockfile.acquireAsync. * Add support for multiple locks in the same process. * Refactor @rushstack/rush-azure-storage-build-cache-plugin to delay locking the CredentialCache file until the credential has been acquired. * Apply suggestions from code review Co-authored-by: Daniel <3473356+D4N14L@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: David Michon * Refactor the retryLoop to be a simple while loop. * Update libraries/node-core-library/src/LockFile.ts Co-authored-by: David Michon * Revert changes to Rush. * Rush change. --------- Co-authored-by: Daniel <3473356+D4N14L@users.noreply.github.com> Co-authored-by: David Michon --- .../rush/lockfile-fixes_2024-09-12-22-36.json | 10 ++ .../lockfile-fixes_2024-09-12-06-22.json | 10 ++ .../lockfile-fixes_2024-09-12-06-25.json | 10 ++ common/reviews/api/node-core-library.api.md | 2 + libraries/node-core-library/src/LockFile.ts | 112 ++++++++---- .../src/test/LockFile.test.ts | 163 +++++++++++------- libraries/rush-lib/src/logic/Autoinstaller.ts | 2 +- .../rush-lib/src/logic/CredentialCache.ts | 2 +- .../logic/installManager/InstallHelpers.ts | 2 +- 9 files changed, 221 insertions(+), 92 deletions(-) create mode 100644 common/changes/@microsoft/rush/lockfile-fixes_2024-09-12-22-36.json create mode 100644 common/changes/@rushstack/node-core-library/lockfile-fixes_2024-09-12-06-22.json create mode 100644 common/changes/@rushstack/node-core-library/lockfile-fixes_2024-09-12-06-25.json diff --git a/common/changes/@microsoft/rush/lockfile-fixes_2024-09-12-22-36.json b/common/changes/@microsoft/rush/lockfile-fixes_2024-09-12-22-36.json new file mode 100644 index 00000000000..bd7ff97cb34 --- /dev/null +++ b/common/changes/@microsoft/rush/lockfile-fixes_2024-09-12-22-36.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@microsoft/rush", + "comment": "", + "type": "none" + } + ], + "packageName": "@microsoft/rush" +} \ No newline at end of file diff --git a/common/changes/@rushstack/node-core-library/lockfile-fixes_2024-09-12-06-22.json b/common/changes/@rushstack/node-core-library/lockfile-fixes_2024-09-12-06-22.json new file mode 100644 index 00000000000..cefc1763275 --- /dev/null +++ b/common/changes/@rushstack/node-core-library/lockfile-fixes_2024-09-12-06-22.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@rushstack/node-core-library", + "comment": "Rename `LockFile.acquire` to `Lockfile.acquireAsync`.", + "type": "minor" + } + ], + "packageName": "@rushstack/node-core-library" +} \ No newline at end of file diff --git a/common/changes/@rushstack/node-core-library/lockfile-fixes_2024-09-12-06-25.json b/common/changes/@rushstack/node-core-library/lockfile-fixes_2024-09-12-06-25.json new file mode 100644 index 00000000000..aca72b934f1 --- /dev/null +++ b/common/changes/@rushstack/node-core-library/lockfile-fixes_2024-09-12-06-25.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@rushstack/node-core-library", + "comment": "Fix an issue where attempting to acquire multiple `LockFile`s at the same time on POSIX would cause the second to immediately be acquired without releasing the first.", + "type": "patch" + } + ], + "packageName": "@rushstack/node-core-library" +} \ No newline at end of file diff --git a/common/reviews/api/node-core-library.api.md b/common/reviews/api/node-core-library.api.md index 8e2f99a6cd1..a11e5a042da 100644 --- a/common/reviews/api/node-core-library.api.md +++ b/common/reviews/api/node-core-library.api.md @@ -719,7 +719,9 @@ export type LegacyCallback = (error: TError | null | undefined, // @public export class LockFile { + // @deprecated (undocumented) static acquire(resourceFolder: string, resourceName: string, maxWaitMs?: number): Promise; + static acquireAsync(resourceFolder: string, resourceName: string, maxWaitMs?: number): Promise; get dirtyWhenAcquired(): boolean; get filePath(): string; static getLockFilePath(resourceFolder: string, resourceName: string, pid?: number): string; diff --git a/libraries/node-core-library/src/LockFile.ts b/libraries/node-core-library/src/LockFile.ts index 7181ced69cd..6b73339c8eb 100644 --- a/libraries/node-core-library/src/LockFile.ts +++ b/libraries/node-core-library/src/LockFile.ts @@ -58,7 +58,7 @@ export function getProcessStartTimeFromProcStat(stat: string): string | undefine // In theory, the representations of start time returned by `cat /proc/[pid]/stat` and `ps -o lstart` can change // while the system is running, but we assume this does not happen. // So the caller can safely use this value as part of a unique process id (on the machine, without comparing - // accross reboots). + // across reboots). return startTimeJiffies; } @@ -121,7 +121,7 @@ export function getProcessStartTime(pid: number): string | undefined { const psSplit: string[] = psStdout.split('\n'); - // successfuly able to run "ps", but no process was found + // successfully able to run "ps", but no process was found if (psSplit[1] === '') { return undefined; } @@ -136,6 +136,10 @@ export function getProcessStartTime(pid: number): string | undefined { throw new Error(`Unexpected output from the "ps" command`); } +// A set of locks that currently exist in the current process, to be used when +// multiple locks are acquired in the same process. +const IN_PROC_LOCKS: Set = new Set(); + /** * The `LockFile` implements a file-based mutex for synchronizing access to a shared resource * between multiple Node.js processes. It is not recommended for synchronization solely within @@ -157,6 +161,8 @@ export class LockFile { this._fileWriter = fileWriter; this._filePath = filePath; this._dirtyWhenAcquired = dirtyWhenAcquired; + + IN_PROC_LOCKS.add(filePath); } /** @@ -174,17 +180,24 @@ export class LockFile { if (!resourceName.match(/^[a-zA-Z0-9][a-zA-Z0-9-.]+[a-zA-Z0-9]$/)) { throw new Error( `The resource name "${resourceName}" is invalid.` + - ` It must be an alphanumberic string with only "-" or "." It must start with an alphanumeric character.` + ` It must be an alphanumeric string with only "-" or "." It must start and end with an alphanumeric character.` ); } - if (process.platform === 'win32') { - return path.join(path.resolve(resourceFolder), `${resourceName}.lock`); - } else if (process.platform === 'linux' || process.platform === 'darwin') { - return path.join(path.resolve(resourceFolder), `${resourceName}#${pid}.lock`); - } + switch (process.platform) { + case 'win32': { + return path.resolve(resourceFolder, `${resourceName}.lock`); + } + + case 'linux': + case 'darwin': { + return path.resolve(resourceFolder, `${resourceName}#${pid}.lock`); + } - throw new Error(`File locking not implemented for platform: "${process.platform}"`); + default: { + throw new Error(`File locking not implemented for platform: "${process.platform}"`); + } + } } /** @@ -196,12 +209,15 @@ export class LockFile { */ public static tryAcquire(resourceFolder: string, resourceName: string): LockFile | undefined { FileSystem.ensureFolder(resourceFolder); - if (process.platform === 'win32') { - return LockFile._tryAcquireWindows(resourceFolder, resourceName); - } else if (process.platform === 'linux' || process.platform === 'darwin') { - return LockFile._tryAcquireMacOrLinux(resourceFolder, resourceName); - } - throw new Error(`File locking not implemented for platform: "${process.platform}"`); + const lockFilePath: string = LockFile.getLockFilePath(resourceFolder, resourceName); + return LockFile._tryAcquireInner(resourceFolder, resourceName, lockFilePath); + } + + /** + * @deprecated Use {@link LockFile.acquireAsync} instead. + */ + public static acquire(resourceFolder: string, resourceName: string, maxWaitMs?: number): Promise { + return LockFile.acquireAsync(resourceFolder, resourceName, maxWaitMs); } /** @@ -218,30 +234,67 @@ export class LockFile { * the filename of the temporary file created to manage the lock. * @param maxWaitMs - The maximum number of milliseconds to wait for the lock before reporting an error */ - public static acquire(resourceFolder: string, resourceName: string, maxWaitMs?: number): Promise { + public static async acquireAsync( + resourceFolder: string, + resourceName: string, + maxWaitMs?: number + ): Promise { const interval: number = 100; const startTime: number = Date.now(); + const timeoutTime: number | undefined = maxWaitMs ? startTime + maxWaitMs : undefined; + + await FileSystem.ensureFolderAsync(resourceFolder); + + const lockFilePath: string = LockFile.getLockFilePath(resourceFolder, resourceName); - const retryLoop: () => Promise = async () => { - const lock: LockFile | undefined = LockFile.tryAcquire(resourceFolder, resourceName); + // eslint-disable-next-line no-unmodified-loop-condition + while (!timeoutTime || Date.now() <= timeoutTime) { + const lock: LockFile | undefined = LockFile._tryAcquireInner( + resourceFolder, + resourceName, + lockFilePath + ); if (lock) { return lock; } - if (maxWaitMs && Date.now() > startTime + maxWaitMs) { - throw new Error(`Exceeded maximum wait time to acquire lock for resource "${resourceName}"`); - } await Async.sleepAsync(interval); - return retryLoop(); - }; + } + + throw new Error(`Exceeded maximum wait time to acquire lock for resource "${resourceName}"`); + } + + private static _tryAcquireInner( + resourceFolder: string, + resourceName: string, + lockFilePath: string + ): LockFile | undefined { + if (!IN_PROC_LOCKS.has(lockFilePath)) { + switch (process.platform) { + case 'win32': { + return LockFile._tryAcquireWindows(lockFilePath); + } - return retryLoop(); + case 'linux': + case 'darwin': { + return LockFile._tryAcquireMacOrLinux(resourceFolder, resourceName, lockFilePath); + } + + default: { + throw new Error(`File locking not implemented for platform: "${process.platform}"`); + } + } + } } /** * Attempts to acquire the lock on a Linux or OSX machine */ - private static _tryAcquireMacOrLinux(resourceFolder: string, resourceName: string): LockFile | undefined { + private static _tryAcquireMacOrLinux( + resourceFolder: string, + resourceName: string, + pidLockFilePath: string + ): LockFile | undefined { let dirtyWhenAcquired: boolean = false; // get the current process' pid @@ -252,7 +305,6 @@ export class LockFile { throw new Error(`Unable to calculate start time for current process.`); } - const pidLockFilePath: string = LockFile.getLockFilePath(resourceFolder, resourceName); let lockFileHandle: FileWriter | undefined; let lockFile: LockFile; @@ -283,7 +335,7 @@ export class LockFile { (otherPid = match[2]) !== pid.toString() ) { // we found at least one lockfile hanging around that isn't ours - const fileInFolderPath: string = path.join(resourceFolder, fileInFolder); + const fileInFolderPath: string = `${resourceFolder}/${fileInFolder}`; dirtyWhenAcquired = true; // console.log(`FOUND OTHER LOCKFILE: ${otherPid}`); @@ -382,8 +434,7 @@ export class LockFile { * Attempts to acquire the lock using Windows * This algorithm is much simpler since we can rely on the operating system */ - private static _tryAcquireWindows(resourceFolder: string, resourceName: string): LockFile | undefined { - const lockFilePath: string = LockFile.getLockFilePath(resourceFolder, resourceName); + private static _tryAcquireWindows(lockFilePath: string): LockFile | undefined { let dirtyWhenAcquired: boolean = false; let fileHandle: FileWriter | undefined; @@ -433,10 +484,13 @@ export class LockFile { throw new Error(`The lock for file "${path.basename(this._filePath)}" has already been released.`); } + IN_PROC_LOCKS.delete(this._filePath); + this._fileWriter!.close(); if (deleteFile) { FileSystem.deleteFile(this._filePath); } + this._fileWriter = undefined; } diff --git a/libraries/node-core-library/src/test/LockFile.test.ts b/libraries/node-core-library/src/test/LockFile.test.ts index 9661e84b387..80eb1f6fd69 100644 --- a/libraries/node-core-library/src/test/LockFile.test.ts +++ b/libraries/node-core-library/src/test/LockFile.test.ts @@ -99,6 +99,46 @@ describe(LockFile.name, () => { }); }); + it('supports two lockfiles in the same process', async () => { + const testFolder: string = `${libTestFolder}/6`; + await FileSystem.ensureEmptyFolderAsync(testFolder); + + const resourceName: string = 'test1'; + + const lock1: LockFile = await LockFile.acquireAsync(testFolder, resourceName); + const lock2Promise: Promise = LockFile.acquireAsync(testFolder, resourceName); + + let lock2Acquired: boolean = false; + lock2Promise + .then(() => { + lock2Acquired = true; + }) + .catch(() => { + fail(); + }); + + const lock1Exists: boolean = await FileSystem.existsAsync(lock1.filePath); + expect(lock1Exists).toEqual(true); + expect(lock1.isReleased).toEqual(false); + expect(lock2Acquired).toEqual(false); + + lock1.release(); + + expect(lock1.isReleased).toEqual(true); + + const lock2: LockFile = await lock2Promise; + + const lock2Exists: boolean = await FileSystem.existsAsync(lock2.filePath); + expect(lock2Exists).toEqual(true); + expect(lock2.isReleased).toEqual(false); + + expect(lock2Acquired).toEqual(true); + + lock2.release(); + + expect(lock2.isReleased).toEqual(true); + }); + if (process.platform === 'darwin' || process.platform === 'linux') { describe('Linux and Mac', () => { describe(LockFile.getLockFilePath.name, () => { @@ -171,6 +211,7 @@ describe(LockFile.name, () => { // this lock should be undefined since there is an existing lock expect(lock).toBeUndefined(); }); + test('cannot acquire a lock if another valid lock exists with the same start time', () => { // ensure test folder is clean const testFolder: string = path.join(libTestFolder, '3'); @@ -235,7 +276,7 @@ describe(LockFile.name, () => { expect(deleteFileSpy).toHaveBeenNthCalledWith(1, otherPidLockFileName); }); - test('doesn’t attempt deleting other process lockfile if it is released in the middle of acquiring process', () => { + test("doesn't attempt deleting other process lockfile if it is released in the middle of acquiring process", () => { // ensure test folder is clean const testFolder: string = path.join(libTestFolder, '5'); FileSystem.ensureEmptyFolder(testFolder); @@ -286,81 +327,83 @@ describe(LockFile.name, () => { } if (process.platform === 'win32') { - describe(LockFile.getLockFilePath.name, () => { - test("returns a resolved path that doesn't contain", () => { - expect(path.join(process.cwd(), `test.lock`)).toEqual(LockFile.getLockFilePath('./', 'test')); - }); + describe('Windows', () => { + describe(LockFile.getLockFilePath.name, () => { + test("returns a resolved path that doesn't contain", () => { + expect(path.join(process.cwd(), `test.lock`)).toEqual(LockFile.getLockFilePath('./', 'test')); + }); - test('ignores pid that is passed in', () => { - expect(path.join(process.cwd(), `test.lock`)).toEqual(LockFile.getLockFilePath('./', 'test', 99)); + test('ignores pid that is passed in', () => { + expect(path.join(process.cwd(), `test.lock`)).toEqual(LockFile.getLockFilePath('./', 'test', 99)); + }); }); - }); - test('will not acquire if existing lock is there', () => { - // ensure test folder is clean - const testFolder: string = path.join(libTestFolder, '1'); - FileSystem.deleteFolder(testFolder); - FileSystem.ensureFolder(testFolder); - - // create an open lockfile - const resourceName: string = 'test'; - const lockFileName: string = LockFile.getLockFilePath(testFolder, resourceName); - const lockFileHandle: FileWriter = FileWriter.open(lockFileName, { exclusive: true }); + test('will not acquire if existing lock is there', () => { + // ensure test folder is clean + const testFolder: string = path.join(libTestFolder, '1'); + FileSystem.deleteFolder(testFolder); + FileSystem.ensureFolder(testFolder); - const lock: LockFile | undefined = LockFile.tryAcquire(testFolder, resourceName); + // create an open lockfile + const resourceName: string = 'test'; + const lockFileName: string = LockFile.getLockFilePath(testFolder, resourceName); + const lockFileHandle: FileWriter = FileWriter.open(lockFileName, { exclusive: true }); - // this lock should be undefined since there is an existing lock - expect(lock).toBeUndefined(); - lockFileHandle.close(); - }); + const lock: LockFile | undefined = LockFile.tryAcquire(testFolder, resourceName); - test('can acquire and close a dirty lockfile', () => { - // ensure test folder is clean - const testFolder: string = path.join(libTestFolder, '1'); - FileSystem.ensureEmptyFolder(testFolder); + // this lock should be undefined since there is an existing lock + expect(lock).toBeUndefined(); + lockFileHandle.close(); + }); - // Create a lockfile that is still hanging around on disk, - const resourceName: string = 'test'; - const lockFileName: string = LockFile.getLockFilePath(testFolder, resourceName); - FileWriter.open(lockFileName, { exclusive: true }).close(); + test('can acquire and close a dirty lockfile', () => { + // ensure test folder is clean + const testFolder: string = path.join(libTestFolder, '1'); + FileSystem.ensureEmptyFolder(testFolder); - const lock: LockFile | undefined = LockFile.tryAcquire(testFolder, resourceName); + // Create a lockfile that is still hanging around on disk, + const resourceName: string = 'test'; + const lockFileName: string = LockFile.getLockFilePath(testFolder, resourceName); + FileWriter.open(lockFileName, { exclusive: true }).close(); - expect(lock).toBeDefined(); - expect(lock!.dirtyWhenAcquired).toEqual(true); - expect(lock!.isReleased).toEqual(false); - expect(FileSystem.exists(lockFileName)).toEqual(true); + const lock: LockFile | undefined = LockFile.tryAcquire(testFolder, resourceName); - // Ensure that we can release the "dirty" lockfile - lock!.release(); - expect(FileSystem.exists(lockFileName)).toEqual(false); - expect(lock!.isReleased).toEqual(true); - }); + expect(lock).toBeDefined(); + expect(lock!.dirtyWhenAcquired).toEqual(true); + expect(lock!.isReleased).toEqual(false); + expect(FileSystem.exists(lockFileName)).toEqual(true); - test('can acquire and close a clean lockfile', () => { - // ensure test folder is clean - const testFolder: string = path.join(libTestFolder, '1'); - FileSystem.ensureEmptyFolder(testFolder); + // Ensure that we can release the "dirty" lockfile + lock!.release(); + expect(FileSystem.exists(lockFileName)).toEqual(false); + expect(lock!.isReleased).toEqual(true); + }); - const resourceName: string = 'test'; - const lockFileName: string = LockFile.getLockFilePath(testFolder, resourceName); - const lock: LockFile | undefined = LockFile.tryAcquire(testFolder, resourceName); + test('can acquire and close a clean lockfile', () => { + // ensure test folder is clean + const testFolder: string = path.join(libTestFolder, '1'); + FileSystem.ensureEmptyFolder(testFolder); - // The lockfile should exist and be in a clean state - expect(lock).toBeDefined(); - expect(lock!.dirtyWhenAcquired).toEqual(false); - expect(lock!.isReleased).toEqual(false); - expect(FileSystem.exists(lockFileName)).toEqual(true); + const resourceName: string = 'test'; + const lockFileName: string = LockFile.getLockFilePath(testFolder, resourceName); + const lock: LockFile | undefined = LockFile.tryAcquire(testFolder, resourceName); - // Ensure that we can release the "clean" lockfile - lock!.release(); - expect(FileSystem.exists(lockFileName)).toEqual(false); - expect(lock!.isReleased).toEqual(true); + // The lockfile should exist and be in a clean state + expect(lock).toBeDefined(); + expect(lock!.dirtyWhenAcquired).toEqual(false); + expect(lock!.isReleased).toEqual(false); + expect(FileSystem.exists(lockFileName)).toEqual(true); - // Ensure we cannot release the lockfile twice - expect(() => { + // Ensure that we can release the "clean" lockfile lock!.release(); - }).toThrow(); + expect(FileSystem.exists(lockFileName)).toEqual(false); + expect(lock!.isReleased).toEqual(true); + + // Ensure we cannot release the lockfile twice + expect(() => { + lock!.release(); + }).toThrow(); + }); }); } }); diff --git a/libraries/rush-lib/src/logic/Autoinstaller.ts b/libraries/rush-lib/src/logic/Autoinstaller.ts index b136a554d3f..3b04e6c0cca 100644 --- a/libraries/rush-lib/src/logic/Autoinstaller.ts +++ b/libraries/rush-lib/src/logic/Autoinstaller.ts @@ -101,7 +101,7 @@ export class Autoinstaller { this._logIfConsoleOutputIsNotRestricted(`Acquiring lock for "${relativePathForLogs}" folder...`); - const lock: LockFile = await LockFile.acquire(autoinstallerFullPath, 'autoinstaller'); + const lock: LockFile = await LockFile.acquireAsync(autoinstallerFullPath, 'autoinstaller'); try { // Example: .../common/autoinstallers/my-task/.rush/temp diff --git a/libraries/rush-lib/src/logic/CredentialCache.ts b/libraries/rush-lib/src/logic/CredentialCache.ts index e0cb67b5e3f..de2bb123967 100644 --- a/libraries/rush-lib/src/logic/CredentialCache.ts +++ b/libraries/rush-lib/src/logic/CredentialCache.ts @@ -82,7 +82,7 @@ export class CredentialCache /* implements IDisposable */ { let lockfile: LockFile | undefined; if (options.supportEditing) { - lockfile = await LockFile.acquire(rushUserFolderPath, `${CACHE_FILENAME}.lock`); + lockfile = await LockFile.acquireAsync(rushUserFolderPath, `${CACHE_FILENAME}.lock`); } const credentialCache: CredentialCache = new CredentialCache(cacheFilePath, loadedJson, lockfile); diff --git a/libraries/rush-lib/src/logic/installManager/InstallHelpers.ts b/libraries/rush-lib/src/logic/installManager/InstallHelpers.ts index 83927cc7d77..cb8a4df9410 100644 --- a/libraries/rush-lib/src/logic/installManager/InstallHelpers.ts +++ b/libraries/rush-lib/src/logic/installManager/InstallHelpers.ts @@ -167,7 +167,7 @@ export class InstallHelpers { logIfConsoleOutputIsNotRestricted(`Trying to acquire lock for ${packageManagerAndVersion}`); - const lock: LockFile = await LockFile.acquire(rushUserFolder, packageManagerAndVersion); + const lock: LockFile = await LockFile.acquireAsync(rushUserFolder, packageManagerAndVersion); logIfConsoleOutputIsNotRestricted(`Acquired lock for ${packageManagerAndVersion}`);