Skip to content

Commit 022644c

Browse files
committed
[js] Clean-up semantics around promise cancellation.
1 parent e5d67a7 commit 022644c

File tree

2 files changed

+119
-47
lines changed

2 files changed

+119
-47
lines changed

javascript/node/selenium-webdriver/lib/promise.js

Lines changed: 23 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -962,12 +962,14 @@ const PromiseState = {
962962

963963

964964
/**
965-
* Internal symbol used to store a cancellation handler for
966-
* {@link ManagedPromise} objects. This is an internal implementation detail
967-
* used by the {@link TaskQueue} class to monitor for when a promise is
968-
* cancelled without generating an extra promise via then().
965+
* Internal map used to store cancellation handlers for {@link ManagedPromise}
966+
* objects. This is an internal implementation detail used by the
967+
* {@link TaskQueue} class to monitor for when a promise is cancelled without
968+
* generating an extra promise via then().
969+
*
970+
* @const {!WeakMap<!ManagedPromise, function(!CancellationError)>}
969971
*/
970-
const CANCEL_HANDLER_SYMBOL = Symbol('on cancel');
972+
const ON_CANCEL_HANDLER = new WeakMap;
971973

972974

973975
/**
@@ -978,7 +980,6 @@ const CANCEL_HANDLER_SYMBOL = Symbol('on cancel');
978980
* resolved.
979981
*
980982
* @implements {Thenable<T>}
981-
* @unrestricted
982983
* @template T
983984
* @see http://promises-aplus.github.io/promises-spec/
984985
*/
@@ -1005,7 +1006,7 @@ class ManagedPromise {
10051006
this.stack_ = captureStackTrace('ManagedPromise', 'new', this.constructor);
10061007
}
10071008

1008-
/** @private {ManagedPromise<?>} */
1009+
/** @private {Thenable<?>} */
10091010
this.parent_ = null;
10101011

10111012
/** @private {Array<!Task>} */
@@ -1023,9 +1024,6 @@ class ManagedPromise {
10231024
/** @private {TaskQueue} */
10241025
this.queue_ = null;
10251026

1026-
/** @private {(function(CancellationError)|null)} */
1027-
this[CANCEL_HANDLER_SYMBOL] = null;
1028-
10291027
try {
10301028
var self = this;
10311029
resolver(function(value) {
@@ -1071,6 +1069,7 @@ class ManagedPromise {
10711069
if (Thenable.isImplementation(newValue)) {
10721070
// 2.3.2
10731071
newValue = /** @type {!Thenable} */(newValue);
1072+
this.parent_ = newValue;
10741073
newValue.then(
10751074
this.unblockAndResolve_.bind(this, PromiseState.FULFILLED),
10761075
this.unblockAndResolve_.bind(this, PromiseState.REJECTED));
@@ -1164,7 +1163,7 @@ class ManagedPromise {
11641163
scheduleNotifications_() {
11651164
vlog(2, () => this + ' scheduling notifications', this);
11661165

1167-
this[CANCEL_HANDLER_SYMBOL] = null;
1166+
ON_CANCEL_HANDLER.delete(this);
11681167
if (this.value_ instanceof CancellationError
11691168
&& this.value_.silent_) {
11701169
this.callbacks_ = null;
@@ -1192,9 +1191,10 @@ class ManagedPromise {
11921191
this.parent_.cancel(opt_reason);
11931192
} else {
11941193
var reason = CancellationError.wrap(opt_reason);
1195-
if (this[CANCEL_HANDLER_SYMBOL]) {
1196-
this[CANCEL_HANDLER_SYMBOL](reason);
1197-
this[CANCEL_HANDLER_SYMBOL] = null;
1194+
let onCancel = ON_CANCEL_HANDLER.get(this);
1195+
if (onCancel) {
1196+
onCancel(reason);
1197+
ON_CANCEL_HANDLER.delete(this);
11981198
}
11991199

12001200
if (this.state_ === PromiseState.BLOCKED) {
@@ -1205,6 +1205,9 @@ class ManagedPromise {
12051205
}
12061206

12071207
function canCancel(promise) {
1208+
if (!(promise instanceof ManagedPromise)) {
1209+
return Thenable.isImplementation(promise);
1210+
}
12081211
return promise.state_ === PromiseState.PENDING
12091212
|| promise.state_ === PromiseState.BLOCKED;
12101213
}
@@ -2566,8 +2569,9 @@ class TaskQueue extends events.EventEmitter {
25662569

25672570
this.tasks_.push(task);
25682571
task.queue = this;
2569-
task.promise[CANCEL_HANDLER_SYMBOL] =
2570-
this.onTaskCancelled_.bind(this, task);
2572+
ON_CANCEL_HANDLER.set(
2573+
task.promise,
2574+
(e) => this.onTaskCancelled_(task, e));
25712575

25722576
vlog(1, () => this + '.enqueue(' + task + ')', this);
25732577
vlog(2, () => this.flow_.toString(), this);
@@ -2600,7 +2604,9 @@ class TaskQueue extends events.EventEmitter {
26002604
return;
26012605
}
26022606

2603-
cb.promise[CANCEL_HANDLER_SYMBOL] = this.onTaskCancelled_.bind(this, cb);
2607+
ON_CANCEL_HANDLER.set(
2608+
cb.promise,
2609+
(e) => this.onTaskCancelled_(cb, e));
26042610

26052611
if (cb.queue === this && this.tasks_.indexOf(cb) !== -1) {
26062612
return;

javascript/node/selenium-webdriver/test/lib/promise_test.js

Lines changed: 96 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -147,16 +147,101 @@ describe('promise', function() {
147147
return res;
148148
});
149149

150-
it('canCancelADeferredFromAChainedPromise', function() {
151-
var d = new promise.Deferred();
152-
var p = d.promise.then(assert.fail, function(e) {
153-
assert.ok(e instanceof promise.CancellationError);
154-
assert.equal('because i said so', e.message);
150+
describe('can cancel original promise from its child;', function() {
151+
it('child created by then()', function() {
152+
var d = new promise.Deferred();
153+
var p = d.promise.then(assert.fail, function(e) {
154+
assert.ok(e instanceof promise.CancellationError);
155+
assert.equal('because i said so', e.message);
156+
return 123;
157+
});
158+
159+
p.cancel('because i said so');
160+
return p.then(v => assert.equal(123, v));
161+
});
162+
163+
it('child linked by resolving with parent', function() {
164+
let parent = promise.defer();
165+
let child = new promise.Promise(resolve => resolve(parent.promise));
166+
child.cancel('all done');
167+
168+
return parent.promise.then(
169+
() => assert.fail('expected a cancellation'),
170+
e => {
171+
assert.ok(e instanceof promise.CancellationError);
172+
assert.equal('all done', e.message);
173+
});
174+
});
175+
176+
it('grand child through thenable chain', function() {
177+
let p = new promise.Promise(function() {/* never resolve*/});
178+
179+
let noop = function() {};
180+
let gc = p.then(noop).then(noop).then(noop);
181+
gc.cancel('stop!');
182+
183+
return p.then(
184+
() => assert.fail('expected to be cancelled'),
185+
(e) => {
186+
assert.ok(e instanceof promise.CancellationError);
187+
assert.equal('stop!', e.message);
188+
});
189+
});
190+
191+
it('grand child through thenable chain started at resolve', function() {
192+
function noop() {}
193+
194+
let parent = promise.defer();
195+
let child = new promise.Promise(resolve => resolve(parent.promise));
196+
let grandChild = child.then(noop).then(noop).then(noop);
197+
grandChild.cancel('all done');
198+
199+
return parent.promise.then(
200+
() => assert.fail('expected a cancellation'),
201+
e => {
202+
assert.ok(e instanceof promise.CancellationError);
203+
assert.equal('all done', e.message);
204+
});
155205
});
156-
var p2 = p.then(function() {}, assert.fail);
157206

158-
p.cancel('because i said so');
159-
return p2;
207+
it('"parent" is a Thenable', function() {
208+
function noop() {}
209+
210+
class FakeThenable {
211+
constructor(p) {
212+
this.promise = p;
213+
}
214+
215+
cancel(reason) {
216+
this.promise.cancel(reason);
217+
}
218+
219+
then(cb, eb) {
220+
let result = this.promise.then(cb, eb);
221+
return new FakeThenable(result);
222+
}
223+
}
224+
promise.Thenable.addImplementation(FakeThenable);
225+
226+
let root = new promise.Promise(noop);
227+
let thenable = new FakeThenable(root);
228+
assert.ok(promise.Thenable.isImplementation(thenable));
229+
230+
let child = new promise.Promise(resolve => resolve(thenable));
231+
assert.ok(child instanceof promise.Promise);
232+
child.cancel('stop!');
233+
234+
function assertStopped(p) {
235+
return p.then(
236+
() => assert.fail('not stopped!'),
237+
(e) => {
238+
assert.ok(e instanceof promise.CancellationError);
239+
assert.equal('stop!', e.message);
240+
});
241+
}
242+
243+
return assertStopped(child).then(() => assertStopped(root));
244+
});
160245
});
161246

162247
it('canCancelATimeout', function() {
@@ -167,6 +252,9 @@ describe('promise', function() {
167252
return p;
168253
});
169254

255+
it('can cancel timeout from grandchild', function() {
256+
});
257+
170258
it('cancelIsANoopOnceAPromiseHasBeenFulfilled', function() {
171259
var p = promise.fulfilled(123);
172260
p.cancel();
@@ -214,28 +302,6 @@ describe('promise', function() {
214302
d.fulfill('hi');
215303
return result.then(callback.assertCalled);
216304
});
217-
218-
it('canCancelReturnedPromise', function() {
219-
var callbacks = callbackPair(null, function(e) {
220-
assert.ok(e instanceof promise.CancellationError);
221-
assert.equal('just because', e.message);
222-
});
223-
224-
var promiseLike = {
225-
then: function(cb, eb) {
226-
this.callback = cb;
227-
this.errback = eb;
228-
}
229-
};
230-
231-
var aPromise = promise.when(promiseLike,
232-
callbacks.callback, callbacks.errback);
233-
234-
assert.ok(aPromise.isPending());
235-
aPromise.cancel('just because');
236-
237-
return aPromise.finally(callbacks.assertErrback);
238-
});
239305
});
240306

241307
it('firesUncaughtExceptionEventIfRejectionNeverHandled', function() {

0 commit comments

Comments
 (0)