Promise returned from $timeout should not be chained because $$timeoutId is lost #16424

abobwhite · 2018-01-29T20:22:56Z

I'm submitting a ...

bug report
feature request
other

Current behavior:

$timeout returns a promise that has been "decorated" with a $$timeoutId that is then required when attempting to cancel the timeout via $timeout.cancel(promise). When chaining to this promise upon creation (i.e. then, catch, and/or finally), the promise returned from the chained function(s) is a new promise that does not contain $$timeoutId.

Expected / new behavior:

Ideally, Angular.js would not be adding properties willy-nilly to this promise. Secondly, it would be great if the chained calls would return back any extra properties set on the original promise (i.e. $$timeoutId). BUT, it would suffice if the documentation for $timeout could be enhanced to provide a note about this limitation.

Minimal reproduction of the problem with instructions:

Plnkr - Take note that this is built with a chained catch and causes the timeout to not be canceled. If you remove the chained catch or move the catch to a separate line after initial assignment, it cancels just fine.

AngularJS version:
1.6.6

Browser:
All

Anything else:
I strongly would consider this a defect but because I don't think you're going to change your underlying then, catch, and finally implementation to copy any ole' properties from the chained promise, documentation would suffice for any other future souls.

The guilty line of code is here: https://github.com/angular/angular.js/blob/master/src/ng/timeout.js#L66

The text was updated successfully, but these errors were encountered:

gkalpak · 2018-01-30T11:18:33Z

Good catch, @abobwhite! I think this is a reasonable expectation.
(If we could only rely on WeakMaps 😉)

We could either use a HashMap (our internal implementation) to keep the promise<->timeoutId association (and ensure it gets cleaned up when the promise is resolved/rejected) or copy the $$timeoutId (and also $$intervalId) properties over to the new promise here:

angular.js/src/ng/q.js

Lines 320 to 331 in a8830d2

    
           then: function(onFulfilled, onRejected, progressBack) { 
        
             if (isUndefined(onFulfilled) && isUndefined(onRejected) && isUndefined(progressBack)) { 
        
               return this; 
        
             } 
        
             var result = new Promise(); 
        
             this.$$state.pending = this.$$state.pending || []; 
        
             this.$$state.pending.push([result, onFulfilled, onRejected, progressBack]); 
        
             if (this.$$state.status > 0) scheduleProcessQueue(this.$$state); 
        
             return result; 
        
           },

Since it will only affect promises returned by $timeout/$interval and people very rarely chain those, I don't think anyone will notice :)

abobwhite · 2018-01-31T16:30:14Z

Thanks @gkalpak Let me know if I can be of any service.

gkalpak · 2018-02-01T09:45:23Z

@abobwhite, if you fancy taking a stab at it, PRs are always welcome 😉

petebacondarwin · 2018-02-26T19:54:03Z

We are going to throw an error if you pass a promise to $timeout.cancel that doesn't contain a $$timeoutId - i.e. it was not the promise returned from the call to $timeout.

This requires a breaking change, so will go in 1.7.0.

abobwhite · 2018-03-05T18:42:10Z

@petebacondarwin Thanks for the heads up. The proposed solution seems like a good compromise given the EOL timeline for Angular.js and the (likely) pervasiveness of a change to passing along the $$timeoutId to chained promise methods.

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 angular#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. ```

Previously, calling `$interval.cancel()` with a promise that was not generated by a call to `$interval()` 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 `$interval.cancel()`. With this commit, `$interval.cancel()` will throw an error if called with a non-$interval promise, thus surfacing errors that would otherwise go unnoticed. Related to angular#16424. BREAKING CHNAGE: `$interval.cancel()` will throw an error if called with a promise that was not generated by `$interval()`. Previously, it would silently do nothing. Before: ```js var promise = $interval(doSomething, 1000, 5).then(doSomethingElse); $interval.cancel(promise); // No error; interval NOT canceled. ``` After: ```js var promise = $interval(doSomething, 1000, 5).then(doSomethingElse); $interval.cancel(promise); // Throws error. ``` Correct usage: ```js var promise = $interval(doSomething, 1000, 5); var newPromise = promise.then(doSomethingElse); $interval.cancel(promise); // Interval canceled. ```

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 angular#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. ```

Previously, calling `$interval.cancel()` with a promise that was not generated by a call to `$interval()` 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 `$interval.cancel()`. With this commit, `$interval.cancel()` will throw an error if called with a non-$interval promise, thus surfacing errors that would otherwise go unnoticed. Related to angular#16424. BREAKING CHNAGE: `$interval.cancel()` will throw an error if called with a promise that was not generated by `$interval()`. Previously, it would silently do nothing. Before: ```js var promise = $interval(doSomething, 1000, 5).then(doSomethingElse); $interval.cancel(promise); // No error; interval NOT canceled. ``` After: ```js var promise = $interval(doSomething, 1000, 5).then(doSomethingElse); $interval.cancel(promise); // Throws error. ``` Correct usage: ```js var promise = $interval(doSomething, 1000, 5); var newPromise = promise.then(doSomethingElse); $interval.cancel(promise); // Interval canceled. ```

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 angular#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. ```

Previously, calling `$interval.cancel()` with a promise that was not generated by a call to `$interval()` 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 `$interval.cancel()`. With this commit, `$interval.cancel()` will throw an error if called with a non-$interval promise, thus surfacing errors that would otherwise go unnoticed. Related to angular#16424. BREAKING CHNAGE: `$interval.cancel()` will throw an error if called with a promise that was not generated by `$interval()`. Previously, it would silently do nothing. Before: ```js var promise = $interval(doSomething, 1000, 5).then(doSomethingElse); $interval.cancel(promise); // No error; interval NOT canceled. ``` After: ```js var promise = $interval(doSomething, 1000, 5).then(doSomethingElse); $interval.cancel(promise); // Throws error. ``` Correct usage: ```js var promise = $interval(doSomething, 1000, 5); var newPromise = promise.then(doSomethingElse); $interval.cancel(promise); // Interval canceled. ```

Previously, calling `$interval.cancel()` with a promise that was not generated by a call to `$interval()` 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 `$interval.cancel()`. With this commit, `$interval.cancel()` will throw an error if called with a non-$interval promise, thus surfacing errors that would otherwise go unnoticed. Related to #16424. BREAKING CHNAGE: `$interval.cancel()` will throw an error if called with a promise that was not generated by `$interval()`. Previously, it would silently do nothing. Before: ```js var promise = $interval(doSomething, 1000, 5).then(doSomethingElse); $interval.cancel(promise); // No error; interval NOT canceled. ``` After: ```js var promise = $interval(doSomething, 1000, 5).then(doSomethingElse); $interval.cancel(promise); // Throws error. ``` Correct usage: ```js var promise = $interval(doSomething, 1000, 5); var newPromise = promise.then(doSomethingElse); $interval.cancel(promise); // Interval canceled. ``` Closes #16476

gkalpak added type: bug needs: investigation component: misc core frequency: low severity: confusing labels Jan 30, 2018

gkalpak added this to the Backlog milestone Jan 30, 2018

petebacondarwin modified the milestones: Backlog, 1.7.0 Feb 26, 2018

petebacondarwin added the needs: breaking change label Feb 26, 2018

gkalpak self-assigned this Feb 26, 2018

abobwhite changed the title ~~Documentation: Promise returned from $timeout should not be chained~~ Promise returned from $timeout should not be chained because $$timeoutId is lost Mar 5, 2018

gkalpak mentioned this issue Mar 5, 2018

fix($timeout/$interval): throw when trying to cancel non-$timeout/$interval promise #16476

Closed

3 tasks

gkalpak closed this as completed in 3365256 Mar 13, 2018

dependencies bot mentioned this issue May 11, 2018

Update 3 dependencies from npm JetBrains/ring-ui#257

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Promise returned from $timeout should not be chained because $$timeoutId is lost #16424

Promise returned from $timeout should not be chained because $$timeoutId is lost #16424

abobwhite commented Jan 29, 2018 •

edited

Loading

gkalpak commented Jan 30, 2018

abobwhite commented Jan 31, 2018

gkalpak commented Feb 1, 2018

petebacondarwin commented Feb 26, 2018

abobwhite commented Mar 5, 2018

Promise returned from $timeout should not be chained because $$timeoutId is lost #16424

Promise returned from $timeout should not be chained because $$timeoutId is lost #16424

Comments

abobwhite commented Jan 29, 2018 • edited Loading

gkalpak commented Jan 30, 2018

abobwhite commented Jan 31, 2018

gkalpak commented Feb 1, 2018

petebacondarwin commented Feb 26, 2018

abobwhite commented Mar 5, 2018

abobwhite commented Jan 29, 2018 •

edited

Loading