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 <christina@balena.io>

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<InstancedAppState>) {
 	events.emit('change', data ?? {});
 }
 
-export async function lockingIfNecessary<T extends unknown>(
-	appId: number,
-	{ force = false, skipLock = false } = {},
-	fn: () => Resolvable<T>,
-) {
-	if (skipLock) {
-		return fn();
-	}
-	const lockOverride = (await config.get('lockOverride')) || force;
-	return updateLock.lock(
-		appId,
-		{ force: lockOverride },
-		fn as () => PromiseLike<void>,
-	);
-}
-
 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];

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;
 

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<void>;
@@ -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<T extends unknown>(
 	appId: number | null,
-	{ force = false }: { force: boolean },
-	fn: () => PromiseLike<void>,
-): Bluebird<void> {
+	{ force = false, skipLock = false }: { force: boolean; skipLock?: boolean },
+	fn: () => Resolvable<T>,
+): Bluebird<T> {
+	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<T>);
 	} else {
 		return Bluebird.resolve(fn());
 	}