Skip to content

fix: ensure a single web client connected for debugging #4396

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 26, 2019
Merged

fix: ensure a single web client connected for debugging #4396

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/common/definitions/mobile.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,9 +107,9 @@ declare module Mobile {

interface IiOSDevice extends IDevice {
getDebugSocket(appId: string, projectName: string): Promise<any>;
destroyDebugSocket(appId: string): void;
destroyDebugSocket(appId: string): Promise<void>;
openDeviceLogStream(options?: IiOSLogStreamOptions): Promise<void>;
destroyAllSockets(): void;
destroyAllSockets(): Promise<void>;
}

interface IAndroidDevice extends IDevice {
Expand Down
2 changes: 1 addition & 1 deletion lib/common/mobile/ios/device/ios-application-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ export class IOSApplicationManager extends ApplicationManagerBase {
public async stopApplication(appData: Mobile.IApplicationData): Promise<void> {
const { appId } = appData;

this.device.destroyDebugSocket(appId);
await this.device.destroyDebugSocket(appId);

const action = () => this.$iosDeviceOperations.stop([{ deviceId: this.device.deviceInfo.identifier, ddi: this.$options.ddi, appId }]);

Expand Down
19 changes: 11 additions & 8 deletions lib/common/mobile/ios/ios-device-base.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ export abstract class IOSDeviceBase implements Mobile.IiOSDevice {
this.cachedSockets[appId] = await this.getDebugSocketCore(appId, projectName);

if (this.cachedSockets[appId]) {
this.cachedSockets[appId].on("close", () => {
this.destroyDebugSocket(appId);
this.cachedSockets[appId].on("close", async () => {
await this.destroyDebugSocket(appId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This callback will not be awaited and the lock might be released too early. Maybe just resolve a promise outside of the event handler scope after the callback is called and the result of this.destroyDebugSocket(appId) is resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be an infinite wait if we wait for the close event in the create socket method. We are attaching here in order to clean up the cache when the socket is closed.

});

this.$processService.attachToProcessExitSignals(this, () => this.destroyDebugSocket(appId));
Expand All @@ -51,22 +51,25 @@ export abstract class IOSDeviceBase implements Mobile.IiOSDevice {
return port;
}

public destroyAllSockets() {
public async destroyAllSockets(): Promise<void> {
for (const appId in this.cachedSockets) {
this.destroySocketSafe(this.cachedSockets[appId]);
await this.destroySocketSafe(this.cachedSockets[appId]);
}

this.cachedSockets = {};
}

public destroyDebugSocket(appId: string) {
this.destroySocketSafe(this.cachedSockets[appId]);
public async destroyDebugSocket(appId: string): Promise<void> {
await this.destroySocketSafe(this.cachedSockets[appId]);
this.cachedSockets[appId] = null;
}

private destroySocketSafe(socket: net.Socket) {
private async destroySocketSafe(socket: net.Socket): Promise<void> {
if (socket && !socket.destroyed) {
socket.destroy();
return new Promise<void>((resolve, reject) => {
socket.on("close", resolve);
socket.destroy();
});
}
}

Expand Down
2 changes: 1 addition & 1 deletion lib/common/mobile/ios/simulator/ios-simulator-application-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ export class IOSSimulatorApplicationManager extends ApplicationManagerBase {
public async stopApplication(appData: Mobile.IApplicationData): Promise<void> {
const { appId } = appData;

this.device.destroyDebugSocket(appId);
await this.device.destroyDebugSocket(appId);
await this.detachNativeDebugger(appId);

await this.iosSim.stopApplication(this.device.deviceInfo.identifier, appData.appId, appData.projectName);
Expand Down
4 changes: 2 additions & 2 deletions lib/common/services/lock-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ export class LockService implements ILockService {
}
}

private lock(lockFilePath?: string, lockOpts?: ILockOptions): Promise<string> {
public lock(lockFilePath?: string, lockOpts?: ILockOptions): Promise<string> {
const { filePath, fileOpts } = this.getLockFileSettings(lockFilePath, lockOpts);
this.currentlyLockedFiles.push(filePath);

Expand All @@ -62,7 +62,7 @@ export class LockService implements ILockService {
});
}

private unlock(lockFilePath?: string): void {
public unlock(lockFilePath?: string): void {
const { filePath } = this.getLockFileSettings(lockFilePath);
_.remove(this.currentlyLockedFiles, e => e === lockFilePath);
lockfile.unlockSync(filePath);
Expand Down
7 changes: 7 additions & 0 deletions lib/common/test/unit-tests/stubs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,13 @@ import * as util from "util";
import { EventEmitter } from "events";

export class LockServiceStub implements ILockService {
public async lock(lockFilePath?: string, lockOpts?: ILockOptions): Promise<string> {
return lockFilePath;
}

public unlock(lockFilePath?: string): void {
}

public async executeActionWithLock<T>(action: () => Promise<T>, lockFilePath?: string, lockOpts?: ILockOptions): Promise<T> {
const result = await action();
return result;
Expand Down
14 changes: 14 additions & 0 deletions lib/definitions/lock-service.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,19 @@ declare global {
*/
executeActionWithLock<T>(action: () => Promise<T>, lockFilePath?: string, lockOpts?: ILockOptions): Promise<T>
// TODO: expose as decorator

/**
* Wait until the `unlock` method is called for the specified file
* @param {string} lockFilePath Path to lock file that has to be created. Defaults to `<profile dir>/lockfile.lock`
* @param {ILockOptions} lockOpts Options used for creating the lock file.
* @returns {Promise<T>}
*/
lock(lockFilePath?: string, lockOpts?: ILockOptions): Promise<string>

/**
* Resolve the lock methods for the specified file
* @param {string} lockFilePath Path to lock file that has to be removed. Defaults to `<profile dir>/lockfile.lock`
*/
unlock(lockFilePath?: string): void
}
}
29 changes: 25 additions & 4 deletions lib/device-sockets/ios/app-debug-socket-proxy-factory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ export class AppDebugSocketProxyFactory extends EventEmitter implements IAppDebu

constructor(private $logger: ILogger,
private $errors: IErrors,
private $lockService: ILockService,
private $options: IOptions,
private $net: INet) {
super();
Expand Down Expand Up @@ -54,9 +55,9 @@ export class AppDebugSocketProxyFactory extends EventEmitter implements IAppDebu
}
});

frontendSocket.on("close", () => {
frontendSocket.on("close", async () => {
this.$logger.info("Frontend socket closed");
device.destroyDebugSocket(appId);
await device.destroyDebugSocket(appId);
});

appDebugSocket.on("close", () => {
Expand Down Expand Up @@ -91,6 +92,7 @@ export class AppDebugSocketProxyFactory extends EventEmitter implements IAppDebu
}

private async addWebSocketProxy(device: Mobile.IiOSDevice, appId: string, projectName: string): Promise<ws.Server> {
const clientConnectionLockFile = `debug-connection-${device.deviceInfo.identifier}-${appId}.lock`;
const cacheKey = `${device.deviceInfo.identifier}-${appId}`;
const existingServer = this.deviceWebServers[cacheKey];
if (existingServer) {
Expand All @@ -107,22 +109,37 @@ export class AppDebugSocketProxyFactory extends EventEmitter implements IAppDebu
// We store the socket that connects us to the device in the upgrade request object itself and later on retrieve it
// in the connection callback.

let currentAppSocket: net.Socket = null;
let currentWebSocket: ws = null;
const server = new ws.Server(<any>{
port: localPort,
host: "localhost",
verifyClient: async (info: any, callback: (res: boolean, code?: number, message?: string) => void) => {
await this.$lockService.lock(clientConnectionLockFile);
let acceptHandshake = true;
this.$logger.info("Frontend client connected.");
let appDebugSocket;
try {
if (currentAppSocket) {
currentAppSocket.removeAllListeners();
currentAppSocket = null;
if (currentWebSocket) {
currentWebSocket.removeAllListeners();
currentWebSocket.close();
currentWebSocket = null;
}
await device.destroyDebugSocket(appId);
}
appDebugSocket = await device.getDebugSocket(appId, projectName);
currentAppSocket = appDebugSocket;
this.$logger.info("Backend socket created.");
info.req["__deviceSocket"] = appDebugSocket;
} catch (err) {
err.deviceIdentifier = device.deviceInfo.identifier;
this.$logger.trace(err);
this.emit(CONNECTION_ERROR_EVENT_NAME, err);
acceptHandshake = false;
this.$lockService.unlock(clientConnectionLockFile);
this.$logger.warn(`Cannot connect to device socket. The error message is '${err.message}'.`);
}

Expand All @@ -131,6 +148,7 @@ export class AppDebugSocketProxyFactory extends EventEmitter implements IAppDebu
});
this.deviceWebServers[cacheKey] = server;
server.on("connection", (webSocket, req) => {
currentWebSocket = webSocket;
const encoding = "utf16le";

const appDebugSocket: net.Socket = (<any>req)["__deviceSocket"];
Expand Down Expand Up @@ -163,20 +181,23 @@ export class AppDebugSocketProxyFactory extends EventEmitter implements IAppDebu
});

appDebugSocket.on("close", () => {
currentAppSocket = null;
this.$logger.info("Backend socket closed!");
webSocket.close();
});

webSocket.on("close", () => {
webSocket.on("close", async () => {
currentWebSocket = null;
this.$logger.info('Frontend socket closed!');
appDebugSocket.unpipe(packets);
packets.destroy();
device.destroyDebugSocket(appId);
await device.destroyDebugSocket(appId);
if (!this.$options.watch) {
process.exit(0);
}
});

this.$lockService.unlock(clientConnectionLockFile);
});

return server;
Expand Down
6 changes: 3 additions & 3 deletions lib/services/livesync/ios-device-livesync-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,16 +150,16 @@ export class IOSDeviceLiveSyncService extends DeviceLiveSyncServiceBase implemen
});
} catch (error) {
this.$logger.trace("Error while sending message:", error);
this.destroySocket();
await this.destroySocket();
}
}

private destroySocket(): void {
private async destroySocket(): Promise<void> {
if (this.socket) {
// we do not support LiveSync on multiple apps on the same device
// in order to do that, we should cache the socket per app
// and destroy just the current app socket when possible
this.device.destroyAllSockets();
await this.device.destroyAllSockets();
this.socket = null;
}
}
Expand Down