Merge pull request #1022 from telerik/vladimirov/fix-sidekick-justlaunch

Fix leaking handlers for iOS device logs

Author: rosen-vladimirov

appbuilder/device-emitter.ts

@@ -1,5 +1,5 @@
 import { EventEmitter } from "events";
-import { DeviceDiscoveryEventNames } from "../constants";
+import { DeviceDiscoveryEventNames, DEVICE_LOG_EVENT_NAME } from "../constants";
 
 export class DeviceEmitter extends EventEmitter {
 	constructor(private $deviceLogProvider: EventEmitter,
@@ -34,7 +34,7 @@ export class DeviceEmitter extends EventEmitter {
 		});
 
 		this.$deviceLogProvider.on("data", (identifier: string, data: any) => {
-			this.emit('deviceLogData', identifier, data.toString());
+			this.emit(DEVICE_LOG_EVENT_NAME, identifier, data.toString());
 		});
 	}
 

constants.ts

@@ -39,6 +39,8 @@ export class DeviceDiscoveryEventNames {
 	static DEVICE_LOST = "deviceLost";
 }
 
+export const DEVICE_LOG_EVENT_NAME = "deviceLogData";
+
 export const TARGET_FRAMEWORK_IDENTIFIERS = {
 	Cordova: "Cordova",
 	NativeScript: "NativeScript"

definitions/mobile.d.ts

@@ -96,6 +96,12 @@ declare module Mobile {
 		isEmulator: boolean;
 		openDeviceLogStream(): Promise<void>;
 		getApplicationInfo(applicationIdentifier: string): Promise<Mobile.IApplicationInfo>;
+
+		/**
+		 * Called when device is lost. Its purpose is to clean any resources used by the instance.
+		 * @returns {void}
+		 */
+		detach?(): void;
 	}
 
 	interface IiOSDevice extends IDevice {
@@ -767,14 +773,14 @@ declare module Mobile {
 	}
 }
 
-interface IIOSDeviceOperations extends IDisposable {
+interface IIOSDeviceOperations extends IDisposable, NodeJS.EventEmitter {
 	install(ipaPath: string, deviceIdentifiers: string[], errorHandler?: DeviceOperationErrorHandler): Promise<IOSDeviceResponse>;
 
 	uninstall(appIdentifier: string, deviceIdentifiers: string[], errorHandler?: DeviceOperationErrorHandler): Promise<IOSDeviceResponse>;
 
 	startLookingForDevices(deviceFoundCallback: DeviceInfoCallback, deviceLostCallback: DeviceInfoCallback, options?: Mobile.IDeviceLookingOptions): Promise<void>;
 
-	startDeviceLog(deviceIdentifier: string, printLogFunction: (response: IOSDeviceLib.IDeviceLogData) => void): void;
+	startDeviceLog(deviceIdentifier: string): void;
 
 	apps(deviceIdentifiers: string[], errorHandler?: DeviceOperationErrorHandler): Promise<IOSDeviceAppInfo>;
 

mobile/ios/device/ios-device-operations.ts

@@ -1,14 +1,18 @@
 import { IOSDeviceLib as IOSDeviceLibModule } from "ios-device-lib";
 import { cache } from "../../../decorators";
+import { DEVICE_LOG_EVENT_NAME } from "../../../constants";
 import assert = require("assert");
+import { EventEmitter } from "events";
 
-export class IOSDeviceOperations implements IIOSDeviceOperations, IDisposable {
+export class IOSDeviceOperations extends EventEmitter implements IIOSDeviceOperations, IDisposable {
 	public isInitialized: boolean;
 	public shouldDispose: boolean;
 	private deviceLib: IOSDeviceLib.IOSDeviceLib;
 
 	constructor(private $logger: ILogger,
 		private $processService: IProcessService) {
+		super();
+
 		this.isInitialized = false;
 		this.shouldDispose = true;
 		this.$processService.attachToProcessExitSignals(this, () => {
@@ -67,15 +71,13 @@ export class IOSDeviceOperations implements IIOSDeviceOperations, IDisposable {
 		}
 	}
 
-	public startDeviceLog(deviceIdentifier: string, printLogFunction: (response: IOSDeviceLib.IDeviceLogData) => void): void {
+	public startDeviceLog(deviceIdentifier: string): void {
 		this.assertIsInitialized();
 		this.setShouldDispose(false);
 
 		this.$logger.trace(`Printing device log for device with identifier: ${deviceIdentifier}.`);
 
-		this.deviceLib.on("deviceLogData", (response: IOSDeviceLib.IDeviceLogData) => {
-			printLogFunction(response);
-		});
+		this.attacheDeviceLogDataHandler();
 
 		this.deviceLib.startDeviceLog([deviceIdentifier]);
 	}
@@ -230,6 +232,13 @@ export class IOSDeviceOperations implements IIOSDeviceOperations, IDisposable {
 	private assertIsInitialized(): void {
 		assert.ok(this.isInitialized, "iOS device operations not initialized.");
 	}
+
+	@cache()
+	private attacheDeviceLogDataHandler(): void {
+		this.deviceLib.on(DEVICE_LOG_EVENT_NAME, (response: IOSDeviceLib.IDeviceLogData) => {
+			this.emit(DEVICE_LOG_EVENT_NAME, response);
+		});
+	}
 }
 
 $injector.register("iosDeviceOperations", IOSDeviceOperations);

mobile/ios/device/ios-device.ts

@@ -2,13 +2,15 @@ import * as applicationManagerPath from "./ios-application-manager";
 import * as fileSystemPath from "./ios-device-file-system";
 import * as constants from "../../../constants";
 import * as net from "net";
+import { cache } from "../../../decorators";
 
 export class IOSDevice implements Mobile.IiOSDevice {
 	public applicationManager: Mobile.IDeviceApplicationManager;
 	public fileSystem: Mobile.IDeviceFileSystem;
 	public deviceInfo: Mobile.IDeviceInfo;
 
 	private _socket: net.Socket;
+	private _deviceLogHandler: Function;
 
 	constructor(private deviceActionInfo: IOSDeviceLib.IDeviceActionInfo,
 		private $injector: IInjector,
@@ -48,13 +50,24 @@ export class IOSDevice implements Mobile.IiOSDevice {
 		return this.applicationManager.getApplicationInfo(applicationIdentifier);
 	}
 
+	private actionOnDeviceLog(response: IOSDeviceLib.IDeviceLogData): void {
+		if (response.deviceId === this.deviceInfo.identifier) {
+			this.$deviceLogProvider.logData(response.message, this.$devicePlatformsConstants.iOS, this.deviceInfo.identifier);
+		}
+	}
+
+	@cache()
 	public async openDeviceLogStream(): Promise<void> {
 		if (this.deviceInfo.status !== constants.UNREACHABLE_STATUS) {
-			this.$iosDeviceOperations.startDeviceLog(this.deviceInfo.identifier, (response: IOSDeviceLib.IDeviceLogData) => {
-				if (response.deviceId === this.deviceInfo.identifier) {
-					this.$deviceLogProvider.logData(response.message, this.$devicePlatformsConstants.iOS, this.deviceInfo.identifier);
-				}
-			});
+			this.$iosDeviceOperations.startDeviceLog(this.deviceInfo.identifier);
+			this._deviceLogHandler = this.actionOnDeviceLog.bind(this);
+			this.$iosDeviceOperations.on(constants.DEVICE_LOG_EVENT_NAME, this._deviceLogHandler);
+		}
+	}
+
+	public detach(): void {
+		if (this._deviceLogHandler) {
+			this.$iosDeviceOperations.removeListener(constants.DEVICE_LOG_EVENT_NAME, this._deviceLogHandler);
 		}
 	}
 

mobile/mobile-core/devices-service.ts

@@ -139,6 +139,11 @@ export class DevicesService extends EventEmitter implements Mobile.IDevicesServi
 
 	private onDeviceLost(device: Mobile.IDevice): void {
 		this.$logger.trace(`Lost device with identifier '${device.deviceInfo.identifier}'`);
+
+		if (device.detach) {
+			device.detach();
+		}
+
 		delete this._devices[device.deviceInfo.identifier];
 		this.emit(constants.DeviceDiscoveryEventNames.DEVICE_LOST, device);
 	}

test/unit-tests/appbuilder/device-emitter.ts

@@ -3,7 +3,7 @@ import { assert } from "chai";
 import { EventEmitter } from "events";
 import { ProjectConstants } from "../../../appbuilder/project-constants";
 import { DeviceEmitter } from "../../../appbuilder/device-emitter";
-import { DeviceDiscoveryEventNames } from "../../../constants";
+import { DeviceDiscoveryEventNames, DEVICE_LOG_EVENT_NAME } from "../../../constants";
 // Injector dependencies must be classes.
 // EventEmitter is function, so our annotate method will fail.
 class CustomEventEmitter extends EventEmitter {
@@ -109,11 +109,11 @@ describe("deviceEmitter", () => {
 				deviceLogProvider = testInjector.resolve("deviceLogProvider");
 			});
 
-			describe("raises deviceLogData with correct identifier and data", () => {
+			describe(`raises ${DEVICE_LOG_EVENT_NAME} with correct identifier and data`, () => {
 				const expectedDeviceLogData = "This is some log data from device.";
 
 				const attachDeviceLogDataVerificationHandler = (expectedDeviceIdentifier: string, done: mocha.Done) => {
-					deviceEmitter.on("deviceLogData", (identifier: string, data: any) => {
+					deviceEmitter.on(DEVICE_LOG_EVENT_NAME, (identifier: string, data: any) => {
 						assert.deepEqual(identifier, expectedDeviceIdentifier);
 						assert.deepEqual(data, expectedDeviceLogData);
 						// Wait for all operations to be completed and call done after that.