fix: lock the debug connection execution in order to avoid possible issues with multiple device socket connections and attach requests, remove all locks on process exit

Author: DimitarTachev

lib/common/declarations.d.ts

@@ -1958,67 +1958,6 @@ interface IiOSNotificationService {
 	postNotification(deviceIdentifier: string, notification: string, commandType?: string): Promise<number>;
 }
 
-/**
- * Copied from @types/lockfile
- * Describes the options that can be passed to lockfile methods.
- */
-interface ILockFileOptions {
-	/**
-	 * A number of milliseconds to wait for locks to expire before giving up.
-	 * Only used by lockFile.lock. Poll for opts.wait ms.
-	 * If the lock is not cleared by the time the wait expires, then it returns with the original error.
-	 */
-	wait?: number;
-
-	/**
-	 * When using opts.wait, this is the period in ms in which it polls to check if the lock has expired. Defaults to 100.
-	 */
-	pollPeriod?: number;
-
-	/**
-	 * A number of milliseconds before locks are considered to have expired.
-	 */
-	stale?: number;
-
-	/**
-	 * Used by lock and lockSync. Retry n number of times before giving up.
-	 */
-	retries?: number;
-
-	/**
-	 * Used by lock. Wait n milliseconds before retrying.
-	 */
-	retryWait?: number;
-}
-
-/**
- * Describes methods that can be used to use file locking.
- */
-interface ILockFile {
-	/**
-	 * Acquire a file lock on the specified path.
-	 * @param {string} lockFilePath Path to lockfile that has to be created. Defaults to `<profile dir>/lockfile.lock`
-	 * @param {ILockFileOptions} lockFileOpts Options used for creating the lockfile.
-	 * @returns {Promise<void>}
-	 */
-	lock(lockFilePath?: string, lockFileOpts?: ILockFileOptions): Promise<void>;
-
-	/**
-	 * Close and unlink the lockfile.
-	 * @param {string} lockFilePath Path to lockfile that has to be created. Defaults to `<profile dir>/lockfile.lock`
-	 * @returns {void}
-	 */
-	unlock(lockFilePath?: string): void;
-
-	/**
-	 * Check if the lockfile is locked and not stale.
-	 * @param {string} lockFilePath Path to lockfile that has to be created. Defaults to `<profile dir>/lockfile.lock`
-	 * @param {ILockFileOptions} lockFileOpts Options used for creating the lockfile.
-	 * @returns {boolean} true in case file is locked, false otherwise
-	 */
-	check(lockFilePath?: string, lockFileOpts?: ILockFileOptions): boolean;
-}
-
 declare module "stringify-package" {
 	function stringifyPackage(data: any, indent: any, newline: string): string
 	export = stringifyPackage

lib/common/mobile/ios/device/ios-device.ts

@@ -17,6 +17,7 @@ export class IOSDevice extends IOSDeviceBase {
 		protected $errors: IErrors,
 		private $injector: IInjector,
 		protected $iOSDebuggerPortService: IIOSDebuggerPortService,
+		protected $lockfile: ILockFile,
 		private $iOSSocketRequestExecutor: IiOSSocketRequestExecutor,
 		protected $processService: IProcessService,
 		private $deviceLogProvider: Mobile.IDeviceLogProvider,
@@ -63,6 +64,7 @@ export class IOSDevice extends IOSDeviceBase {
 		await this.$iOSSocketRequestExecutor.executeAttachRequest(this, constants.AWAIT_NOTIFICATION_TIMEOUT_SECONDS, appId);
 		const port = await this.getDebuggerPort(appId);
 		const deviceId = this.deviceInfo.identifier;
+
 		const socket = await helpers.connectEventuallyUntilTimeout(
 			async () => {
 				const deviceResponse = _.first((await this.$iosDeviceOperations.connectToPort([{ deviceId: deviceId, port: port }]))[deviceId]);

lib/common/mobile/ios/ios-device-base.ts

@@ -5,6 +5,7 @@ export abstract class IOSDeviceBase implements Mobile.IiOSDevice {
 	protected abstract $errors: IErrors;
 	protected abstract $iOSDebuggerPortService: IIOSDebuggerPortService;
 	protected abstract $processService: IProcessService;
+	protected abstract $lockfile: ILockFile;
 	abstract deviceInfo: Mobile.IDeviceInfo;
 	abstract applicationManager: Mobile.IDeviceApplicationManager;
 	abstract fileSystem: Mobile.IDeviceFileSystem;
@@ -24,21 +25,23 @@ export abstract class IOSDeviceBase implements Mobile.IiOSDevice {
 	}
 
 	public async getSocket(appId: string): Promise<net.Socket> {
-		if (this.cachedSockets[appId]) {
-			return this.cachedSockets[appId];
-		}
+		return this.$lockfile.executeActionWithLock(async () => {
+			if (this.cachedSockets[appId]) {
+				return this.cachedSockets[appId];
+			}
 
-		this.cachedSockets[appId] = await this.getSocketCore(appId);
+			this.cachedSockets[appId] = await this.getSocketCore(appId);
 
-		if (this.cachedSockets[appId]) {
-			this.cachedSockets[appId].on("close", () => {
-				this.destroySocket(appId);
-			});
+			if (this.cachedSockets[appId]) {
+				this.cachedSockets[appId].on("close", () => {
+					this.destroySocket(appId);
+				});
 
-			this.$processService.attachToProcessExitSignals(this, () => this.destroySocket(appId));
-		}
+				this.$processService.attachToProcessExitSignals(this, () => this.destroySocket(appId));
+			}
 
-		return this.cachedSockets[appId];
+			return this.cachedSockets[appId];
+		}, "ios-debug-socket.lock");
 	}
 
 	public destroyLiveSyncSocket(appId: string) {

lib/common/mobile/ios/simulator/ios-simulator-device.ts

@@ -14,6 +14,7 @@ export class IOSSimulator extends IOSDeviceBase implements Mobile.IiOSDevice {
 	constructor(private simulator: Mobile.IiSimDevice,
 		private $devicePlatformsConstants: Mobile.IDevicePlatformsConstants,
 		protected $errors: IErrors,
+		protected $lockfile: ILockFile,
 		private $injector: IInjector,
 		protected $iOSDebuggerPortService: IIOSDebuggerPortService,
 		private $iOSSimResolver: Mobile.IiOSSimResolver,

lib/common/services/lockfile.ts

@@ -3,53 +3,82 @@ import * as path from "path";
 import { cache } from "../decorators";
 
 export class LockFile implements ILockFile {
+	private currentlyLockedFiles: string[] = [];
 
 	@cache()
 	private get defaultLockFilePath(): string {
-		return path.join(this.$settingsService.getProfileDir(), "lockfile.lock");
+		return this.getAbsoluteLockFilePath("lockfile.lock");
 	}
 
-	private get defaultLockParams(): lockfile.Options {
+	private getAbsoluteLockFilePath(relativeLockFilePath: string) {
+		return path.join(this.$settingsService.getProfileDir(), relativeLockFilePath);
+	}
+
+	private get defaultLockParams(): ILockFileOptions {
 		// We'll retry 100 times and time between retries is 100ms, i.e. full wait in case we are unable to get lock will be 10 seconds.
-		// In case lock is older than 3 minutes, consider it stale and try to get a new lock.
-		const lockParams: lockfile.Options = {
+		// In case lock is older than the `stale` value, consider it stale and try to get a new lock.
+		const lockParams: ILockFileOptions = {
 			retryWait: 100,
 			retries: 100,
-			stale: 180 * 1000,
+			stale: 30 * 1000,
 		};
 
 		return lockParams;
 	}
 
 	constructor(private $fs: IFileSystem,
-		private $settingsService: ISettingsService) {
+		private $settingsService: ISettingsService,
+		private $processService: IProcessService) {
+		this.$processService.attachToProcessExitSignals(this, () => {
+			const locksToRemove = _.clone(this.currentlyLockedFiles);
+			_.each(locksToRemove, lock => {
+				this.unlock(lock);
+			});
+		});
 	}
 
-	public lock(lockFilePath?: string, lockFileOpts?: lockfile.Options): Promise<void> {
+	public lock(lockFilePath?: string, lockFileOpts?: ILockFileOptions): Promise<string> {
 		const { filePath, fileOpts } = this.getLockFileSettings(lockFilePath, lockFileOpts);
+		this.currentlyLockedFiles.push(filePath);
 
 		// Prevent ENOENT error when the dir, where lock should be created, does not exist.
 		this.$fs.ensureDirectoryExists(path.dirname(filePath));
 
-		return new Promise<void>((resolve, reject) => {
+		return new Promise<string>((resolve, reject) => {
 			lockfile.lock(filePath, fileOpts, (err: Error) => {
-				err ? reject(err) : resolve();
+				err ? reject(new Error(`Timeout while waiting for lock "${filePath}"`)) : resolve(filePath);
 			});
 		});
 	}
 
+	public async executeActionWithLock<T>(action: () => Promise<T>, lockFilePath?: string, lockFileOpts?: ILockFileOptions): Promise<T> {
+		const resolvedLockFilePath = await this.lock(lockFilePath, lockFileOpts);
+
+		try {
+			const result = await action();
+			return result;
+		} finally {
+			this.unlock(resolvedLockFilePath);
+		}
+	}
+
 	public unlock(lockFilePath?: string): void {
 		const { filePath } = this.getLockFileSettings(lockFilePath);
+		_.remove(this.currentlyLockedFiles, e => e === lockFilePath);
 		lockfile.unlockSync(filePath);
 	}
 
-	public check(lockFilePath?: string, lockFileOpts?: lockfile.Options): boolean {
+	public check(lockFilePath?: string, lockFileOpts?: ILockFileOptions): boolean {
 		const { filePath, fileOpts } = this.getLockFileSettings(lockFilePath, lockFileOpts);
 
 		return lockfile.checkSync(filePath, fileOpts);
 	}
 
-	private getLockFileSettings(filePath?: string, fileOpts?: lockfile.Options): { filePath: string, fileOpts: lockfile.Options } {
+	private getLockFileSettings(filePath?: string, fileOpts?: ILockFileOptions): { filePath: string, fileOpts: ILockFileOptions } {
+		if (filePath && !path.isAbsolute(filePath)) {
+			filePath = this.getAbsoluteLockFilePath(filePath);
+		}
+
 		filePath = filePath || this.defaultLockFilePath;
 		fileOpts = fileOpts || this.defaultLockParams;
 

lib/common/services/user-settings-service.ts

@@ -5,7 +5,7 @@ export class UserSettingsServiceBase implements IUserSettingsService {
 	private userSettingsFilePath: string = null;
 	protected userSettingsData: any = null;
 	private get lockFilePath(): string {
-		return `${this.userSettingsFilePath}.lock`;
+		return `user-settings.lock`;
 	}
 
 	constructor(userSettingsFilePath: string,
@@ -21,7 +21,7 @@ export class UserSettingsServiceBase implements IUserSettingsService {
 			return this.userSettingsData ? this.userSettingsData[settingName] : null;
 		};
 
-		return this.executeActionWithLock<T>(action);
+		return this.$lockfile.executeActionWithLock<T>(action, this.lockFilePath);
 	}
 
 	public async saveSetting<T>(key: string, value: T): Promise<void> {
@@ -39,17 +39,7 @@ export class UserSettingsServiceBase implements IUserSettingsService {
 			await this.saveSettings();
 		};
 
-		return this.executeActionWithLock<void>(action);
-	}
-
-	private async executeActionWithLock<T>(action: () => Promise<T>): Promise<T> {
-		try {
-			await this.$lockfile.lock(this.lockFilePath);
-			const result = await action();
-			return result;
-		} finally {
-			this.$lockfile.unlock(this.lockFilePath);
-		}
+		return this.$lockfile.executeActionWithLock<void>(action, this.lockFilePath);
 	}
 
 	public saveSettings(data?: any): Promise<void> {
@@ -66,7 +56,7 @@ export class UserSettingsServiceBase implements IUserSettingsService {
 			this.$fs.writeJson(this.userSettingsFilePath, this.userSettingsData);
 		};
 
-		return this.executeActionWithLock<void>(action);
+		return this.$lockfile.executeActionWithLock<void>(action, this.lockFilePath);
 	}
 
 	// TODO: Remove Promise, reason: writeFile - blocked as other implementation of the interface has async operation.

lib/common/test/unit-tests/mobile/ios-simulator-discovery.ts

@@ -4,7 +4,7 @@ import { Yok } from "../../../yok";
 import { assert } from "chai";
 import { DeviceDiscoveryEventNames, CONNECTED_STATUS } from "../../../constants";
 import { DevicePlatformsConstants } from "../../../mobile/device-platforms-constants";
-import { ErrorsStub, CommonLoggerStub, HooksServiceStub } from "../stubs";
+import { ErrorsStub, CommonLoggerStub, HooksServiceStub, LockFileStub } from "../stubs";
 import { FileSystemStub } from "../../../../../test/stubs";
 
 let currentlyRunningSimulators: Mobile.IiSimDevice[];
@@ -16,6 +16,7 @@ function createTestInjector(): IInjector {
 	injector.register("injector", injector);
 	injector.register("errors", ErrorsStub);
 	injector.register("iOSDebuggerPortService", {});
+	injector.register("lockfile", LockFileStub);
 	injector.register("iOSSimResolver", {
 		iOSSim: {
 			getRunningSimulators: async () => currentlyRunningSimulators

lib/common/test/unit-tests/stubs.ts

@@ -3,6 +3,13 @@
 import * as util from "util";
 import { EventEmitter } from "events";
 
+export class LockFileStub implements ILockFile {
+	public async executeActionWithLock<T>(action: () => Promise<T>, lockFilePath?: string, lockFileOpts?: ILockFileOptions): Promise<T> {
+		const result = await action();
+		return result;
+	}
+}
+
 export class CommonLoggerStub implements ILogger {
 	setLevel(level: string): void { }
 	getLevel(): string { return undefined; }

lib/definitions/lockfile.d.ts

@@ -0,0 +1,19 @@
+import * as lockfile from "lockfile";
+
+declare global {
+    interface ILockFileOptions extends lockfile.Options { }
+
+    /**
+     * Describes methods that can be used to use file locking.
+     */
+    interface ILockFile {
+        /**
+         * @param action The code to be locked.
+         * @param {string} lockFilePath Path to lockfile that has to be created. Defaults to `<profile dir>/lockfile.lock`
+         * @param {ILockFileOptions} lockFileOpts Options used for creating the lockfile.
+         * @returns {Promise<T>}
+         */
+        executeActionWithLock<T>(action: () => Promise<T>, lockFilePath?: string, lockFileOpts?: ILockFileOptions): Promise<T>
+        // TODO: expose as decorator
+    }
+}