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

Commit

Permalink
fix($timeout): throw when trying to cancel non-$timeout promise
Browse files Browse the repository at this point in the history
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.
```
  • Loading branch information
gkalpak committed Mar 13, 2018
1 parent de94dd4 commit 3365256
Show file tree
Hide file tree
Showing 3 changed files with 51 additions and 8 deletions.
25 changes: 25 additions & 0 deletions docs/content/error/$timeout/badprom.ngdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
@ngdoc error
@name $timeout:badprom
@fullName Non-$timeout promise
@description

This error occurs when calling {@link ng.$timeout#cancel $timeout.cancel()} with a promise that
was not generated by the {@link ng.$timeout $timeout} service. This can, for example, happen when
calling {@link ng.$q#the-promise-api then()/catch()} on the returned promise, which creates a new
promise, and pass that new promise to {@link ng.$timeout#cancel $timeout.cancel()}.

Example of incorrect usage that leads to this error:

```js
var promise = $timeout(doSomething, 1000).then(doSomethingElse);
$timeout.cancel(promise);
```

To fix the example above, keep a reference to the promise returned by
{@link ng.$timeout $timeout()} and pass that to {@link ng.$timeout#cancel $timeout.cancel()}:

```js
var promise = $timeout(doSomething, 1000);
var newPromise = promise.then(doSomethingElse);
$timeout.cancel(promise);
```
26 changes: 19 additions & 7 deletions src/ng/timeout.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
'use strict';

var $timeoutMinErr = minErr('$timeout');

/** @this */
function $TimeoutProvider() {
this.$get = ['$rootScope', '$browser', '$q', '$$q', '$exceptionHandler',
Expand Down Expand Up @@ -83,14 +85,24 @@ function $TimeoutProvider() {
* canceled.
*/
timeout.cancel = function(promise) {
if (promise && promise.$$timeoutId in deferreds) {
// Timeout cancels should not report an unhandled promise.
markQExceptionHandled(deferreds[promise.$$timeoutId].promise);
deferreds[promise.$$timeoutId].reject('canceled');
delete deferreds[promise.$$timeoutId];
return $browser.defer.cancel(promise.$$timeoutId);
if (!promise) return false;

if (!promise.hasOwnProperty('$$timeoutId')) {
throw $timeoutMinErr('badprom',
'`$timeout.cancel()` called with a promise that was not generated by `$timeout()`.');
}
return false;

if (!deferreds.hasOwnProperty(promise.$$timeoutId)) return false;

var id = promise.$$timeoutId;
var deferred = deferreds[id];

// Timeout cancels should not report an unhandled promise.
markQExceptionHandled(deferred.promise);
deferred.reject('canceled');
delete deferreds[id];

return $browser.defer.cancel(id);
};

return timeout;
Expand Down
8 changes: 7 additions & 1 deletion test/ng/timeoutSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -280,11 +280,17 @@ describe('$timeout', function() {
}));


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


it('should throw an error when given a non-$timeout promise', inject(function($timeout) {
var promise = $timeout(noop).then(noop);
expect(function() { $timeout.cancel(promise); }).toThrowMinErr('$timeout', 'badprom');
}));


it('should forget references to relevant deferred', inject(function($timeout, $browser) {
// $browser.defer.cancel is only called on cancel if the deferred object is still referenced
var cancelSpy = spyOn($browser.defer, 'cancel').and.callThrough();
Expand Down

0 comments on commit 3365256

Please sign in to comment.