From 233e6aae88046c97eccb14d92b7a6fdd1c0df79e Mon Sep 17 00:00:00 2001 From: DimitarTachev Date: Thu, 10 Jan 2019 11:25:00 +0200 Subject: [PATCH 1/4] fix: fix `connectEventuallyUntilTimeout` by retrying on exceptions --- lib/common/helpers.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/common/helpers.ts b/lib/common/helpers.ts index c355e3a6dd..886754d424 100644 --- a/lib/common/helpers.ts +++ b/lib/common/helpers.ts @@ -495,6 +495,7 @@ export async function connectEventuallyUntilTimeout(factory: () => Promise Date: Thu, 10 Jan 2019 11:34:17 +0200 Subject: [PATCH 2/4] fix: execute only attach request in order to enable the ios debugging We don't need the AttachAvailability response as now we don't need different actions based on the response. We just need to get the debug port which is printed by the AttachRequest and the Runtime is now accepting connections regardless of its state. This also allows us to remove the availability calls from the runtime. --- lib/declarations.d.ts | 2 +- .../ios/app-debug-socket-proxy-factory.ts | 2 +- .../ios/socket-request-executor.ts | 62 ++++--------------- lib/services/android-device-debug-service.ts | 2 +- 4 files changed, 14 insertions(+), 54 deletions(-) diff --git a/lib/declarations.d.ts b/lib/declarations.d.ts index 5f948e84b6..ecfd0ccf22 100644 --- a/lib/declarations.d.ts +++ b/lib/declarations.d.ts @@ -755,7 +755,7 @@ interface IiOSNotification extends NodeJS.EventEmitter { } interface IiOSSocketRequestExecutor { - executeLaunchRequest(deviceIdentifier: string, timeout: number, readyForAttachTimeout: number, projectId: string, debugOptions: IDebugOptions): Promise; + executeLaunchRequest(device: Mobile.IiOSDevice, timeout: number, readyForAttachTimeout: number, projectId: string, debugOptions: IDebugOptions): Promise; executeAttachRequest(device: Mobile.IiOSDevice, timeout: number, projectId: string): Promise; } diff --git a/lib/device-sockets/ios/app-debug-socket-proxy-factory.ts b/lib/device-sockets/ios/app-debug-socket-proxy-factory.ts index 18448d876a..10cebd65ea 100644 --- a/lib/device-sockets/ios/app-debug-socket-proxy-factory.ts +++ b/lib/device-sockets/ios/app-debug-socket-proxy-factory.ts @@ -123,7 +123,7 @@ export class AppDebugSocketProxyFactory extends EventEmitter implements IAppDebu this.$logger.trace(err); this.emit(CONNECTION_ERROR_EVENT_NAME, err); acceptHandshake = false; - this.$logger.warn(`Cannot connect to device socket. The error message is '${err.message}'. Try starting the application manually.`); + this.$logger.warn(`Cannot connect to device socket. The error message is '${err.message}'.`); } callback(acceptHandshake); diff --git a/lib/device-sockets/ios/socket-request-executor.ts b/lib/device-sockets/ios/socket-request-executor.ts index c3eb4ccf5d..9dd85844a2 100644 --- a/lib/device-sockets/ios/socket-request-executor.ts +++ b/lib/device-sockets/ios/socket-request-executor.ts @@ -8,47 +8,21 @@ export class IOSSocketRequestExecutor implements IiOSSocketRequestExecutor { public async executeAttachRequest(device: Mobile.IiOSDevice, timeout: number, projectId: string): Promise { const deviceIdentifier = device.deviceInfo.identifier; - - const observeNotificationSockets = [ - await this.$iOSNotificationService.postNotification(deviceIdentifier, this.$iOSNotification.getAlreadyConnected(projectId), constants.IOS_OBSERVE_NOTIFICATION_COMMAND_TYPE), - await this.$iOSNotificationService.postNotification(deviceIdentifier, this.$iOSNotification.getReadyForAttach(projectId), constants.IOS_OBSERVE_NOTIFICATION_COMMAND_TYPE), - await this.$iOSNotificationService.postNotification(deviceIdentifier, this.$iOSNotification.getAttachAvailable(projectId), constants.IOS_OBSERVE_NOTIFICATION_COMMAND_TYPE) - ]; - - const observeNotificationPromises = _(observeNotificationSockets) - .uniq() - .map(s => { - return this.$iOSNotificationService.awaitNotification(deviceIdentifier, s, timeout); - }) - .value(); - - // Trigger the notifications update. - await this.$iOSNotificationService.postNotification(deviceIdentifier, this.$iOSNotification.getAttachAvailabilityQuery(projectId)); - - let receivedNotification: string; try { - receivedNotification = await Promise.race(observeNotificationPromises); + // We should create this promise here because we need to send the ObserveNotification on the device + // before we send the PostNotification. + const readyForAttachSocket = await this.$iOSNotificationService.postNotification(deviceIdentifier, this.$iOSNotification.getReadyForAttach(projectId), constants.IOS_OBSERVE_NOTIFICATION_COMMAND_TYPE); + const readyForAttachPromise = this.$iOSNotificationService.awaitNotification(deviceIdentifier, +readyForAttachSocket, timeout); + await this.$iOSNotificationService.postNotification(deviceIdentifier, this.$iOSNotification.getAttachRequest(projectId, deviceIdentifier)); + await readyForAttachPromise; } catch (e) { - this.$errors.failWithoutHelp(`The application ${projectId} does not appear to be running on ${device.deviceInfo.displayName} or is not built with debugging enabled.`); - } - - switch (receivedNotification) { - case this.$iOSNotification.getAlreadyConnected(projectId): - this.$errors.failWithoutHelp("A client is already connected."); - break; - case this.$iOSNotification.getAttachAvailable(projectId): - await this.executeAttachAvailable(deviceIdentifier, projectId, timeout); - break; - case this.$iOSNotification.getReadyForAttach(projectId): - break; - default: - this.$logger.trace("Response from attach availability query:"); - this.$logger.trace(receivedNotification); - this.$errors.failWithoutHelp("No notification received while executing attach request."); + this.$errors.failWithoutHelp(`The application ${projectId} does not appear to be running on ${deviceIdentifier} or is not built with debugging enabled. Try starting the application manually.`); } } - public async executeLaunchRequest(deviceIdentifier: string, timeout: number, readyForAttachTimeout: number, projectId: string, debugOptions: IDebugOptions): Promise { + public async executeLaunchRequest(device: Mobile.IiOSDevice, timeout: number, readyForAttachTimeout: number, projectId: string, debugOptions: IDebugOptions): Promise { + const deviceIdentifier = device.deviceInfo.identifier; + try { if (!debugOptions.skipHandshake) { await this.executeHandshake(deviceIdentifier, projectId, timeout); @@ -58,27 +32,13 @@ export class IOSSocketRequestExecutor implements IiOSSocketRequestExecutor { await this.$iOSNotificationService.postNotification(deviceIdentifier, this.$iOSNotification.getWaitForDebug(projectId)); } - await this.executeAttachAvailable(deviceIdentifier, projectId, readyForAttachTimeout); + await this.executeAttachRequest(device, readyForAttachTimeout, projectId); } catch (e) { this.$logger.trace("Launch request error: ", e); this.$errors.failWithoutHelp("Error while waiting for response from NativeScript runtime."); } } - private async executeAttachAvailable(deviceIdentifier: string, projectId: string, timeout: number): Promise { - try { - // We should create this promise here because we need to send the ObserveNotification on the device - // before we send the PostNotification. - const readyForAttachSocket = await this.$iOSNotificationService.postNotification(deviceIdentifier, this.$iOSNotification.getReadyForAttach(projectId), constants.IOS_OBSERVE_NOTIFICATION_COMMAND_TYPE); - const readyForAttachPromise = this.$iOSNotificationService.awaitNotification(deviceIdentifier, +readyForAttachSocket, timeout); - await this.$iOSNotificationService.postNotification(deviceIdentifier, this.$iOSNotification.getAttachRequest(projectId, deviceIdentifier)); - await readyForAttachPromise; - } catch (e) { - this.$logger.trace("Attach available error: ", e); - this.$errors.failWithoutHelp(`The application ${projectId} timed out when performing the socket handshake.`); - } - } - private async executeHandshake(deviceIdentifier: string, projectId: string, timeout: number): Promise { // This notification will be send only once by the runtime during application start. // In case app is already running, we'll fail here as we'll not receive it. diff --git a/lib/services/android-device-debug-service.ts b/lib/services/android-device-debug-service.ts index 110946dbef..8b6cf0106a 100644 --- a/lib/services/android-device-debug-service.ts +++ b/lib/services/android-device-debug-service.ts @@ -147,7 +147,7 @@ export class AndroidDeviceDebugService extends DebugServiceBase implements IDevi private async validateRunningApp(deviceId: string, packageName: string): Promise { if (!(await this.isAppRunning(packageName, deviceId))) { - this.$errors.failWithoutHelp(`The application ${packageName} does not appear to be running on ${deviceId} or is not built with debugging enabled.`); + this.$errors.failWithoutHelp(`The application ${packageName} does not appear to be running on ${deviceId} or is not built with debugging enabled. Try starting the application manually.`); } } From d12300e983eb4e6048baab31d8d1de7c4a3b197e Mon Sep 17 00:00:00 2001 From: DimitarTachev Date: Thu, 10 Jan 2019 11:36:01 +0200 Subject: [PATCH 3/4] 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 --- lib/common/declarations.d.ts | 61 ------------------- lib/common/mobile/ios/device/ios-device.ts | 2 + lib/common/mobile/ios/ios-device-base.ts | 25 ++++---- .../ios/simulator/ios-simulator-device.ts | 1 + lib/common/services/lockfile.ts | 51 ++++++++++++---- lib/common/services/user-settings-service.ts | 18 ++---- .../mobile/ios-simulator-discovery.ts | 3 +- lib/common/test/unit-tests/stubs.ts | 7 +++ lib/definitions/lockfile.d.ts | 19 ++++++ 9 files changed, 89 insertions(+), 98 deletions(-) create mode 100644 lib/definitions/lockfile.d.ts diff --git a/lib/common/declarations.d.ts b/lib/common/declarations.d.ts index 39587c2024..c7b6e8c8db 100644 --- a/lib/common/declarations.d.ts +++ b/lib/common/declarations.d.ts @@ -1958,67 +1958,6 @@ interface IiOSNotificationService { postNotification(deviceIdentifier: string, notification: string, commandType?: string): Promise; } -/** - * 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 `/lockfile.lock` - * @param {ILockFileOptions} lockFileOpts Options used for creating the lockfile. - * @returns {Promise} - */ - lock(lockFilePath?: string, lockFileOpts?: ILockFileOptions): Promise; - - /** - * Close and unlink the lockfile. - * @param {string} lockFilePath Path to lockfile that has to be created. Defaults to `/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 `/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 diff --git a/lib/common/mobile/ios/device/ios-device.ts b/lib/common/mobile/ios/device/ios-device.ts index f0d3b7a223..bebb20828d 100644 --- a/lib/common/mobile/ios/device/ios-device.ts +++ b/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]); diff --git a/lib/common/mobile/ios/ios-device-base.ts b/lib/common/mobile/ios/ios-device-base.ts index c2ef1c4602..ee34547eff 100644 --- a/lib/common/mobile/ios/ios-device-base.ts +++ b/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 { - 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) { diff --git a/lib/common/mobile/ios/simulator/ios-simulator-device.ts b/lib/common/mobile/ios/simulator/ios-simulator-device.ts index 1a50da4617..5d2144fecb 100644 --- a/lib/common/mobile/ios/simulator/ios-simulator-device.ts +++ b/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, diff --git a/lib/common/services/lockfile.ts b/lib/common/services/lockfile.ts index 8612e684e8..faaffb02b1 100644 --- a/lib/common/services/lockfile.ts +++ b/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 { + public lock(lockFilePath?: string, lockFileOpts?: ILockFileOptions): Promise { 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((resolve, reject) => { + return new Promise((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(action: () => Promise, lockFilePath?: string, lockFileOpts?: ILockFileOptions): Promise { + 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; diff --git a/lib/common/services/user-settings-service.ts b/lib/common/services/user-settings-service.ts index f37337d4e3..5c9a99c253 100644 --- a/lib/common/services/user-settings-service.ts +++ b/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(action); + return this.$lockfile.executeActionWithLock(action, this.lockFilePath); } public async saveSetting(key: string, value: T): Promise { @@ -39,17 +39,7 @@ export class UserSettingsServiceBase implements IUserSettingsService { await this.saveSettings(); }; - return this.executeActionWithLock(action); - } - - private async executeActionWithLock(action: () => Promise): Promise { - try { - await this.$lockfile.lock(this.lockFilePath); - const result = await action(); - return result; - } finally { - this.$lockfile.unlock(this.lockFilePath); - } + return this.$lockfile.executeActionWithLock(action, this.lockFilePath); } public saveSettings(data?: any): Promise { @@ -66,7 +56,7 @@ export class UserSettingsServiceBase implements IUserSettingsService { this.$fs.writeJson(this.userSettingsFilePath, this.userSettingsData); }; - return this.executeActionWithLock(action); + return this.$lockfile.executeActionWithLock(action, this.lockFilePath); } // TODO: Remove Promise, reason: writeFile - blocked as other implementation of the interface has async operation. diff --git a/lib/common/test/unit-tests/mobile/ios-simulator-discovery.ts b/lib/common/test/unit-tests/mobile/ios-simulator-discovery.ts index 406a501fe6..e5480fcc52 100644 --- a/lib/common/test/unit-tests/mobile/ios-simulator-discovery.ts +++ b/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 diff --git a/lib/common/test/unit-tests/stubs.ts b/lib/common/test/unit-tests/stubs.ts index 52f17bc965..8e475030eb 100644 --- a/lib/common/test/unit-tests/stubs.ts +++ b/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(action: () => Promise, lockFilePath?: string, lockFileOpts?: ILockFileOptions): Promise { + const result = await action(); + return result; + } +} + export class CommonLoggerStub implements ILogger { setLevel(level: string): void { } getLevel(): string { return undefined; } diff --git a/lib/definitions/lockfile.d.ts b/lib/definitions/lockfile.d.ts new file mode 100644 index 0000000000..ef41a5a2d2 --- /dev/null +++ b/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 `/lockfile.lock` + * @param {ILockFileOptions} lockFileOpts Options used for creating the lockfile. + * @returns {Promise} + */ + executeActionWithLock(action: () => Promise, lockFilePath?: string, lockFileOpts?: ILockFileOptions): Promise + // TODO: expose as decorator + } +} \ No newline at end of file From 2135c65e71764e595278847c64432435519147eb Mon Sep 17 00:00:00 2001 From: DimitarTachev Date: Thu, 10 Jan 2019 15:59:17 +0200 Subject: [PATCH 4/4] fix: fix pr comments --- lib/common/bootstrap.ts | 2 +- lib/common/mobile/ios/device/ios-device.ts | 2 +- lib/common/mobile/ios/ios-device-base.ts | 4 +- .../ios/simulator/ios-simulator-device.ts | 2 +- .../services/{lockfile.ts => lock-service.ts} | 46 +++++++++---------- lib/common/services/user-settings-service.ts | 8 ++-- .../mobile/ios-simulator-discovery.ts | 4 +- lib/common/test/unit-tests/stubs.ts | 4 +- lib/declarations.d.ts | 6 --- lib/definitions/debug.d.ts | 4 -- lib/definitions/lock-service.d.ts | 19 ++++++++ lib/definitions/lockfile.d.ts | 19 -------- lib/device-sockets/ios/notification.ts | 25 ---------- .../ios/socket-request-executor.ts | 30 +----------- lib/services/livesync/livesync-service.ts | 1 - lib/services/user-settings-service.ts | 4 +- test/stubs.ts | 13 ------ 17 files changed, 56 insertions(+), 137 deletions(-) rename lib/common/services/{lockfile.ts => lock-service.ts} (76%) create mode 100644 lib/definitions/lock-service.d.ts delete mode 100644 lib/definitions/lockfile.d.ts diff --git a/lib/common/bootstrap.ts b/lib/common/bootstrap.ts index 2aa92eee6b..2aa7a2ddb7 100644 --- a/lib/common/bootstrap.ts +++ b/lib/common/bootstrap.ts @@ -122,4 +122,4 @@ $injector.require("qr", "./services/qr"); $injector.require("printPluginsService", "./services/plugins/print-plugins-service"); $injector.require("npmPluginsService", "./services/plugins/npm-plugins-service"); -$injector.require("lockfile", "./services/lockfile"); +$injector.require(["lockfile", "lockService"], "./services/lock-service"); diff --git a/lib/common/mobile/ios/device/ios-device.ts b/lib/common/mobile/ios/device/ios-device.ts index bebb20828d..b53dec43c2 100644 --- a/lib/common/mobile/ios/device/ios-device.ts +++ b/lib/common/mobile/ios/device/ios-device.ts @@ -17,7 +17,7 @@ export class IOSDevice extends IOSDeviceBase { protected $errors: IErrors, private $injector: IInjector, protected $iOSDebuggerPortService: IIOSDebuggerPortService, - protected $lockfile: ILockFile, + protected $lockService: ILockService, private $iOSSocketRequestExecutor: IiOSSocketRequestExecutor, protected $processService: IProcessService, private $deviceLogProvider: Mobile.IDeviceLogProvider, diff --git a/lib/common/mobile/ios/ios-device-base.ts b/lib/common/mobile/ios/ios-device-base.ts index ee34547eff..fd384c8de8 100644 --- a/lib/common/mobile/ios/ios-device-base.ts +++ b/lib/common/mobile/ios/ios-device-base.ts @@ -5,7 +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; + protected abstract $lockService: ILockService; abstract deviceInfo: Mobile.IDeviceInfo; abstract applicationManager: Mobile.IDeviceApplicationManager; abstract fileSystem: Mobile.IDeviceFileSystem; @@ -25,7 +25,7 @@ export abstract class IOSDeviceBase implements Mobile.IiOSDevice { } public async getSocket(appId: string): Promise { - return this.$lockfile.executeActionWithLock(async () => { + return this.$lockService.executeActionWithLock(async () => { if (this.cachedSockets[appId]) { return this.cachedSockets[appId]; } diff --git a/lib/common/mobile/ios/simulator/ios-simulator-device.ts b/lib/common/mobile/ios/simulator/ios-simulator-device.ts index 5d2144fecb..5a34ba89c0 100644 --- a/lib/common/mobile/ios/simulator/ios-simulator-device.ts +++ b/lib/common/mobile/ios/simulator/ios-simulator-device.ts @@ -14,7 +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, + protected $lockService: ILockService, private $injector: IInjector, protected $iOSDebuggerPortService: IIOSDebuggerPortService, private $iOSSimResolver: Mobile.IiOSSimResolver, diff --git a/lib/common/services/lockfile.ts b/lib/common/services/lock-service.ts similarity index 76% rename from lib/common/services/lockfile.ts rename to lib/common/services/lock-service.ts index faaffb02b1..807a421006 100644 --- a/lib/common/services/lockfile.ts +++ b/lib/common/services/lock-service.ts @@ -2,7 +2,7 @@ import * as lockfile from "lockfile"; import * as path from "path"; import { cache } from "../decorators"; -export class LockFile implements ILockFile { +export class LockService implements ILockService { private currentlyLockedFiles: string[] = []; @cache() @@ -14,10 +14,10 @@ export class LockFile implements ILockFile { return path.join(this.$settingsService.getProfileDir(), relativeLockFilePath); } - private get defaultLockParams(): ILockFileOptions { + private get defaultLockParams(): ILockOptions { // 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 the `stale` value, consider it stale and try to get a new lock. - const lockParams: ILockFileOptions = { + const lockParams: ILockOptions = { retryWait: 100, retries: 100, stale: 30 * 1000, @@ -37,8 +37,19 @@ export class LockFile implements ILockFile { }); } - public lock(lockFilePath?: string, lockFileOpts?: ILockFileOptions): Promise { - const { filePath, fileOpts } = this.getLockFileSettings(lockFilePath, lockFileOpts); + public async executeActionWithLock(action: () => Promise, lockFilePath?: string, lockOpts?: ILockOptions): Promise { + const resolvedLockFilePath = await this.lock(lockFilePath, lockOpts); + + try { + const result = await action(); + return result; + } finally { + this.unlock(resolvedLockFilePath); + } + } + + private lock(lockFilePath?: string, lockOpts?: ILockOptions): Promise { + const { filePath, fileOpts } = this.getLockFileSettings(lockFilePath, lockOpts); this.currentlyLockedFiles.push(filePath); // Prevent ENOENT error when the dir, where lock should be created, does not exist. @@ -51,30 +62,13 @@ export class LockFile implements ILockFile { }); } - public async executeActionWithLock(action: () => Promise, lockFilePath?: string, lockFileOpts?: ILockFileOptions): Promise { - const resolvedLockFilePath = await this.lock(lockFilePath, lockFileOpts); - - try { - const result = await action(); - return result; - } finally { - this.unlock(resolvedLockFilePath); - } - } - - public unlock(lockFilePath?: string): void { + private unlock(lockFilePath?: string): void { const { filePath } = this.getLockFileSettings(lockFilePath); _.remove(this.currentlyLockedFiles, e => e === lockFilePath); lockfile.unlockSync(filePath); } - public check(lockFilePath?: string, lockFileOpts?: ILockFileOptions): boolean { - const { filePath, fileOpts } = this.getLockFileSettings(lockFilePath, lockFileOpts); - - return lockfile.checkSync(filePath, fileOpts); - } - - private getLockFileSettings(filePath?: string, fileOpts?: ILockFileOptions): { filePath: string, fileOpts: ILockFileOptions } { + private getLockFileSettings(filePath?: string, fileOpts?: ILockOptions): { filePath: string, fileOpts: ILockOptions } { if (filePath && !path.isAbsolute(filePath)) { filePath = this.getAbsoluteLockFilePath(filePath); } @@ -89,4 +83,6 @@ export class LockFile implements ILockFile { } } -$injector.register("lockfile", LockFile); +$injector.register("lockService", LockService); +// backwards compatibility +$injector.register("lockfile", LockService); diff --git a/lib/common/services/user-settings-service.ts b/lib/common/services/user-settings-service.ts index 5c9a99c253..d10954b616 100644 --- a/lib/common/services/user-settings-service.ts +++ b/lib/common/services/user-settings-service.ts @@ -10,7 +10,7 @@ export class UserSettingsServiceBase implements IUserSettingsService { constructor(userSettingsFilePath: string, protected $fs: IFileSystem, - protected $lockfile: ILockFile, + protected $lockService: ILockService, private $logger: ILogger) { this.userSettingsFilePath = userSettingsFilePath; } @@ -21,7 +21,7 @@ export class UserSettingsServiceBase implements IUserSettingsService { return this.userSettingsData ? this.userSettingsData[settingName] : null; }; - return this.$lockfile.executeActionWithLock(action, this.lockFilePath); + return this.$lockService.executeActionWithLock(action, this.lockFilePath); } public async saveSetting(key: string, value: T): Promise { @@ -39,7 +39,7 @@ export class UserSettingsServiceBase implements IUserSettingsService { await this.saveSettings(); }; - return this.$lockfile.executeActionWithLock(action, this.lockFilePath); + return this.$lockService.executeActionWithLock(action, this.lockFilePath); } public saveSettings(data?: any): Promise { @@ -56,7 +56,7 @@ export class UserSettingsServiceBase implements IUserSettingsService { this.$fs.writeJson(this.userSettingsFilePath, this.userSettingsData); }; - return this.$lockfile.executeActionWithLock(action, this.lockFilePath); + return this.$lockService.executeActionWithLock(action, this.lockFilePath); } // TODO: Remove Promise, reason: writeFile - blocked as other implementation of the interface has async operation. diff --git a/lib/common/test/unit-tests/mobile/ios-simulator-discovery.ts b/lib/common/test/unit-tests/mobile/ios-simulator-discovery.ts index e5480fcc52..f6c4cb0d31 100644 --- a/lib/common/test/unit-tests/mobile/ios-simulator-discovery.ts +++ b/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, LockFileStub } from "../stubs"; +import { ErrorsStub, CommonLoggerStub, HooksServiceStub, LockServiceStub } from "../stubs"; import { FileSystemStub } from "../../../../../test/stubs"; let currentlyRunningSimulators: Mobile.IiSimDevice[]; @@ -16,7 +16,7 @@ function createTestInjector(): IInjector { injector.register("injector", injector); injector.register("errors", ErrorsStub); injector.register("iOSDebuggerPortService", {}); - injector.register("lockfile", LockFileStub); + injector.register("lockService", LockServiceStub); injector.register("iOSSimResolver", { iOSSim: { getRunningSimulators: async () => currentlyRunningSimulators diff --git a/lib/common/test/unit-tests/stubs.ts b/lib/common/test/unit-tests/stubs.ts index 8e475030eb..b7dfee7f17 100644 --- a/lib/common/test/unit-tests/stubs.ts +++ b/lib/common/test/unit-tests/stubs.ts @@ -3,8 +3,8 @@ import * as util from "util"; import { EventEmitter } from "events"; -export class LockFileStub implements ILockFile { - public async executeActionWithLock(action: () => Promise, lockFilePath?: string, lockFileOpts?: ILockFileOptions): Promise { +export class LockServiceStub implements ILockService { + public async executeActionWithLock(action: () => Promise, lockFilePath?: string, lockOpts?: ILockOptions): Promise { const result = await action(); return result; } diff --git a/lib/declarations.d.ts b/lib/declarations.d.ts index ecfd0ccf22..8b8bb6c328 100644 --- a/lib/declarations.d.ts +++ b/lib/declarations.d.ts @@ -745,17 +745,11 @@ interface IAppDebugSocketProxyFactory extends NodeJS.EventEmitter { } interface IiOSNotification extends NodeJS.EventEmitter { - getWaitForDebug(projectId: string): string; getAttachRequest(projectId: string, deviceId: string): string; - getAppLaunching(projectId: string): string; getReadyForAttach(projectId: string): string; - getAttachAvailabilityQuery(projectId: string): string; - getAlreadyConnected(projectId: string): string; - getAttachAvailable(projectId: string): string; } interface IiOSSocketRequestExecutor { - executeLaunchRequest(device: Mobile.IiOSDevice, timeout: number, readyForAttachTimeout: number, projectId: string, debugOptions: IDebugOptions): Promise; executeAttachRequest(device: Mobile.IiOSDevice, timeout: number, projectId: string): Promise; } diff --git a/lib/definitions/debug.d.ts b/lib/definitions/debug.d.ts index 48bbba18f8..c2586789b4 100644 --- a/lib/definitions/debug.d.ts +++ b/lib/definitions/debug.d.ts @@ -96,10 +96,6 @@ interface IDebugOptions { * The sdk version of the emulator. */ sdk?: string; - /** - * Defines if the handshake(AppLaunching notification) between CLI and runtime should be executed. The handshake is not needed when CLI retries to attach to the debugger. - */ - skipHandshake?: boolean; /** * Forces the debugger attach event to be emitted. */ diff --git a/lib/definitions/lock-service.d.ts b/lib/definitions/lock-service.d.ts new file mode 100644 index 0000000000..66c21d1a6c --- /dev/null +++ b/lib/definitions/lock-service.d.ts @@ -0,0 +1,19 @@ +import * as lockfile from "lockfile"; + +declare global { + interface ILockOptions extends lockfile.Options { } + + /** + * Describes methods that can be used to use file locking. + */ + interface ILockService { + /** + * @param action The code to be locked. + * @param {string} lockFilePath Path to lock file that has to be created. Defaults to `/lockfile.lock` + * @param {ILockOptions} lockOpts Options used for creating the lock file. + * @returns {Promise} + */ + executeActionWithLock(action: () => Promise, lockFilePath?: string, lockOpts?: ILockOptions): Promise + // TODO: expose as decorator + } +} \ No newline at end of file diff --git a/lib/definitions/lockfile.d.ts b/lib/definitions/lockfile.d.ts deleted file mode 100644 index ef41a5a2d2..0000000000 --- a/lib/definitions/lockfile.d.ts +++ /dev/null @@ -1,19 +0,0 @@ -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 `/lockfile.lock` - * @param {ILockFileOptions} lockFileOpts Options used for creating the lockfile. - * @returns {Promise} - */ - executeActionWithLock(action: () => Promise, lockFilePath?: string, lockFileOpts?: ILockFileOptions): Promise - // TODO: expose as decorator - } -} \ No newline at end of file diff --git a/lib/device-sockets/ios/notification.ts b/lib/device-sockets/ios/notification.ts index d72e46057e..a9e72eb23e 100644 --- a/lib/device-sockets/ios/notification.ts +++ b/lib/device-sockets/ios/notification.ts @@ -2,17 +2,8 @@ import { EventEmitter } from "events"; import { ATTACH_REQUEST_EVENT_NAME } from "../../common/constants"; export class IOSNotification extends EventEmitter implements IiOSNotification { - private static WAIT_FOR_DEBUG_NOTIFICATION_NAME = "WaitForDebugger"; private static ATTACH_REQUEST_NOTIFICATION_NAME = "AttachRequest"; - private static APP_LAUNCHING_NOTIFICATION_NAME = "AppLaunching"; private static READY_FOR_ATTACH_NOTIFICATION_NAME = "ReadyForAttach"; - private static ATTACH_AVAILABILITY_QUERY_NOTIFICATION_NAME = "AttachAvailabilityQuery"; - private static ALREADY_CONNECTED_NOTIFICATION_NAME = "AlreadyConnected"; - private static ATTACH_AVAILABLE_NOTIFICATION_NAME = "AttachAvailable"; - - public getWaitForDebug(projectId: string) { - return this.formatNotification(IOSNotification.WAIT_FOR_DEBUG_NOTIFICATION_NAME, projectId); - } public getAttachRequest(appId: string, deviceId: string): string { // It could be too early to emit this event, but we rely that if you construct attach request, @@ -21,26 +12,10 @@ export class IOSNotification extends EventEmitter implements IiOSNotification { return this.formatNotification(IOSNotification.ATTACH_REQUEST_NOTIFICATION_NAME, appId); } - public getAppLaunching(projectId: string): string { - return this.formatNotification(IOSNotification.APP_LAUNCHING_NOTIFICATION_NAME, projectId); - } - public getReadyForAttach(projectId: string): string { return this.formatNotification(IOSNotification.READY_FOR_ATTACH_NOTIFICATION_NAME, projectId); } - public getAttachAvailabilityQuery(projectId: string) { - return this.formatNotification(IOSNotification.ATTACH_AVAILABILITY_QUERY_NOTIFICATION_NAME, projectId); - } - - public getAlreadyConnected(projectId: string) { - return this.formatNotification(IOSNotification.ALREADY_CONNECTED_NOTIFICATION_NAME, projectId); - } - - public getAttachAvailable(projectId: string) { - return this.formatNotification(IOSNotification.ATTACH_AVAILABLE_NOTIFICATION_NAME, projectId); - } - private formatNotification(notification: string, projectId: string) { return `${projectId}:NativeScript.Debug.${notification}`; } diff --git a/lib/device-sockets/ios/socket-request-executor.ts b/lib/device-sockets/ios/socket-request-executor.ts index 9dd85844a2..b593815c83 100644 --- a/lib/device-sockets/ios/socket-request-executor.ts +++ b/lib/device-sockets/ios/socket-request-executor.ts @@ -3,8 +3,7 @@ import * as constants from "../../common/constants"; export class IOSSocketRequestExecutor implements IiOSSocketRequestExecutor { constructor(private $errors: IErrors, private $iOSNotification: IiOSNotification, - private $iOSNotificationService: IiOSNotificationService, - private $logger: ILogger) { } + private $iOSNotificationService: IiOSNotificationService) { } public async executeAttachRequest(device: Mobile.IiOSDevice, timeout: number, projectId: string): Promise { const deviceIdentifier = device.deviceInfo.identifier; @@ -19,33 +18,6 @@ export class IOSSocketRequestExecutor implements IiOSSocketRequestExecutor { this.$errors.failWithoutHelp(`The application ${projectId} does not appear to be running on ${deviceIdentifier} or is not built with debugging enabled. Try starting the application manually.`); } } - - public async executeLaunchRequest(device: Mobile.IiOSDevice, timeout: number, readyForAttachTimeout: number, projectId: string, debugOptions: IDebugOptions): Promise { - const deviceIdentifier = device.deviceInfo.identifier; - - try { - if (!debugOptions.skipHandshake) { - await this.executeHandshake(deviceIdentifier, projectId, timeout); - } - - if (debugOptions.debugBrk) { - await this.$iOSNotificationService.postNotification(deviceIdentifier, this.$iOSNotification.getWaitForDebug(projectId)); - } - - await this.executeAttachRequest(device, readyForAttachTimeout, projectId); - } catch (e) { - this.$logger.trace("Launch request error: ", e); - this.$errors.failWithoutHelp("Error while waiting for response from NativeScript runtime."); - } - } - - private async executeHandshake(deviceIdentifier: string, projectId: string, timeout: number): Promise { - // This notification will be send only once by the runtime during application start. - // In case app is already running, we'll fail here as we'll not receive it. - const appLaunchingNotification = this.$iOSNotification.getAppLaunching(projectId); - const appLaunchingSocket = await this.$iOSNotificationService.postNotification(deviceIdentifier, appLaunchingNotification, constants.IOS_OBSERVE_NOTIFICATION_COMMAND_TYPE); - await this.$iOSNotificationService.awaitNotification(deviceIdentifier, +appLaunchingSocket, timeout); - } } $injector.register("iOSSocketRequestExecutor", IOSSocketRequestExecutor); diff --git a/lib/services/livesync/livesync-service.ts b/lib/services/livesync/livesync-service.ts index 38e3b8603d..f39e165ef2 100644 --- a/lib/services/livesync/livesync-service.ts +++ b/lib/services/livesync/livesync-service.ts @@ -329,7 +329,6 @@ export class LiveSyncService extends EventEmitter implements IDebugLiveSyncServi } catch (err) { this.$logger.trace("Couldn't attach debugger, will modify options and try again.", err); attachDebuggerOptions.debugOptions.start = false; - attachDebuggerOptions.debugOptions.skipHandshake = true; try { debugInformation = await this.attachDebugger(attachDebuggerOptions); } catch (innerErr) { diff --git a/lib/services/user-settings-service.ts b/lib/services/user-settings-service.ts index 8f382aadf7..6e20579c3d 100644 --- a/lib/services/user-settings-service.ts +++ b/lib/services/user-settings-service.ts @@ -4,10 +4,10 @@ import * as userSettingsServiceBaseLib from "../common/services/user-settings-se export class UserSettingsService extends userSettingsServiceBaseLib.UserSettingsServiceBase { constructor($fs: IFileSystem, $settingsService: ISettingsService, - $lockfile: ILockFile, + $lockService: ILockService, $logger: ILogger) { const userSettingsFilePath = path.join($settingsService.getProfileDir(), "user-settings.json"); - super(userSettingsFilePath, $fs, $lockfile, $logger); + super(userSettingsFilePath, $fs, $lockService, $logger); } public async loadUserSettingsFile(): Promise { diff --git a/test/stubs.ts b/test/stubs.ts index b06e61e7b8..8c5cb43069 100644 --- a/test/stubs.ts +++ b/test/stubs.ts @@ -564,19 +564,6 @@ export class HooksServiceStub implements IHooksService { hookArgsName = "hookArgs"; } -export class LockFile { - - async check(): Promise { - return false; - } - - async lock(): Promise { - } - - async unlock(): Promise { - } -} - export class PrompterStub implements IPrompter { private strings: IDictionary = {}; private passwords: IDictionary = {};