refactor: Replace custom commands queue implementation with async lock
MODIFY
lib/basedriver/commands/session.js
lib/basedriver/commands/session.js
@@ -109,6 +109,12 @@ commands.getSession = async function getSession () {
commands.deleteSession = async function deleteSession (/* sessionId */) {
this.clearNewCommandTimeout();
+ if (this.isCommandsQueueEnabled && this.commandsQueueGuard.isBusy()) {
+ // simple hack to release pending commands if they exist
+ for (const key of _.keys(this.commandsQueueGuard.queues)) {
+ this.commandsQueueGuard.queues[key] = [];
+ }
+ }
this.sessionId = null;
};

MODIFY
lib/basedriver/commands/timeout.js
lib/basedriver/commands/timeout.js
import log from '../logger';
import { waitForCondition } from 'asyncbox';
-import B from 'bluebird';
import _ from 'lodash';
import { util } from 'appium-support';
import { errors } from '../../protocol';
@@ -120,7 +119,7 @@ helpers.setNewCommandTimeout = function setNewCommandTimeout (ms) {
helpers.clearNewCommandTimeout = function clearNewCommandTimeout () {
if (this.noCommandTimer) {
- this.noCommandTimer.cancel();
+ clearTimeout(this.noCommandTimer);
this.noCommandTimer = null;
}
};
@@ -132,20 +131,15 @@ helpers.startNewCommandTimeout = function startNewCommandTimeout () {
// if command timeout is 0, it is disabled
if (!this.newCommandTimeoutMs) return; // eslint-disable-line curly
- this.noCommandTimer = util.cancellableDelay(this.newCommandTimeoutMs);
- this.noCommandTimer
- .then(async () => { // eslint-disable-line promise/prefer-await-to-then
+ this.noCommandTimer = setTimeout(async () => {
log.warn(`Shutting down because we waited ` +
- `${this.newCommandTimeoutMs / 1000} seconds for a command`);
- let errorMessage = `New Command Timeout of ` +
- `${this.newCommandTimeoutMs / 1000} seconds ` +
+ `${this.newCommandTimeoutMs / 1000.0} seconds for a command`);
+ const errorMessage = `New Command Timeout of ` +
+ `${this.newCommandTimeoutMs / 1000.0} seconds ` +
`expired. Try customizing the timeout using the ` +
`'newCommandTimeout' desired capability`;
await this.startUnexpectedShutdown(new Error(errorMessage));
- })
- .catch(B.CancellationError, (/*err*/) => {
- // ignore
- });
+ }, this.newCommandTimeoutMs);
};
helpers.implicitWaitForCondition = async function implicitWaitForCondition (condFn) {

MODIFY
lib/basedriver/driver.js
lib/basedriver/driver.js
@@ -11,6 +11,7 @@ import B from 'bluebird';
import _ from 'lodash';
import { util } from 'appium-support';
import { ImageElement, makeImageElementCache, getImgElFromArgs } from './image-element';
+import AsyncLock from 'async-lock';
B.config({
@@ -63,11 +64,10 @@ class BaseDriver extends Protocol {
os.tmpdir();
// base-driver internals
- this.curCommand = B.resolve(); // see note in execute
- this.curCommandCancellable = B.resolve(); // see note in execute
this.shutdownUnexpectedly = false;
this.noCommandTimer = null;
this.shouldValidateCaps = shouldValidateCaps;
+ this.commandsQueueGuard = new AsyncLock();
// settings should be instantiated by drivers which extend BaseDriver, but
// we set it to an empty DeviceSettings instance here to make sure that the
@@ -167,6 +167,7 @@ class BaseDriver extends Protocol {
onCancel(() => reject(new B.CancellationError()));
this.unexpectedShutdownDeferred = {resolve, reject};
});
+ this.unexpectedShutdownDeferred.promise = this.onUnexpectedShutdown;
// noop handler to avoid warning.
this.onUnexpectedShutdown.catch(() => {});
}
@@ -310,6 +311,10 @@ class BaseDriver extends Protocol {
// a new command and so don't want to time out
this.clearNewCommandTimeout();
+ if (this.shutdownUnexpectedly) {
+ throw new errors.NoSuchDriverError('The driver was unexpectedly shut down!');
+ }
+
// If we don't have this command, it must not be implemented
// If the target element is ImageElement, we must try to call `ImageElement.execute` which exist following lines
// since ImageElement supports few commands by itself
@@ -318,63 +323,12 @@ class BaseDriver extends Protocol {
throw new errors.NotYetImplementedError();
}
- let res;
- if (this.isCommandsQueueEnabled && cmd !== 'executeDriverScript') {
- // What we're doing here is pretty clever. this.curCommand is always
- // a promise representing the command currently being executed by the
- // driver, or the last command executed by the driver (it starts off as
- // essentially a pre-resolved promise). When a command comes in, we tack it
- // to the end of this.curCommand, essentially saying we want to execute it
- // whenever this.curCommand is done. We call this new promise nextCommand,
- // and its resolution is what we ultimately will return to whomever called
- // us. Meanwhile, we reset this.curCommand to _be_ nextCommand (but
- // ignoring any rejections), so that if another command comes into the
- // server, it gets tacked on to the end of nextCommand. Thus we create
- // a chain of promises that acts as a queue with single concurrency.
- const nextCommand = this.curCommand.then(() => { // eslint-disable-line promise/prefer-await-to-then
- // if we unexpectedly shut down, we need to reject every command in
- // the queue before we actually try to run it
- if (this.shutdownUnexpectedly) {
- return B.reject(new errors.NoSuchDriverError('The driver was unexpectedly shut down!'));
- }
- // We also need to turn the command into a cancellable promise so if we
- // have an unexpected shutdown event, for example, we can cancel it from
- // outside, rejecting the current command immediately
- let reject;
- this.curCommandCancellable = B.resolve().then(() => { // eslint-disable-line promise/prefer-await-to-then
- // in order to abort the promise, we need to have it in a race
- // with one we can reject from outside
- const cancelPromise = new B(function (_, _reject) { // eslint-disable-line no-unused-vars
- reject = _reject;
- });
-
- // if one of the args is an image element, handle it separately
- return B.race([
- imgElId ? ImageElement.execute(this, cmd, imgElId, ...args) : this[cmd](...args),
- cancelPromise,
- ]);
- });
- // override the B#cancel function, which just turns off listeners
- this.curCommandCancellable.cancel = function cancel (err) {
- if (reject) {
- reject(err);
- }
- };
- return this.curCommandCancellable;
- });
- this.curCommand = nextCommand.catch(() => {});
- res = await nextCommand;
- } else {
- // If we've gotten here because we're running executeDriverScript, we
- // never want to add the command to the queue. This is because it runs
- // other commands _inside_ it, so those commands would never start if we
- // were waiting for executeDriverScript to finish. So it is a special
- // case.
- if (this.shutdownUnexpectedly) {
- throw new errors.NoSuchDriverError('The driver was unexpectedly shut down!');
- }
- res = await this[cmd](...args);
- }
+ const commandExecutor = async () => imgElId
+ ? await ImageElement.execute(this, cmd, imgElId, ...args)
+ : await B.race([this[cmd](...args), this.unexpectedShutdownDeferred.promise]);
+ const res = this.isCommandsQueueEnabled && cmd !== 'executeDriverScript'
+ ? await this.commandsQueueGuard.acquire(BaseDriver.name, commandExecutor)
+ : await commandExecutor();
// if we have set a new command timeout (which is the default), start a
// timer once we've finished executing this command. If we don't clear
@@ -383,7 +337,7 @@ class BaseDriver extends Protocol {
// want to trigger the timer when the user is shutting down the session
// intentionally
if (this.isCommandsQueueEnabled && cmd !== 'deleteSession') {
- // reseting existing timeout
+ // resetting existing timeout
this.startNewCommandTimeout();
}
@@ -402,9 +356,11 @@ class BaseDriver extends Protocol {
async startUnexpectedShutdown (err = new errors.NoSuchDriverError('The driver was unexpectedly shut down!')) {
this.unexpectedShutdownDeferred.reject(err); // allow others to listen for this
this.shutdownUnexpectedly = true;
+ try {
await this.deleteSession(this.sessionId);
+ } finally {
this.shutdownUnexpectedly = false;
- this.curCommandCancellable.cancel(err);
+ }
}
validateLocatorStrategy (strategy, webContext = false) {

MODIFY
test/basedriver/driver-tests.js
test/basedriver/driver-tests.js
@@ -111,12 +111,15 @@ function baseDriverUnitTests (DriverClass, defaultCaps = {}) {
await B.delay(100);
await this.oldDeleteSession();
}.bind(d);
+
let caps = _.clone(defaultCaps);
await d.createSession(caps);
d.startUnexpectedShutdown(new Error('We crashed'));
await d.onUnexpectedShutdown.should.be.rejectedWith(/We crashed/);
+
await d.executeCommand('getSession').should.be.rejectedWith(/shut down/);
- await B.delay(100);
+ await B.delay(500);
+
await d.executeCommand('createSession', caps);
await d.deleteSession();
});

refactor: Replace custom commands queue implementation with async lock (#362)