From 31db88ec06f27d4e42bcfd980eeaef8a96be6632 Mon Sep 17 00:00:00 2001 From: Ian Clanton-Thuon Date: Wed, 11 Sep 2024 22:59:28 -0700 Subject: [PATCH 01/10] Introduce a test that's broken on mac and linux for acquiring two locks on the same resource at the same time. --- .../src/test/LockFile.test.ts | 163 +++++++++++------- 1 file changed, 103 insertions(+), 60 deletions(-) diff --git a/libraries/node-core-library/src/test/LockFile.test.ts b/libraries/node-core-library/src/test/LockFile.test.ts index 9661e84b387..e18935c2b56 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.acquire(testFolder, resourceName); + const lock2Promise: Promise = LockFile.acquire(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(); + }); }); } }); From 9742743305e10b1ccbd30182a755d6cbafae8448 Mon Sep 17 00:00:00 2001 From: Ian Clanton-Thuon Date: Wed, 11 Sep 2024 23:22:50 -0700 Subject: [PATCH 02/10] Rename LockFile.acquire to Lockfile.acquireAsync. --- .../lockfile-fixes_2024-09-12-06-22.json | 10 ++++++++++ common/reviews/api/node-core-library.api.md | 2 ++ libraries/node-core-library/src/LockFile.ts | 13 ++++++++++++- .../node-core-library/src/test/LockFile.test.ts | 4 ++-- libraries/rush-lib/src/logic/Autoinstaller.ts | 2 +- libraries/rush-lib/src/logic/CredentialCache.ts | 2 +- .../src/logic/installManager/InstallHelpers.ts | 2 +- 7 files changed, 29 insertions(+), 6 deletions(-) create mode 100644 common/changes/@rushstack/node-core-library/lockfile-fixes_2024-09-12-06-22.json 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/reviews/api/node-core-library.api.md b/common/reviews/api/node-core-library.api.md index 842a74830bb..cd8ff9b8b72 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..9b9b296ebfa 100644 --- a/libraries/node-core-library/src/LockFile.ts +++ b/libraries/node-core-library/src/LockFile.ts @@ -204,6 +204,13 @@ export class LockFile { throw new Error(`File locking not implemented for platform: "${process.platform}"`); } + /** + * @deprecated Use {@link LockFile.acquireAsync} instead. + */ + public static acquire(resourceFolder: string, resourceName: string, maxWaitMs?: number): Promise { + return LockFile.acquireAsync(resourceFolder, resourceName, maxWaitMs); + } + /** * Attempts to create the lockfile. Will continue to loop at every 100ms until the lock becomes available * or the maxWaitMs is surpassed. @@ -218,7 +225,11 @@ 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(); diff --git a/libraries/node-core-library/src/test/LockFile.test.ts b/libraries/node-core-library/src/test/LockFile.test.ts index e18935c2b56..80eb1f6fd69 100644 --- a/libraries/node-core-library/src/test/LockFile.test.ts +++ b/libraries/node-core-library/src/test/LockFile.test.ts @@ -105,8 +105,8 @@ describe(LockFile.name, () => { const resourceName: string = 'test1'; - const lock1: LockFile = await LockFile.acquire(testFolder, resourceName); - const lock2Promise: Promise = LockFile.acquire(testFolder, resourceName); + const lock1: LockFile = await LockFile.acquireAsync(testFolder, resourceName); + const lock2Promise: Promise = LockFile.acquireAsync(testFolder, resourceName); let lock2Acquired: boolean = false; lock2Promise 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}`); From 18ada35aaa024505bd27cdf14f49cd64a5c4be0a Mon Sep 17 00:00:00 2001 From: Ian Clanton-Thuon Date: Wed, 11 Sep 2024 23:23:34 -0700 Subject: [PATCH 03/10] Add support for multiple locks in the same process. --- .../lockfile-fixes_2024-09-12-06-25.json | 10 ++ libraries/node-core-library/src/LockFile.ts | 92 ++++++++++++++----- 2 files changed, 79 insertions(+), 23 deletions(-) create mode 100644 common/changes/@rushstack/node-core-library/lockfile-fixes_2024-09-12-06-25.json 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/libraries/node-core-library/src/LockFile.ts b/libraries/node-core-library/src/LockFile.ts index 9b9b296ebfa..9aee3426072 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 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.join(path.resolve(resourceFolder), `${resourceName}.lock`); + } - throw new Error(`File locking not implemented for platform: "${process.platform}"`); + case 'linux': + case 'darwin': { + return path.join(path.resolve(resourceFolder), `${resourceName}#${pid}.lock`); + } + + default: { + throw new Error(`File locking not implemented for platform: "${process.platform}"`); + } + } } /** @@ -196,12 +209,8 @@ 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); } /** @@ -233,26 +242,62 @@ export class LockFile { const interval: number = 100; const startTime: number = Date.now(); + await FileSystem.ensureFolderAsync(resourceFolder); + + const lockFilePath: string = LockFile.getLockFilePath(resourceFolder, resourceName); + const retryLoop: () => Promise = async () => { - const lock: LockFile | undefined = LockFile.tryAcquire(resourceFolder, resourceName); + 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(); + return await retryLoop(); }; - return retryLoop(); + return await retryLoop(); + } + + 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); + } + + 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 @@ -263,7 +308,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; @@ -294,7 +338,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}`); @@ -393,8 +437,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; @@ -444,10 +487,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; } From a91f9024cca7a79e077f218d2f9cb16a13089442 Mon Sep 17 00:00:00 2001 From: Ian Clanton-Thuon Date: Wed, 11 Sep 2024 23:28:36 -0700 Subject: [PATCH 04/10] Refactor @rushstack/rush-azure-storage-build-cache-plugin to delay locking the CredentialCache file until the credential has been acquired. --- .../rush/lockfile-fixes_2024-09-12-06-27.json | 10 +++ .../src/AzureAuthenticationBase.ts | 61 +++++++++++-------- 2 files changed, 47 insertions(+), 24 deletions(-) create mode 100644 common/changes/@microsoft/rush/lockfile-fixes_2024-09-12-06-27.json diff --git a/common/changes/@microsoft/rush/lockfile-fixes_2024-09-12-06-27.json b/common/changes/@microsoft/rush/lockfile-fixes_2024-09-12-06-27.json new file mode 100644 index 00000000000..56267484046 --- /dev/null +++ b/common/changes/@microsoft/rush/lockfile-fixes_2024-09-12-06-27.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@microsoft/rush", + "comment": "Refactor `@rushstack/rush-azure-storage-build-cache-plugin` to delay locking the `CredentialCache` file until the credential has been acquired.", + "type": "none" + } + ], + "packageName": "@microsoft/rush" +} \ No newline at end of file diff --git a/rush-plugins/rush-azure-storage-build-cache-plugin/src/AzureAuthenticationBase.ts b/rush-plugins/rush-azure-storage-build-cache-plugin/src/AzureAuthenticationBase.ts index 99c39ae58a6..fa03f84312a 100644 --- a/rush-plugins/rush-azure-storage-build-cache-plugin/src/AzureAuthenticationBase.ts +++ b/rush-plugins/rush-azure-storage-build-cache-plugin/src/AzureAuthenticationBase.ts @@ -181,32 +181,45 @@ export abstract class AzureAuthenticationBase { terminal: ITerminal, onlyIfExistingCredentialExpiresAfter?: Date ): Promise { - await CredentialCache.usingAsync( - { - supportEditing: true - }, - async (credentialsCache: CredentialCache) => { - if (onlyIfExistingCredentialExpiresAfter) { - const existingCredentialExpiration: Date | undefined = credentialsCache.tryGetCacheEntry( - this._credentialCacheId - )?.expires; - if ( - existingCredentialExpiration && - existingCredentialExpiration > onlyIfExistingCredentialExpiresAfter - ) { - return; - } + let shouldUpdateCredential: boolean; + if (onlyIfExistingCredentialExpiresAfter) { + let existingCredentialExpiration: Date | undefined; + await CredentialCache.usingAsync( + { + supportEditing: false + }, + async (credentialsCache: CredentialCache) => { + existingCredentialExpiration = credentialsCache.tryGetCacheEntry(this._credentialCacheId)?.expires; } + ); - const credential: ICredentialResult = await this._getCredentialAsync(terminal, this._loginFlow); - credentialsCache.setCacheEntry(this._credentialCacheId, { - credential: credential.credentialString, - expires: credential.expiresOn, - credentialMetadata: credential.credentialMetadata - }); - await credentialsCache.saveIfModifiedAsync(); - } - ); + shouldUpdateCredential = + !existingCredentialExpiration || existingCredentialExpiration <= onlyIfExistingCredentialExpiresAfter; + } else { + shouldUpdateCredential = true; + } + + if (shouldUpdateCredential) { + const { + credentialString: credential, + expiresOn: expires, + credentialMetadata + }: ICredentialResult = await this._getCredentialAsync(terminal, this._loginFlow); + + await CredentialCache.usingAsync( + { + supportEditing: true + }, + async (credentialsCache: CredentialCache) => { + credentialsCache.setCacheEntry(this._credentialCacheId, { + credential, + expires, + credentialMetadata + }); + await credentialsCache.saveIfModifiedAsync(); + } + ); + } } public async deleteCachedCredentialsAsync(terminal: ITerminal): Promise { From 87f97406d1a1b867682e6761c2a39467321c9e1e Mon Sep 17 00:00:00 2001 From: Ian Clanton-Thuon Date: Thu, 12 Sep 2024 14:53:18 -0700 Subject: [PATCH 05/10] Apply suggestions from code review Co-authored-by: Daniel <3473356+D4N14L@users.noreply.github.com> --- libraries/node-core-library/src/LockFile.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/node-core-library/src/LockFile.ts b/libraries/node-core-library/src/LockFile.ts index 9aee3426072..9f33b1ae1af 100644 --- a/libraries/node-core-library/src/LockFile.ts +++ b/libraries/node-core-library/src/LockFile.ts @@ -180,7 +180,7 @@ 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 alphanumeric 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.` ); } From c6bc7799ffb5ab9794762357ad52e17ceb14189d Mon Sep 17 00:00:00 2001 From: Ian Clanton-Thuon Date: Thu, 12 Sep 2024 14:54:09 -0700 Subject: [PATCH 06/10] Apply suggestions from code review Co-authored-by: David Michon --- libraries/node-core-library/src/LockFile.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/node-core-library/src/LockFile.ts b/libraries/node-core-library/src/LockFile.ts index 9f33b1ae1af..a5d9ea0bd10 100644 --- a/libraries/node-core-library/src/LockFile.ts +++ b/libraries/node-core-library/src/LockFile.ts @@ -186,7 +186,7 @@ export class LockFile { switch (process.platform) { case 'win32': { - return path.join(path.resolve(resourceFolder), `${resourceName}.lock`); + return path.resolve(resourceFolder, `${resourceName}.lock`); } case 'linux': From 31f36a6524337cbd8efc6d1a7b1bb86c16d31b84 Mon Sep 17 00:00:00 2001 From: Ian Clanton-Thuon Date: Thu, 12 Sep 2024 15:08:52 -0700 Subject: [PATCH 07/10] Refactor the retryLoop to be a simple while loop. --- libraries/node-core-library/src/LockFile.ts | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/libraries/node-core-library/src/LockFile.ts b/libraries/node-core-library/src/LockFile.ts index a5d9ea0bd10..4481ac344f3 100644 --- a/libraries/node-core-library/src/LockFile.ts +++ b/libraries/node-core-library/src/LockFile.ts @@ -241,12 +241,14 @@ export class LockFile { ): 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 () => { + // eslint-disable-next-line no-unmodified-loop-condition + while (!timeoutTime || Date.now() <= timeoutTime) { const lock: LockFile | undefined = LockFile._tryAcquireInner( resourceFolder, resourceName, @@ -256,15 +258,10 @@ export class LockFile { 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 await retryLoop(); - }; + } - return await retryLoop(); + throw new Error(`Exceeded maximum wait time to acquire lock for resource "${resourceName}"`); } private static _tryAcquireInner( From 33604c56d41b0ae8966561805e403b0ab552f04d Mon Sep 17 00:00:00 2001 From: Ian Clanton-Thuon Date: Thu, 12 Sep 2024 15:19:20 -0700 Subject: [PATCH 08/10] Update libraries/node-core-library/src/LockFile.ts Co-authored-by: David Michon --- libraries/node-core-library/src/LockFile.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/node-core-library/src/LockFile.ts b/libraries/node-core-library/src/LockFile.ts index a5d9ea0bd10..5e0c70f97dc 100644 --- a/libraries/node-core-library/src/LockFile.ts +++ b/libraries/node-core-library/src/LockFile.ts @@ -191,7 +191,7 @@ export class LockFile { case 'linux': case 'darwin': { - return path.join(path.resolve(resourceFolder), `${resourceName}#${pid}.lock`); + return path.resolve(resourceFolder, `${resourceName}#${pid}.lock`); } default: { From d09dd4c40e74e7b2ec88b07986d0291539a55e77 Mon Sep 17 00:00:00 2001 From: Ian Clanton-Thuon Date: Thu, 12 Sep 2024 15:29:30 -0700 Subject: [PATCH 09/10] Revert changes to Rush. --- .../rush/lockfile-fixes_2024-09-12-06-27.json | 10 --- .../src/AzureAuthenticationBase.ts | 61 ++++++++----------- 2 files changed, 24 insertions(+), 47 deletions(-) delete mode 100644 common/changes/@microsoft/rush/lockfile-fixes_2024-09-12-06-27.json diff --git a/common/changes/@microsoft/rush/lockfile-fixes_2024-09-12-06-27.json b/common/changes/@microsoft/rush/lockfile-fixes_2024-09-12-06-27.json deleted file mode 100644 index 56267484046..00000000000 --- a/common/changes/@microsoft/rush/lockfile-fixes_2024-09-12-06-27.json +++ /dev/null @@ -1,10 +0,0 @@ -{ - "changes": [ - { - "packageName": "@microsoft/rush", - "comment": "Refactor `@rushstack/rush-azure-storage-build-cache-plugin` to delay locking the `CredentialCache` file until the credential has been acquired.", - "type": "none" - } - ], - "packageName": "@microsoft/rush" -} \ No newline at end of file diff --git a/rush-plugins/rush-azure-storage-build-cache-plugin/src/AzureAuthenticationBase.ts b/rush-plugins/rush-azure-storage-build-cache-plugin/src/AzureAuthenticationBase.ts index fa03f84312a..99c39ae58a6 100644 --- a/rush-plugins/rush-azure-storage-build-cache-plugin/src/AzureAuthenticationBase.ts +++ b/rush-plugins/rush-azure-storage-build-cache-plugin/src/AzureAuthenticationBase.ts @@ -181,45 +181,32 @@ export abstract class AzureAuthenticationBase { terminal: ITerminal, onlyIfExistingCredentialExpiresAfter?: Date ): Promise { - let shouldUpdateCredential: boolean; - if (onlyIfExistingCredentialExpiresAfter) { - let existingCredentialExpiration: Date | undefined; - await CredentialCache.usingAsync( - { - supportEditing: false - }, - async (credentialsCache: CredentialCache) => { - existingCredentialExpiration = credentialsCache.tryGetCacheEntry(this._credentialCacheId)?.expires; + await CredentialCache.usingAsync( + { + supportEditing: true + }, + async (credentialsCache: CredentialCache) => { + if (onlyIfExistingCredentialExpiresAfter) { + const existingCredentialExpiration: Date | undefined = credentialsCache.tryGetCacheEntry( + this._credentialCacheId + )?.expires; + if ( + existingCredentialExpiration && + existingCredentialExpiration > onlyIfExistingCredentialExpiresAfter + ) { + return; + } } - ); - - shouldUpdateCredential = - !existingCredentialExpiration || existingCredentialExpiration <= onlyIfExistingCredentialExpiresAfter; - } else { - shouldUpdateCredential = true; - } - if (shouldUpdateCredential) { - const { - credentialString: credential, - expiresOn: expires, - credentialMetadata - }: ICredentialResult = await this._getCredentialAsync(terminal, this._loginFlow); - - await CredentialCache.usingAsync( - { - supportEditing: true - }, - async (credentialsCache: CredentialCache) => { - credentialsCache.setCacheEntry(this._credentialCacheId, { - credential, - expires, - credentialMetadata - }); - await credentialsCache.saveIfModifiedAsync(); - } - ); - } + const credential: ICredentialResult = await this._getCredentialAsync(terminal, this._loginFlow); + credentialsCache.setCacheEntry(this._credentialCacheId, { + credential: credential.credentialString, + expires: credential.expiresOn, + credentialMetadata: credential.credentialMetadata + }); + await credentialsCache.saveIfModifiedAsync(); + } + ); } public async deleteCachedCredentialsAsync(terminal: ITerminal): Promise { From 3e67978e3c29d179ff9afa4e2dd0e9c9ea18b42a Mon Sep 17 00:00:00 2001 From: Ian Clanton-Thuon Date: Thu, 12 Sep 2024 15:36:18 -0700 Subject: [PATCH 10/10] Rush change. --- .../rush/lockfile-fixes_2024-09-12-22-36.json | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 common/changes/@microsoft/rush/lockfile-fixes_2024-09-12-22-36.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