From 0468853cab2cba264829b64835f6fd41a6013529 Mon Sep 17 00:00:00 2001 From: rosen-vladimirov Date: Tue, 30 Apr 2019 16:01:11 +0300 Subject: [PATCH] fix: logcat process is not restarted in some cases In case you have `keepSingleProcess` passed to logcat Stream (this option is passed when using CLI as a library - in this case we keep a long living logcat process alive and change only the filter for it by specifying different PIDs), in case the logcat process dies (for example device is disconnected), the process is not cleaned from CLI's internal cache. This way, in case you try to start the logs again (in the example above - just attach the device again), they are not started as CLI thinks it has a process for this case. So, in case the logcat stream process dies, ensure we clean all the resources related to it. Add unit tests for this case and remove calls to `type`/`cat` from the tests - there's no need to spawn actual child process in the tests. --- lib/common/mobile/android/logcat-helper.ts | 15 ++-- .../mobile/android/logcat-helper.ts | 69 +++++++++++++------ 2 files changed, 57 insertions(+), 27 deletions(-) diff --git a/lib/common/mobile/android/logcat-helper.ts b/lib/common/mobile/android/logcat-helper.ts index 9c7fb9b05c..f95117a47a 100644 --- a/lib/common/mobile/android/logcat-helper.ts +++ b/lib/common/mobile/android/logcat-helper.ts @@ -39,7 +39,8 @@ export class LogcatHelper implements Mobile.ILogcatHelper { logcatStream.on("close", (code: number) => { try { - this.stop(deviceIdentifier); + this.forceStop(deviceIdentifier); + if (code !== 0) { this.$logger.trace("ADB process exited with code " + code.toString()); } @@ -76,13 +77,17 @@ export class LogcatHelper implements Mobile.ILogcatHelper { */ public stop(deviceIdentifier: string): void { if (this.mapDevicesLoggingData[deviceIdentifier] && !this.mapDevicesLoggingData[deviceIdentifier].keepSingleProcess) { - this.mapDevicesLoggingData[deviceIdentifier].loggingProcess.removeAllListeners(); - this.mapDevicesLoggingData[deviceIdentifier].loggingProcess.kill("SIGINT"); - this.mapDevicesLoggingData[deviceIdentifier].lineStream.removeAllListeners(); - delete this.mapDevicesLoggingData[deviceIdentifier]; + this.forceStop(deviceIdentifier); } } + private forceStop(deviceIdentifier: string): void { + this.mapDevicesLoggingData[deviceIdentifier].loggingProcess.removeAllListeners(); + this.mapDevicesLoggingData[deviceIdentifier].loggingProcess.kill("SIGINT"); + this.mapDevicesLoggingData[deviceIdentifier].lineStream.removeAllListeners(); + delete this.mapDevicesLoggingData[deviceIdentifier]; + } + private async getLogcatStream(deviceIdentifier: string, pid?: string) { const device = await this.$devicesService.getDevice(deviceIdentifier); const minAndroidWithLogcatPidSupport = "7.0.0"; diff --git a/lib/common/test/unit-tests/mobile/android/logcat-helper.ts b/lib/common/test/unit-tests/mobile/android/logcat-helper.ts index 561a7b1114..01f8e24426 100644 --- a/lib/common/test/unit-tests/mobile/android/logcat-helper.ts +++ b/lib/common/test/unit-tests/mobile/android/logcat-helper.ts @@ -3,34 +3,39 @@ import { Yok } from "../../../../yok"; import { assert } from "chai"; import * as path from "path"; import * as childProcess from "child_process"; +import * as fileSystem from "fs"; +import { EventEmitter } from "events"; +import stream = require("stream"); + +class ChildProcessMockInstance extends EventEmitter { + public stdout: stream.Readable; + public stderr: any; + public processKillCallCount = 0; + + constructor(pathToSample: string) { + super(); + this.stdout = fileSystem.createReadStream(pathToSample); + this.stderr = new EventEmitter(); + } + + public kill(): void { + this.processKillCallCount++; + } +} class ChildProcessStub { public processSpawnCallCount = 0; - public processKillCallCount = 0; public adbProcessArgs: string[] = []; - private isWin = /^win/.test(process.platform); + public lastSpawnedProcess: ChildProcessMockInstance = null; public spawn(command: string, args?: string[], options?: any): childProcess.ChildProcess { this.adbProcessArgs = args; this.processSpawnCallCount++; - let pathToExecutable = ""; - let shell = ""; - if (this.isWin) { - pathToExecutable = "type"; - shell = "cmd"; - } else { - pathToExecutable = "cat"; - } - pathToExecutable = path.join(pathToExecutable); const pathToSample = path.join(__dirname, "valid-sample.txt"); - const spawnedProcess = childProcess.spawn(pathToExecutable, [pathToSample], { shell }); - const spawnedProcessKill = spawnedProcess.kill; - spawnedProcess.kill = (signal: string) => { - this.processKillCallCount++; - spawnedProcessKill.call(spawnedProcessKill, signal); - }; - - return spawnedProcess; + + this.lastSpawnedProcess = new ChildProcessMockInstance(pathToSample); + + return this.lastSpawnedProcess; } } @@ -207,7 +212,7 @@ describe("logcat-helper", () => { await logcatHelper.stop(validIdentifier); await logcatHelper.stop(validIdentifier); - assert.equal(childProcessStub.processKillCallCount, 1); + assert.equal(childProcessStub.lastSpawnedProcess.processKillCallCount, 1); }); it("should not kill the process if started with keepSingleProcess", async () => { @@ -220,7 +225,7 @@ describe("logcat-helper", () => { await logcatHelper.stop(validIdentifier); await logcatHelper.stop(validIdentifier); - assert.equal(childProcessStub.processKillCallCount, 0); + assert.equal(childProcessStub.lastSpawnedProcess.processKillCallCount, 0); }); it("should do nothing if called without start", async () => { @@ -229,7 +234,27 @@ describe("logcat-helper", () => { await logcatHelper.stop(validIdentifier); assert.equal(childProcessStub.processSpawnCallCount, 0); - assert.equal(childProcessStub.processKillCallCount, 0); }); + + for (const keepSingleProcess of [false, true]) { + it(`should clear the device identifier cache when the logcat process is killed manually and keepSingleProcess is ${keepSingleProcess}`, async () => { + const logcatHelper = injector.resolve("logcatHelper"); + + await logcatHelper.start({ + deviceIdentifier: validIdentifier, + keepSingleProcess + }); + + assert.equal(childProcessStub.processSpawnCallCount, 1); + + childProcessStub.lastSpawnedProcess.emit("close"); + + await logcatHelper.start({ + deviceIdentifier: validIdentifier + }); + + assert.equal(childProcessStub.processSpawnCallCount, 2); + }); + } }); });