From 471f0f0615c7d46a182c2d396b832eb0df9ee95e Mon Sep 17 00:00:00 2001 From: 20k-ultra <3946250+20k-ultra@users.noreply.github.com> Date: Thu, 26 May 2022 14:07:07 -0400 Subject: [PATCH] Refactor update-lock.lock to accept an array of applications to lock Change-type: patch Signed-off-by: 20k-ultra <3946250+20k-ultra@users.noreply.github.com> --- src/compose/composition-steps.ts | 2 +- src/lib/update-lock.ts | 81 +++++++++++++++----------------- test/src/lib/update-lock.spec.ts | 39 +++++++++++++++ 3 files changed, 79 insertions(+), 43 deletions(-) diff --git a/src/compose/composition-steps.ts b/src/compose/composition-steps.ts index c37096a3..1dc3e57f 100644 --- a/src/compose/composition-steps.ts +++ b/src/compose/composition-steps.ts @@ -120,7 +120,7 @@ type Executors = { type LockingFn = ( // TODO: Once the entire codebase is typescript, change // this to number - app: number | null, + app: number | number[] | null, args: BaseCompositionStepArgs, fn: () => Promise, ) => Promise; diff --git a/src/lib/update-lock.ts b/src/lib/update-lock.ts index d114b959..d70a086a 100644 --- a/src/lib/update-lock.ts +++ b/src/lib/update-lock.ts @@ -71,11 +71,20 @@ export const readLock: LockFn = Bluebird.promisify(locker.async.readLock, { }); // Unlock all lockfiles, optionally of an appId | appUuid, then release resources. -async function dispose(appIdentifier: string | number): Promise { +async function dispose( + appIdentifier: string | number, + release: () => void, +): Promise { const locks = lockfile.getLocksTaken((p: string) => p.includes(`${BASE_LOCK_DIR}/${appIdentifier}`), ); - await Promise.all(locks.map((l) => lockfile.unlock(l))); + try { + // Try to unlock all locks taken + await Promise.all(locks.map((l) => lockfile.unlock(l))); + } finally { + // Release final resource + release(); + } } /** @@ -88,15 +97,18 @@ async function dispose(appIdentifier: string | number): Promise { * without an option to skip. */ export async function lock( - appId: number, + appId: number | number[], { force = false, skipLock = false }: { force: boolean; skipLock?: boolean }, fn: () => Resolvable, ): Promise { - if (skipLock || appId == null) { - return Promise.resolve(fn()); + const appIdsToLock = Array.isArray(appId) ? appId : [appId]; + if (skipLock || !appId || !appIdsToLock.length) { + return fn(); } - const lockDir = getPathOnHost(lockPath(appId)); + // Sort appIds so they are always locked in the same sequence + const sortedIds = appIdsToLock.sort(); + let lockOverride: boolean; try { lockOverride = await config.get('lockOverride'); @@ -106,58 +118,43 @@ export async function lock( ); } - let release; + const releases = new Map void>(); try { - // Acquire write lock for appId - release = await writeLock(appId); - // Get list of service folders in lock directory - let serviceFolders: string[] = []; - try { - serviceFolders = await fs.readdir(lockDir); - } catch (err) { - if (ENOENT(err)) { - // Default to empty list of folders - serviceFolders = []; - } else { - throw err; + for (const id of sortedIds) { + const lockDir = getPathOnHost(lockPath(id)); + // Acquire write lock for appId + releases.set(id, await writeLock(id)); + // Get list of service folders in lock directory + const serviceFolders = await fs.readdir(lockDir).catch((e) => { + if (ENOENT(e)) { + return []; + } + throw e; + }); + // Attempt to create a lock for each service + for (const service of serviceFolders) { + await lockService(id, service, force || lockOverride); } } - - // Attempt to create a lock for each service - await Promise.all( - serviceFolders.map((service) => - lockService(appId, service, { force, lockOverride }), - ), - ); - // Resolve the function passed - return Promise.resolve(fn()); + return fn(); } finally { - // Cleanup locks - if (release) { - release(); + for (const [id, release] of releases.entries()) { + // Try to dispose all the locks + await dispose(id, release); } - await dispose(appId); } } -type LockOptions = { - force?: boolean; - lockOverride?: boolean; -}; - async function lockService( appId: number, service: string, - opts: LockOptions = { - force: false, - lockOverride: false, - }, + force: boolean = false, ): Promise { const serviceLockFiles = lockFilesOnHost(appId, service); for await (const file of serviceLockFiles) { try { - if (opts.force || opts.lockOverride) { + if (force) { await lockfile.unlock(file); } await lockfile.lock(file, LOCKFILE_UID); diff --git a/test/src/lib/update-lock.spec.ts b/test/src/lib/update-lock.spec.ts index 6b38aa3c..fca9d828 100644 --- a/test/src/lib/update-lock.spec.ts +++ b/test/src/lib/update-lock.spec.ts @@ -242,6 +242,45 @@ describe('lib/update-lock', () => { ); }); + it('locks all applications before resolving input function', async () => { + const appIds = [111, 222, 333]; + // Set up fake filesystem for lockfiles + mockFs({ + [path.join( + constants.rootMountPoint, + updateLock.lockPath(111), + serviceName, + )]: {}, + [path.join( + constants.rootMountPoint, + updateLock.lockPath(222), + serviceName, + )]: {}, + [path.join( + constants.rootMountPoint, + updateLock.lockPath(333), + serviceName, + )]: {}, + }); + + await expect( + updateLock.lock(appIds, { force: false }, async () => { + // At this point the locks should be taken and not removed + // until this function has been resolved + // Both `updates.lock` and `resin-updates.lock` should have been taken + expect(lockSpy.args).to.have.length(6); + // Make sure that no locks have been removed also + expect(unlockSpy).to.not.be.called; + return Promise.resolve(); + }), + ).to.eventually.be.fulfilled; + + // Everything that was locked should have been unlocked after function resolves + expect(lockSpy.args.map(([lock]) => [lock])).to.deep.equal( + unlockSpy.args, + ); + }); + it('resolves input function without locking when appId is null', async () => { mockLockDir({ createLockfile: true });