From 4f446103f4b9256118434613a9f3c841eeb0a6f3 Mon Sep 17 00:00:00 2001 From: Christina Wang Date: Wed, 9 Feb 2022 02:41:13 +0000 Subject: [PATCH] Remove lockingIfNecessary in favor of updateLock.lock The functionality is pretty much the same, so we don't need the two functions in two different places. Signed-off-by: Christina Wang --- src/compose/application-manager.ts | 24 ++------- src/device-api/common.js | 13 ++--- src/lib/update-lock.ts | 82 +++++++++++++++++++----------- 3 files changed, 62 insertions(+), 57 deletions(-) diff --git a/src/compose/application-manager.ts b/src/compose/application-manager.ts index 2362e77b..844b0386 100644 --- a/src/compose/application-manager.ts +++ b/src/compose/application-manager.ts @@ -1,5 +1,6 @@ import * as express from 'express'; import * as _ from 'lodash'; +import StrictEventEmitter from 'strict-event-emitter-types'; import * as config from '../config'; import { transaction, Transaction } from '../db'; @@ -14,7 +15,7 @@ import { ContractViolationError, InternalInconsistencyError, } from '../lib/errors'; -import StrictEventEmitter from 'strict-event-emitter-types'; +import { lock } from '../lib/update-lock'; import App from './app'; import * as volumeManager from './volume-manager'; @@ -39,7 +40,6 @@ import { } from '../types/state'; import { checkTruthy, checkInt } from '../lib/validation'; import { Proxyvisor } from '../proxyvisor'; -import * as updateLock from '../lib/update-lock'; import { EventEmitter } from 'events'; type ApplicationManagerEventEmitter = StrictEventEmitter< @@ -90,7 +90,7 @@ export function resetTimeSpentFetching(value: number = 0) { } const actionExecutors = getExecutors({ - lockFn: lockingIfNecessary, + lockFn: lock, callbacks: { containerStarted: (id: string) => { containerStarted[id] = true; @@ -157,22 +157,6 @@ function reportCurrentState(data?: Partial) { events.emit('change', data ?? {}); } -export async function lockingIfNecessary( - appId: number, - { force = false, skipLock = false } = {}, - fn: () => Resolvable, -) { - if (skipLock) { - return fn(); - } - const lockOverride = (await config.get('lockOverride')) || force; - return updateLock.lock( - appId, - { force: lockOverride }, - fn as () => PromiseLike, - ); -} - export async function getRequiredSteps( targetApps: InstancedAppState, ignoreImages: boolean = false, @@ -359,7 +343,7 @@ export async function stopAll({ force = false, skipLock = false } = {}) { const services = await serviceManager.getAll(); await Promise.all( services.map(async (s) => { - return lockingIfNecessary(s.appId, { force, skipLock }, async () => { + return lock(s.appId, { force, skipLock }, async () => { await serviceManager.kill(s, { removeContainer: false, wait: true }); if (s.containerId) { delete containerStarted[s.containerId]; diff --git a/src/device-api/common.js b/src/device-api/common.js index 3f05e76f..bf12d542 100644 --- a/src/device-api/common.js +++ b/src/device-api/common.js @@ -1,21 +1,20 @@ import * as Bluebird from 'bluebird'; import * as _ from 'lodash'; -import { appNotFoundMessage } from '../lib/messages'; -import * as logger from '../logger'; +import * as logger from '../logger'; import * as deviceState from '../device-state'; import * as applicationManager from '../compose/application-manager'; import * as serviceManager from '../compose/service-manager'; import * as volumeManager from '../compose/volume-manager'; import { InternalInconsistencyError } from '../lib/errors'; +import { lock } from '../lib/update-lock'; +import { appNotFoundMessage } from '../lib/messages'; export async function doRestart(appId, force) { await deviceState.initialized; await applicationManager.initialized; - const { lockingIfNecessary } = applicationManager; - - return lockingIfNecessary(appId, { force }, () => + return lock(appId, { force }, () => deviceState.getCurrentState().then(function (currentState) { if (currentState.local.apps?.[appId] == null) { throw new InternalInconsistencyError( @@ -42,14 +41,12 @@ export async function doPurge(appId, force) { await deviceState.initialized; await applicationManager.initialized; - const { lockingIfNecessary } = applicationManager; - logger.logSystemMessage( `Purging data for app ${appId}`, { appId }, 'Purge data', ); - return lockingIfNecessary(appId, { force }, () => + return lock(appId, { force }, () => deviceState.getCurrentState().then(function (currentState) { const allApps = currentState.local.apps; diff --git a/src/lib/update-lock.ts b/src/lib/update-lock.ts index db1d24b2..2ac8d236 100644 --- a/src/lib/update-lock.ts +++ b/src/lib/update-lock.ts @@ -6,8 +6,14 @@ import * as path from 'path'; import * as Lock from 'rwlock'; import * as constants from './constants'; -import { ENOENT, EEXIST, UpdatesLockedError } from './errors'; +import { + ENOENT, + EEXIST, + UpdatesLockedError, + InternalInconsistencyError, +} from './errors'; import { getPathOnHost, pathExistsOnHost } from './fs-utils'; +import * as config from '../config'; type asyncLockFile = typeof lockFileLib & { unlockAsync(path: string): Bluebird; @@ -115,47 +121,65 @@ const lockExistsErrHandler = (err: Error, release: () => void) => { /** * Try to take the locks for an application. If force is set, it will remove * all existing lockfiles before performing the operation + * + * TODO: convert to native Promises. May require native implementation of Bluebird's dispose / using + * + * TODO: Remove skipLock as it's not a good interface. If lock is called it should try to take the lock + * without an option to skip. */ -export function lock( +export function lock( appId: number | null, - { force = false }: { force: boolean }, - fn: () => PromiseLike, -): Bluebird { + { force = false, skipLock = false }: { force: boolean; skipLock?: boolean }, + fn: () => Resolvable, +): Bluebird { + if (skipLock) { + return Bluebird.resolve(fn()); + } + const takeTheLock = () => { if (appId == null) { return; } - return writeLock(appId) - .tap((release: () => void) => { - const [lockDir] = getPathOnHost(lockPath(appId)); + return config + .get('lockOverride') + .then((lockOverride) => { + return writeLock(appId) + .tap((release: () => void) => { + const [lockDir] = getPathOnHost(lockPath(appId)); - return Bluebird.resolve(fs.readdir(lockDir)) - .catchReturn(ENOENT, []) - .mapSeries((serviceName) => { - return Bluebird.mapSeries( - lockFilesOnHost(appId, serviceName), - (tmpLockName) => { - return Bluebird.try(() => { - if (force) { - return lockFile.unlockAsync(tmpLockName); - } - }) - .then(() => lockFile.lockAsync(tmpLockName)) - .then(() => { - locksTaken[tmpLockName] = true; - }) - .catchReturn(ENOENT, undefined); - }, - ); + return Bluebird.resolve(fs.readdir(lockDir)) + .catchReturn(ENOENT, []) + .mapSeries((serviceName) => { + return Bluebird.mapSeries( + lockFilesOnHost(appId, serviceName), + (tmpLockName) => { + return Bluebird.try(() => { + if (force || lockOverride) { + return lockFile.unlockAsync(tmpLockName); + } + }) + .then(() => lockFile.lockAsync(tmpLockName)) + .then(() => { + locksTaken[tmpLockName] = true; + }) + .catchReturn(ENOENT, undefined); + }, + ); + }) + .catch((err) => lockExistsErrHandler(err, release)); }) - .catch((err) => lockExistsErrHandler(err, release)); + .disposer(dispose); }) - .disposer(dispose); + .catch((err) => { + throw new InternalInconsistencyError( + `Error getting lockOverride config value: ${err?.message ?? err}`, + ); + }); }; const disposer = takeTheLock(); if (disposer) { - return Bluebird.using(disposer, fn); + return Bluebird.using(disposer, fn as () => PromiseLike); } else { return Bluebird.resolve(fn()); }