From 8587af1e2484f06e6ca10a4fbd8d37ae0242264b Mon Sep 17 00:00:00 2001 From: Mykola Mokhnach Date: Sat, 19 Oct 2019 11:12:29 +0200 Subject: [PATCH 1/7] fix: Make sure ports are closed properly after the cleanup --- lib/device-connections-factory.js | 50 ++++++++++++++++++------------- lib/driver.js | 8 ++++- 2 files changed, 37 insertions(+), 21 deletions(-) diff --git a/lib/device-connections-factory.js b/lib/device-connections-factory.js index 70e02a692..3e2a887c4 100644 --- a/lib/device-connections-factory.js +++ b/lib/device-connections-factory.js @@ -40,12 +40,22 @@ class iProxy { await status; } - quit () { + async quit () { if (!this.serverSocket) { return; } - this.serverSocket.close(); - this.serverSocket = null; + return await new B((resolve, reject) => { + // eslint-disable-next-line promise/prefer-await-to-callbacks + this.serverSocket.close((err) => { + if (err) { + this.log.error(`Failed to close the connection. Original error: ${err.message}`); + return reject(err); + } + this.log.info('The connection has been closed'); + this.serverSocket = null; + resolve(); + }); + }); } } @@ -70,20 +80,18 @@ class DeviceConnectionsFactory { return `${util.hasValue(udid) ? udid : ''}${SPLITTER}${util.hasValue(port) ? port : ''}`; } - _releaseProxiedConnections (connectionKeys) { - const result = []; - connectionKeys - .filter((k) => _.has(this._connectionsMapping[k], 'iproxy')) - .map((k) => { - log.info(`Releasing the listener for '${k}'`); + async _releaseProxiedConnections (connectionKeys) { + const promises = []; + for (const key of connectionKeys.filter((k) => _.has(this._connectionsMapping[k], 'iproxy'))) { + promises.push((async () => { + log.info(`Releasing the listener for '${key}'`); try { - this._connectionsMapping[k].iproxy.quit(); - } catch (err) { - log.warn(`Cannot release the listener for '${k}': ${err.message}`); - } - result.push(k); - }); - return result; + await this._connectionsMapping[key].iproxy.quit(); + } catch (ign) {} + return key; + })()); + } + return await B.all(promises); } listConnections (udid = null, port = null, strict = false) { @@ -132,7 +140,7 @@ class DeviceConnectionsFactory { log.warn(`Port #${port} is busy`); if (isPortBusy && !_.isEmpty(connectionsOnPort)) { log.info('Trying to release the port'); - for (const key of this._releaseProxiedConnections(connectionsOnPort)) { + for (const key of await this._releaseProxiedConnections(connectionsOnPort)) { delete this._connectionsMapping[key]; } if ((await checkPortStatus(port, '127.0.0.1')) !== 'open') { @@ -155,7 +163,9 @@ class DeviceConnectionsFactory { await iproxy.start(); this._connectionsMapping[currentKey] = {iproxy}; } catch (e) { - iproxy.quit(); + try { + await iproxy.quit(); + } catch (ign) {} throw e; } } else { @@ -164,7 +174,7 @@ class DeviceConnectionsFactory { log.info(`Successfully requested the connection for ${currentKey}`); } - releaseConnection (udid = null, port = null) { + async releaseConnection (udid = null, port = null) { if (!udid && !port) { log.warn('Neither device UDID nor local port is set. ' + 'Did not know how to release the connection'); @@ -178,7 +188,7 @@ class DeviceConnectionsFactory { return; } log.info(`Found cached connections to release: ${JSON.stringify(keys)}`); - this._releaseProxiedConnections(keys); + await this._releaseProxiedConnections(keys); for (const key of keys) { delete this._connectionsMapping[key]; } diff --git a/lib/driver.js b/lib/driver.js index f7fa2d2ba..4a2704a72 100644 --- a/lib/driver.js +++ b/lib/driver.js @@ -608,6 +608,12 @@ class XCUITestDriver extends BaseDriver { async deleteSession () { await removeAllSessionWebSocketHandlers(this.server, this.sessionId); + if (this._recentScreenRecorder) { + await this._recentScreenRecorder.interrupt(true); + await this._recentScreenRecorder.cleanup(); + this._recentScreenRecorder = null; + } + await this.stop(); if (this.opts.clearSystemFiles && this.isAppTemporary) { @@ -682,7 +688,7 @@ class XCUITestDriver extends BaseDriver { } } - DEVICE_CONNECTIONS_FACTORY.releaseConnection(this.opts.udid); + await DEVICE_CONNECTIONS_FACTORY.releaseConnection(this.opts.udid); } async executeCommand (cmd, ...args) { From d7e0e8728771995d96af41a488c45c451475b908 Mon Sep 17 00:00:00 2001 From: Mykola Mokhnach Date: Sat, 19 Oct 2019 17:41:18 +0200 Subject: [PATCH 2/7] update the unit test --- test/unit/device-connections-factory-specs.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/unit/device-connections-factory-specs.js b/test/unit/device-connections-factory-specs.js index 6e80d3b0d..ed83d739a 100644 --- a/test/unit/device-connections-factory-specs.js +++ b/test/unit/device-connections-factory-specs.js @@ -47,15 +47,15 @@ describe('DeviceConnectionsFactory', function () { devConFactory.listConnections(null, 23424).should.eql([]); }); - it('should properly release proxied connections', function () { + it('should properly release proxied connections', async function () { devConFactory._connectionsMapping = { 'udid:1234': {iproxy: {quit: () => {}}}, 'udid:5678': {}, 'udid4:6545': {iproxy: {quit: () => {}}}, }; - devConFactory._releaseProxiedConnections(_.keys(devConFactory._connectionsMapping)) - .should.eql(['udid:1234', 'udid4:6545']); + await devConFactory._releaseProxiedConnections(_.keys(devConFactory._connectionsMapping)) + .should.eventually.eql(['udid:1234', 'udid4:6545']); }); }); From aee8ed207776124acf8f899d90e784f4139cfb9f Mon Sep 17 00:00:00 2001 From: Mykola Mokhnach Date: Sat, 19 Oct 2019 21:23:57 +0200 Subject: [PATCH 3/7] Address comments --- lib/device-connections-factory.js | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/lib/device-connections-factory.js b/lib/device-connections-factory.js index 3e2a887c4..ea2c1bb53 100644 --- a/lib/device-connections-factory.js +++ b/lib/device-connections-factory.js @@ -44,18 +44,14 @@ class iProxy { if (!this.serverSocket) { return; } - return await new B((resolve, reject) => { - // eslint-disable-next-line promise/prefer-await-to-callbacks - this.serverSocket.close((err) => { - if (err) { - this.log.error(`Failed to close the connection. Original error: ${err.message}`); - return reject(err); - } - this.log.info('The connection has been closed'); - this.serverSocket = null; - resolve(); - }); - }); + try { + await B.promisify(this.serverSocket.close, {context: this.serverSocket})(); + this.log.info('The connection has been closed'); + this.serverSocket = null; + } catch (err) { + this.log.error(`Failed to close the connection. Original error: ${err.message}`); + throw err; + } } } From 578eb8d5eba3c54ddcbdb8b6a55bbaf32b7c8690 Mon Sep 17 00:00:00 2001 From: Mykola Mokhnach Date: Sat, 26 Oct 2019 09:00:24 +0200 Subject: [PATCH 4/7] fix server close logic --- lib/device-connections-factory.js | 46 ++++++++++++++++++------------- 1 file changed, 27 insertions(+), 19 deletions(-) diff --git a/lib/device-connections-factory.js b/lib/device-connections-factory.js index ea2c1bb53..6fb4b1c76 100644 --- a/lib/device-connections-factory.js +++ b/lib/device-connections-factory.js @@ -44,14 +44,20 @@ class iProxy { if (!this.serverSocket) { return; } - try { - await B.promisify(this.serverSocket.close, {context: this.serverSocket})(); - this.log.info('The connection has been closed'); - this.serverSocket = null; - } catch (err) { - this.log.error(`Failed to close the connection. Original error: ${err.message}`); - throw err; - } + + this.serverSocket.close(); + return await new B((resolve, reject) => { + this.serverSocket.once('close', () => { + this.log.info('The connection has been closed'); + this.serverSocket = null; + resolve(); + }); + this.serverSocket.once('error', (e) => { + this.log.error(`Failed to close the connection. Original error: ${e.message}`); + this.serverSocket = null; + reject(e); + }); + }); } } @@ -133,17 +139,19 @@ class DeviceConnectionsFactory { if (usePortForwarding) { let isPortBusy = (await checkPortStatus(port, '127.0.0.1')) === 'open'; - log.warn(`Port #${port} is busy`); - if (isPortBusy && !_.isEmpty(connectionsOnPort)) { - log.info('Trying to release the port'); - for (const key of await this._releaseProxiedConnections(connectionsOnPort)) { - delete this._connectionsMapping[key]; - } - if ((await checkPortStatus(port, '127.0.0.1')) !== 'open') { - log.info(`Port #${port} has been successfully released`); - isPortBusy = false; - } else { - log.warn(`Did not know how to release port #${port}`); + if (isPortBusy) { + log.warn(`Port #${port} is busy`); + if (!_.isEmpty(connectionsOnPort)) { + log.info('Trying to release the port'); + for (const key of await this._releaseProxiedConnections(connectionsOnPort)) { + delete this._connectionsMapping[key]; + } + if ((await checkPortStatus(port, '127.0.0.1')) !== 'open') { + log.info(`Port #${port} has been successfully released`); + isPortBusy = false; + } else { + log.warn(`Did not know how to release port #${port}`); + } } } From 4d2cd4c3245d195b96c6051e281657afb2b2f3f0 Mon Sep 17 00:00:00 2001 From: Mykola Mokhnach Date: Sat, 26 Oct 2019 09:03:11 +0200 Subject: [PATCH 5/7] Tune exception printing --- lib/device-connections-factory.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/device-connections-factory.js b/lib/device-connections-factory.js index 6fb4b1c76..8f8af7b6e 100644 --- a/lib/device-connections-factory.js +++ b/lib/device-connections-factory.js @@ -28,7 +28,7 @@ class iProxy { connection.pipe(socket); socket.pipe(connection); } catch (e) { - this.log.warn(e.message); + this.log.warn(e); connection.destroy(); } }); From 2ba188a3d922e3822bce86a6e837ad69ef9fad41 Mon Sep 17 00:00:00 2001 From: Mykola Mokhnach Date: Sat, 26 Oct 2019 13:20:29 +0200 Subject: [PATCH 6/7] Close sockets gracefully --- lib/device-connections-factory.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/device-connections-factory.js b/lib/device-connections-factory.js index 8f8af7b6e..3ee67e4a2 100644 --- a/lib/device-connections-factory.js +++ b/lib/device-connections-factory.js @@ -21,15 +21,15 @@ class iProxy { this.serverSocket = net.createServer(async (connection) => { try { const socket = await utilities.connectPort(this.udid, this.deviceport); - socket.on('close', connection.destroy); + socket.on('close', () => connection.end()); socket.on('error', (e) => this.log.error(e)); - connection.on('close', socket.destroy); + connection.on('close', () => socket.end()); connection.on('error', (e) => this.log.error(e)); connection.pipe(socket); socket.pipe(connection); } catch (e) { this.log.warn(e); - connection.destroy(); + connection.end(); } }); const status = new B((resolve, reject) => { From 0625980c5a506d356c631ad2d8ca39d16021842d Mon Sep 17 00:00:00 2001 From: Mykola Mokhnach Date: Thu, 31 Oct 2019 18:47:28 +0100 Subject: [PATCH 7/7] Do not wait until close event happens --- lib/device-connections-factory.js | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/lib/device-connections-factory.js b/lib/device-connections-factory.js index 3ee67e4a2..ffd0abe3f 100644 --- a/lib/device-connections-factory.js +++ b/lib/device-connections-factory.js @@ -11,7 +11,7 @@ class iProxy { this.deviceport = parseInt(deviceport, 10); this.udid = udid; this.serverSocket = null; - this.log = logger.getLogger(`iProxy@${_.truncate(udid, {length: 8})}`); + this.log = logger.getLogger(`iProxy@${udid.substring(0, 8)}`); } async start () { @@ -40,24 +40,25 @@ class iProxy { await status; } + // eslint-disable-next-line require-await async quit () { if (!this.serverSocket) { return; } - this.serverSocket.close(); - return await new B((resolve, reject) => { - this.serverSocket.once('close', () => { - this.log.info('The connection has been closed'); - this.serverSocket = null; - resolve(); - }); - this.serverSocket.once('error', (e) => { - this.log.error(`Failed to close the connection. Original error: ${e.message}`); - this.serverSocket = null; - reject(e); - }); + // TODO: Wait until the connection is actually closed + // after the corresponding fixes are done in appium-ios-device + // module. Without the fixes the close event might be delayed for up to + // 30 seconds (see https://github.com/appium/appium-xcuitest-driver/pull/1094#issuecomment-546578765) + this.serverSocket.once('close', () => { + this.log.info('The connection has been closed'); + this.serverSocket = null; + }); + this.serverSocket.once('error', (e) => { + this.log.error('Failed to close the connection', e); + this.serverSocket = null; }); + this.serverSocket.close(); } }