Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

Commit 8f9027e

Browse files
committed
fix($timeout): throw when trying to cancel non-$timeout promise
Previously, calling `$timeout.cancel()` with a promise that was not generated by a call to `$timeout()` would do nothing. This could, for example, happen when calling `.then()`/`.catch()` on the returned promise, which creates a new promise, and passing that to `$timeout.cancel()`. With this commit, `$timeout.cancel()` will throw an error if called with a non-$timeout promise, thus surfacing errors that would otherwise go unnoticed. Fixes #16424 BREAKING CHNAGE: `$timeout.cancel()` will throw an error if called with a promise that was not generated by `$timeout()`. Previously, it would silently do nothing. Before: ```js var promise = $timeout(doSomething, 1000).then(doSomethingElse); $timeout.cancel(promise); // No error; timeout NOT canceled. ``` After: ```js var promise = $timeout(doSomething, 1000).then(doSomethingElse); $timeout.cancel(promise); // Throws error. ``` Correct usage: ```js var promise = $timeout(doSomething, 1000); var newPromise = promise.then(doSomethingElse); $timeout.cancel(promise); // Timeout canceled. ```
1 parent 4f4ad3c commit 8f9027e

File tree

3 files changed

+92
-49
lines changed

3 files changed

+92
-49
lines changed
+25
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
@ngdoc error
2+
@name $timeout:badprom
3+
@fullName Non-$timeout promise
4+
@description
5+
6+
This error occurs when calling {@link ng.$timeout#cancel $timeout.cancel()} with a promise that
7+
was not generated by the {@link ng.$timeout $timeout} service. This can, for example, happen when
8+
calling {@link ng.$q#the-promise-api then()/catch()} on the returned promise, which creates a new
9+
promise, and pass that new promise to {@link ng.$timeout#cancel $timeout.cancel()}.
10+
11+
Example of incorrect usage that leads to this error:
12+
13+
```js
14+
var promise = $timeout(doSomething, 1000).then(doSomethingElse);
15+
$timeout.cancel(promise);
16+
```
17+
18+
To fix the example above, keep a reference to the promise returned by
19+
{@link ng.$timeout $timeout()} and pass that to {@link ng.$timeout#cancel $timeout.cancel()}:
20+
21+
```js
22+
var promise = $timeout(doSomething, 1000);
23+
var newPromise = promise.then(doSomethingElse);
24+
$timeout.cancel(promise);
25+
```

src/ng/timeout.js

+60-48
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
'use strict';
22

3+
var $timeoutMinErr = minErr('$timeout');
4+
35
/** @this */
46
function $TimeoutProvider() {
57
this.$get = ['$rootScope', '$browser', '$q', '$$q', '$exceptionHandler',
@@ -8,35 +10,35 @@ function $TimeoutProvider() {
810
var deferreds = {};
911

1012

11-
/**
12-
* @ngdoc service
13-
* @name $timeout
14-
*
15-
* @description
16-
* AngularJS's wrapper for `window.setTimeout`. The `fn` function is wrapped into a try/catch
17-
* block and delegates any exceptions to
18-
* {@link ng.$exceptionHandler $exceptionHandler} service.
19-
*
20-
* The return value of calling `$timeout` is a promise, which will be resolved when
21-
* the delay has passed and the timeout function, if provided, is executed.
22-
*
23-
* To cancel a timeout request, call `$timeout.cancel(promise)`.
24-
*
25-
* In tests you can use {@link ngMock.$timeout `$timeout.flush()`} to
26-
* synchronously flush the queue of deferred functions.
27-
*
28-
* If you only want a promise that will be resolved after some specified delay
29-
* then you can call `$timeout` without the `fn` function.
30-
*
31-
* @param {function()=} fn A function, whose execution should be delayed.
32-
* @param {number=} [delay=0] Delay in milliseconds.
33-
* @param {boolean=} [invokeApply=true] If set to `false` skips model dirty checking, otherwise
34-
* will invoke `fn` within the {@link ng.$rootScope.Scope#$apply $apply} block.
35-
* @param {...*=} Pass additional parameters to the executed function.
36-
* @returns {Promise} Promise that will be resolved when the timeout is reached. The promise
37-
* will be resolved with the return value of the `fn` function.
38-
*
39-
*/
13+
/**
14+
* @ngdoc service
15+
* @name $timeout
16+
*
17+
* @description
18+
* AngularJS's wrapper for `window.setTimeout`. The `fn` function is wrapped into a try/catch
19+
* block and delegates any exceptions to
20+
* {@link ng.$exceptionHandler $exceptionHandler} service.
21+
*
22+
* The return value of calling `$timeout` is a promise, which will be resolved when
23+
* the delay has passed and the timeout function, if provided, is executed.
24+
*
25+
* To cancel a timeout request, call `$timeout.cancel(promise)`.
26+
*
27+
* In tests you can use {@link ngMock.$timeout `$timeout.flush()`} to
28+
* synchronously flush the queue of deferred functions.
29+
*
30+
* If you only want a promise that will be resolved after some specified delay
31+
* then you can call `$timeout` without the `fn` function.
32+
*
33+
* @param {function()=} fn A function, whose execution should be delayed.
34+
* @param {number=} [delay=0] Delay in milliseconds.
35+
* @param {boolean=} [invokeApply=true] If set to `false` skips model dirty checking, otherwise
36+
* will invoke `fn` within the {@link ng.$rootScope.Scope#$apply $apply} block.
37+
* @param {...*=} Pass additional parameters to the executed function.
38+
* @returns {Promise} Promise that will be resolved when the timeout is reached. The promise
39+
* will be resolved with the return value of the `fn` function.
40+
*
41+
*/
4042
function timeout(fn, delay, invokeApply) {
4143
if (!isFunction(fn)) {
4244
invokeApply = delay;
@@ -70,27 +72,37 @@ function $TimeoutProvider() {
7072
}
7173

7274

73-
/**
74-
* @ngdoc method
75-
* @name $timeout#cancel
76-
*
77-
* @description
78-
* Cancels a task associated with the `promise`. As a result of this, the promise will be
79-
* resolved with a rejection.
80-
*
81-
* @param {Promise=} promise Promise returned by the `$timeout` function.
82-
* @returns {boolean} Returns `true` if the task hasn't executed yet and was successfully
83-
* canceled.
84-
*/
75+
/**
76+
* @ngdoc method
77+
* @name $timeout#cancel
78+
*
79+
* @description
80+
* Cancels a task associated with the `promise`. As a result of this, the promise will be
81+
* resolved with a rejection.
82+
*
83+
* @param {Promise=} promise Promise returned by the `$timeout` function.
84+
* @returns {boolean} Returns `true` if the task hasn't executed yet and was successfully
85+
* canceled.
86+
*/
8587
timeout.cancel = function(promise) {
86-
if (promise && promise.$$timeoutId in deferreds) {
87-
// Timeout cancels should not report an unhandled promise.
88-
markQExceptionHandled(deferreds[promise.$$timeoutId].promise);
89-
deferreds[promise.$$timeoutId].reject('canceled');
90-
delete deferreds[promise.$$timeoutId];
91-
return $browser.defer.cancel(promise.$$timeoutId);
88+
if (!promise) return false;
89+
90+
if (!promise.hasOwnProperty('$$timeoutId')) {
91+
throw $timeoutMinErr('badprom',
92+
'`$timeout.cancel()` called with a promise that was not generated by `$timeout()`.');
9293
}
93-
return false;
94+
95+
if (!deferreds.hasOwnProperty(promise.$$timeoutId)) return false;
96+
97+
var id = promise.$$timeoutId;
98+
var deferred = deferreds[id];
99+
100+
// Timeout cancels should not report an unhandled promise.
101+
markQExceptionHandled(deferred.promise);
102+
deferred.reject('canceled');
103+
delete deferreds[id];
104+
105+
return $browser.defer.cancel(id);
94106
};
95107

96108
return timeout;

test/ng/timeoutSpec.js

+7-1
Original file line numberDiff line numberDiff line change
@@ -280,11 +280,17 @@ describe('$timeout', function() {
280280
}));
281281

282282

283-
it('should not throw a runtime exception when given an undefined promise', inject(function($timeout) {
283+
it('should not throw an error when given an undefined promise', inject(function($timeout) {
284284
expect($timeout.cancel()).toBe(false);
285285
}));
286286

287287

288+
it('should throw an error when given a non-$timeout promise', inject(function($timeout) {
289+
const promise = $timeout(noop).then(noop);
290+
expect(function() { $timeout.cancel(promise); }).toThrowMinErr('$timeout', 'badprom');
291+
}));
292+
293+
288294
it('should forget references to relevant deferred', inject(function($timeout, $browser) {
289295
// $browser.defer.cancel is only called on cancel if the deferred object is still referenced
290296
var cancelSpy = spyOn($browser.defer, 'cancel').and.callThrough();

0 commit comments

Comments
 (0)