From 8106ebe71d1b91dcda2dd433edec6f2a3456f720 Mon Sep 17 00:00:00 2001 From: Lucas Mirelmann Date: Fri, 1 Jan 2016 23:40:00 +0100 Subject: [PATCH] feat($q): report promises with non rejection callback Rejected promises that do not have a callback to handle the rejection report this to $exceptionHandler so they can be logged to the console. BREAKING CHANGE Unhandled rejected promises will be logged to $exceptionHandler. Tests that depend on specific order or number of messages in $exceptionHandler will need to handle rejected promises report. Closes: #13653 Closes: #7992 --- .../e2e/api-docs/service-pages.scenario.js | 4 +- src/ng/compile.js | 2 +- src/ng/interval.js | 2 + src/ng/q.js | 111 +++++++++++++++--- src/ng/timeout.js | 2 + src/ngMock/angular-mocks.js | 3 +- src/ngResource/resource.js | 3 +- test/ng/animateRunnerSpec.js | 2 +- test/ng/httpSpec.js | 6 +- test/ng/qSpec.js | 80 +++++++++++-- test/ng/timeoutSpec.js | 4 +- test/ngAnimate/animateCssSpec.js | 1 + test/ngResource/resourceSpec.js | 17 ++- 13 files changed, 192 insertions(+), 45 deletions(-) diff --git a/docs/app/e2e/api-docs/service-pages.scenario.js b/docs/app/e2e/api-docs/service-pages.scenario.js index 94d1ac3065c1..b63c7320e5c8 100644 --- a/docs/app/e2e/api-docs/service-pages.scenario.js +++ b/docs/app/e2e/api-docs/service-pages.scenario.js @@ -10,7 +10,7 @@ describe("service pages", function() { browser.get('build/docs/index.html#!/api/ng/service/$q'); providerLink = element.all(by.css('ol.api-profile-header-structure li a')).first(); - expect(providerLink.getText()).not.toEqual('- $qProvider'); + expect(providerLink.getText()).not.toEqual('- $compileProvider'); expect(providerLink.getAttribute('href')).not.toMatch(/api\/ng\/provider\/\$compileProvider/); }); @@ -19,4 +19,4 @@ describe("service pages", function() { expect(element.all(by.css('.input-arguments p em')).first().getText()).toContain('(default: 0)'); }); -}); \ No newline at end of file +}); diff --git a/src/ng/compile.js b/src/ng/compile.js index 704dbda32b98..3edf7093cd50 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -2716,7 +2716,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { childBoundTranscludeFn); } linkQueue = null; - }); + }).catch(noop); return function delayedNodeLinkFn(ignoreChildLinkFn, scope, node, rootElement, boundTranscludeFn) { var childBoundTranscludeFn = boundTranscludeFn; diff --git a/src/ng/interval.js b/src/ng/interval.js index 06a219767d9a..36b655e2ce65 100644 --- a/src/ng/interval.js +++ b/src/ng/interval.js @@ -188,6 +188,8 @@ function $IntervalProvider() { */ interval.cancel = function(promise) { if (promise && promise.$$intervalId in intervals) { + // Interval cancels should not report as unhandled promise. + intervals[promise.$$intervalId].promise.catch(noop); intervals[promise.$$intervalId].reject('canceled'); $window.clearInterval(promise.$$intervalId); delete intervals[promise.$$intervalId]; diff --git a/src/ng/q.js b/src/ng/q.js index 6107e2873cff..c550410dbc5a 100644 --- a/src/ng/q.js +++ b/src/ng/q.js @@ -216,21 +216,58 @@ * * @returns {Promise} The newly created promise. */ +/** + * @ngdoc provider + * @name $qProvider + * + * @description + */ function $QProvider() { - + var errorOnUnhandledRejections = true; this.$get = ['$rootScope', '$exceptionHandler', function($rootScope, $exceptionHandler) { return qFactory(function(callback) { $rootScope.$evalAsync(callback); - }, $exceptionHandler); + }, $exceptionHandler, errorOnUnhandledRejections); }]; + + /** + * @ngdoc method + * @name $qProvider#errorOnUnhandledRejections + * @kind function + * + * @description + * Retrieves or overrides whether to generate an error when a rejected promise is not handled. + * + * @param {boolean=} value Whether to generate an error when a rejected promise is not handled. + * @returns {boolean|ng.$qProvider} Current value when called without a new value or self for + * chaining otherwise. + */ + this.errorOnUnhandledRejections = function(value) { + if (isDefined(value)) { + errorOnUnhandledRejections = value; + return this; + } else { + return errorOnUnhandledRejections; + } + }; } function $$QProvider() { + var errorOnUnhandledRejections = true; this.$get = ['$browser', '$exceptionHandler', function($browser, $exceptionHandler) { return qFactory(function(callback) { $browser.defer(callback); - }, $exceptionHandler); + }, $exceptionHandler, errorOnUnhandledRejections); }]; + + this.errorOnUnhandledRejections = function(value) { + if (isDefined(value)) { + errorOnUnhandledRejections = value; + return this; + } else { + return errorOnUnhandledRejections; + } + }; } /** @@ -239,10 +276,14 @@ function $$QProvider() { * @param {function(function)} nextTick Function for executing functions in the next turn. * @param {function(...*)} exceptionHandler Function into which unexpected exceptions are passed for * debugging purposes. + @ param {=boolean} errorOnUnhandledRejections Whether an error should be generated on unhandled + * promises rejections. * @returns {object} Promise manager. */ -function qFactory(nextTick, exceptionHandler) { +function qFactory(nextTick, exceptionHandler, errorOnUnhandledRejections) { var $qMinErr = minErr('$q', TypeError); + var queueSize = 0; + var checkQueue = []; /** * @ngdoc method @@ -307,28 +348,62 @@ function qFactory(nextTick, exceptionHandler) { pending = state.pending; state.processScheduled = false; state.pending = undefined; - for (var i = 0, ii = pending.length; i < ii; ++i) { - deferred = pending[i][0]; - fn = pending[i][state.status]; - try { - if (isFunction(fn)) { - deferred.resolve(fn(state.value)); - } else if (state.status === 1) { - deferred.resolve(state.value); - } else { - deferred.reject(state.value); + try { + for (var i = 0, ii = pending.length; i < ii; ++i) { + state.pur = true; + deferred = pending[i][0]; + fn = pending[i][state.status]; + try { + if (isFunction(fn)) { + deferred.resolve(fn(state.value)); + } else if (state.status === 1) { + deferred.resolve(state.value); + } else { + deferred.reject(state.value); + } + } catch (e) { + deferred.reject(e); + exceptionHandler(e); } - } catch (e) { - deferred.reject(e); - exceptionHandler(e); + } + } finally { + --queueSize; + if (errorOnUnhandledRejections && queueSize === 0) { + nextTick(processChecksFn()); + } + } + } + + function processChecks() { + while (!queueSize && checkQueue.length) { + var toCheck = checkQueue.shift(); + if (!toCheck.pur) { + toCheck.pur = true; + var errorMessage = 'Possibly unhandled rejection: ' + toDebugString(toCheck.value); + exceptionHandler(errorMessage); } } } + function processChecksFn() { + return function() { processChecks(); }; + } + + function processQueueFn(state) { + return function() { processQueue(state); }; + } + function scheduleProcessQueue(state) { + if (errorOnUnhandledRejections && !state.pending && state.status === 2 && !state.pur) { + if (queueSize === 0 && checkQueue.length === 0) { + nextTick(processChecksFn()); + } + checkQueue.push(state); + } if (state.processScheduled || !state.pending) return; state.processScheduled = true; - nextTick(function() { processQueue(state); }); + ++queueSize; + nextTick(processQueueFn(state)); } function Deferred() { diff --git a/src/ng/timeout.js b/src/ng/timeout.js index 6253d788e121..b4f320e8142d 100644 --- a/src/ng/timeout.js +++ b/src/ng/timeout.js @@ -85,6 +85,8 @@ function $TimeoutProvider() { */ timeout.cancel = function(promise) { if (promise && promise.$$timeoutId in deferreds) { + // Timeout cancels should not report an unhandled promise. + deferreds[promise.$$timeoutId].promise.catch(noop); deferreds[promise.$$timeoutId].reject('canceled'); delete deferreds[promise.$$timeoutId]; return $browser.defer.cancel(promise.$$timeoutId); diff --git a/src/ngMock/angular-mocks.js b/src/ngMock/angular-mocks.js index 33b59d360361..31d043dc928f 100644 --- a/src/ngMock/angular-mocks.js +++ b/src/ngMock/angular-mocks.js @@ -445,7 +445,7 @@ angular.mock.$IntervalProvider = function() { promise = deferred.promise; count = (angular.isDefined(count)) ? count : 0; - promise.then(null, null, (!hasParams) ? fn : function() { + promise.then(null, function() {}, (!hasParams) ? fn : function() { fn.apply(null, args); }); @@ -505,6 +505,7 @@ angular.mock.$IntervalProvider = function() { }); if (angular.isDefined(fnIndex)) { + repeatFns[fnIndex].deferred.promise.then(undefined, function() {}); repeatFns[fnIndex].deferred.reject('canceled'); repeatFns.splice(fnIndex, 1); return true; diff --git a/src/ngResource/resource.js b/src/ngResource/resource.js index e58b0aecac40..3129efad92c2 100644 --- a/src/ngResource/resource.js +++ b/src/ngResource/resource.js @@ -706,13 +706,14 @@ angular.module('ngResource', ['ng']). return $q.reject(response); }); - promise['finally'](function() { + promise = promise['finally'](function(response) { value.$resolved = true; if (!isInstanceCall && cancellable) { value.$cancelRequest = angular.noop; $timeout.cancel(numericTimeoutPromise); timeoutDeferred = numericTimeoutPromise = httpConfig.timeout = null; } + return response; }); promise = promise.then( diff --git a/test/ng/animateRunnerSpec.js b/test/ng/animateRunnerSpec.js index 622da76ff22b..b62aab1c2d17 100644 --- a/test/ng/animateRunnerSpec.js +++ b/test/ng/animateRunnerSpec.js @@ -206,7 +206,7 @@ describe("$$AnimateRunner", function() { var animationComplete = false; runner.finally(function() { animationComplete = true; - }); + }).then(noop, noop); runner[method](); $rootScope.$digest(); expect(animationComplete).toBe(true); diff --git a/test/ng/httpSpec.js b/test/ng/httpSpec.js index 6ba6d2c75ea3..b2cabf668010 100644 --- a/test/ng/httpSpec.js +++ b/test/ng/httpSpec.js @@ -1031,7 +1031,7 @@ describe('$http', function() { it('should $apply after error callback', function() { $httpBackend.when('GET').respond(404); - $http({method: 'GET', url: '/some'}); + $http({method: 'GET', url: '/some'}).catch(noop); $httpBackend.flush(); expect($rootScope.$apply).toHaveBeenCalledOnce(); }); @@ -1432,7 +1432,7 @@ describe('$http', function() { function doFirstCacheRequest(method, respStatus, headers) { $httpBackend.expect(method || 'GET', '/url').respond(respStatus || 200, 'content', headers); - $http({method: method || 'GET', url: '/url', cache: cache}); + $http({method: method || 'GET', url: '/url', cache: cache}).catch(noop); $httpBackend.flush(); } @@ -1640,7 +1640,7 @@ describe('$http', function() { it('should preserve config object when rejecting from pending cache', function() { $httpBackend.expect('GET', '/url').respond(404, 'content'); - $http({method: 'GET', url: '/url', cache: cache, headers: {foo: 'bar'}}); + $http({method: 'GET', url: '/url', cache: cache, headers: {foo: 'bar'}}).catch(noop); $http({method: 'GET', url: '/url', cache: cache, headers: {foo: 'baz'}}).catch(callback); $httpBackend.flush(); diff --git a/test/ng/qSpec.js b/test/ng/qSpec.js index 74980ce0b5e4..aafbb45f87ab 100644 --- a/test/ng/qSpec.js +++ b/test/ng/qSpec.js @@ -32,7 +32,7 @@ */ describe('q', function() { - var q, defer, deferred, promise, log; + var q, q_no_error, defer, deferred, promise, log, exceptionHandlerCalls; // The following private functions are used to help with logging for testing invocation of the // promise callbacks. @@ -181,12 +181,24 @@ describe('q', function() { }; + function exceptionHandler(reason) { + exceptionHandlerCalls.push(reason); + } + + + function exceptionHandlerStr() { + return exceptionHandlerCalls.join('; '); + } + + beforeEach(function() { - q = qFactory(mockNextTick.nextTick, noop), + q = qFactory(mockNextTick.nextTick, exceptionHandler, true), + q_no_error = qFactory(mockNextTick.nextTick, exceptionHandler, false), defer = q.defer; deferred = defer(); promise = deferred.promise; log = []; + exceptionHandlerCalls = []; mockNextTick.queue = []; }); @@ -1586,6 +1598,8 @@ describe('q', function() { var rejectedPromise = q.reject('rejected'); expect(rejectedPromise['finally']).not.toBeUndefined(); expect(rejectedPromise['catch']).not.toBeUndefined(); + rejectedPromise.catch(noop); + mockNextTick.flush(); }); }); @@ -1995,7 +2009,7 @@ describe('q', function() { it('should log exceptions thrown in a success callback and reject the derived promise', function() { var success1 = success(1, 'oops', true); - promise.then(success1).then(success(2), error(2)); + promise.then(success1).then(success(2), error(2)).catch(noop); syncResolve(deferred, 'done'); expect(logStr()).toBe('success1(done)->throw(oops); error2(oops)->reject(oops)'); expect(mockExceptionLogger.log).toEqual(['oops']); @@ -2003,7 +2017,7 @@ describe('q', function() { it('should NOT log exceptions when a success callback returns rejected promise', function() { - promise.then(success(1, q.reject('rejected'))).then(success(2), error(2)); + promise.then(success(1, q.reject('rejected'))).then(success(2), error(2)).catch(noop); syncResolve(deferred, 'done'); expect(logStr()).toBe('success1(done)->{}; error2(rejected)->reject(rejected)'); expect(mockExceptionLogger.log).toEqual([]); @@ -2012,7 +2026,7 @@ describe('q', function() { it('should log exceptions thrown in a errback and reject the derived promise', function() { var error1 = error(1, 'oops', true); - promise.then(null, error1).then(success(2), error(2)); + promise.then(null, error1).then(success(2), error(2)).catch(noop); syncReject(deferred, 'nope'); expect(logStr()).toBe('error1(nope)->throw(oops); error2(oops)->reject(oops)'); expect(mockExceptionLogger.log).toEqual(['oops']); @@ -2020,7 +2034,7 @@ describe('q', function() { it('should NOT log exceptions when an errback returns a rejected promise', function() { - promise.then(null, error(1, q.reject('rejected'))).then(success(2), error(2)); + promise.then(null, error(1, q.reject('rejected'))).then(success(2), error(2)).catch(noop); syncReject(deferred, 'nope'); expect(logStr()).toBe('error1(nope)->{}; error2(rejected)->reject(rejected)'); expect(mockExceptionLogger.log).toEqual([]); @@ -2029,7 +2043,7 @@ describe('q', function() { it('should log exceptions throw in a progressack and stop propagation, but shoud NOT reject ' + 'the promise', function() { - promise.then(success(), error(), progress(1, 'failed', true)).then(null, error(1), progress(2)); + promise.then(success(), error(), progress(1, 'failed', true)).then(null, error(1), progress(2)).catch(noop); syncNotify(deferred, '10%'); expect(logStr()).toBe('progress1(10%)->throw(failed)'); expect(mockExceptionLogger.log).toEqual(['failed']); @@ -2045,7 +2059,7 @@ describe('q', function() { it('should log exceptions thrown in a success callback and reject the derived promise', function() { var success1 = success(1, 'oops', true); - q.when('hi', success1, error()).then(success(), error(2)); + q.when('hi', success1, error()).then(success(), error(2)).catch(noop); mockNextTick.flush(); expect(logStr()).toBe('success1(hi)->throw(oops); error2(oops)->reject(oops)'); expect(mockExceptionLogger.log).toEqual(['oops']); @@ -2053,7 +2067,7 @@ describe('q', function() { it('should NOT log exceptions when a success callback returns rejected promise', function() { - q.when('hi', success(1, q.reject('rejected'))).then(success(2), error(2)); + q.when('hi', success(1, q.reject('rejected'))).then(success(2), error(2)).catch(noop); mockNextTick.flush(); expect(logStr()).toBe('success1(hi)->{}; error2(rejected)->reject(rejected)'); expect(mockExceptionLogger.log).toEqual([]); @@ -2062,7 +2076,7 @@ describe('q', function() { it('should log exceptions thrown in a errback and reject the derived promise', function() { var error1 = error(1, 'oops', true); - q.when(q.reject('sorry'), success(), error1).then(success(), error(2)); + q.when(q.reject('sorry'), success(), error1).then(success(), error(2)).catch(noop); mockNextTick.flush(); expect(logStr()).toBe('error1(sorry)->throw(oops); error2(oops)->reject(oops)'); expect(mockExceptionLogger.log).toEqual(['oops']); @@ -2071,7 +2085,7 @@ describe('q', function() { it('should NOT log exceptions when an errback returns a rejected promise', function() { q.when(q.reject('sorry'), success(), error(1, q.reject('rejected'))). - then(success(2), error(2)); + then(success(2), error(2)).catch(noop); mockNextTick.flush(); expect(logStr()).toBe('error1(sorry)->{}; error2(rejected)->reject(rejected)'); expect(mockExceptionLogger.log).toEqual([]); @@ -2080,6 +2094,50 @@ describe('q', function() { }); + describe('when exceptionHandler is called', function() { + it('should log an unhandled rejected promise', function() { + var defer = q.defer(); + defer.reject('foo'); + mockNextTick.flush(); + expect(exceptionHandlerStr()).toBe('Possibly unhandled rejection: foo'); + }); + + + it('should not log an unhandled rejected promise if disabled', function() { + var defer = q_no_error.defer(); + defer.reject('foo'); + expect(exceptionHandlerStr()).toBe(''); + }); + + + it('should log a handled rejected promise on a promise without rejection callbacks', function() { + var defer = q.defer(); + defer.promise.then(noop); + defer.reject('foo'); + mockNextTick.flush(); + expect(exceptionHandlerStr()).toBe('Possibly unhandled rejection: foo'); + }); + + + it('should not log a handled rejected promise', function() { + var defer = q.defer(); + defer.promise.catch(noop); + defer.reject('foo'); + mockNextTick.flush(); + expect(exceptionHandlerStr()).toBe(''); + }); + + + it('should not log a handled rejected promise that is handled in a future tick', function() { + var defer = q.defer(); + defer.promise.catch(noop); + defer.resolve(q.reject('foo')); + mockNextTick.flush(); + expect(exceptionHandlerStr()).toBe(''); + }); + }); + + describe('when exceptionHandler rethrows exceptions, ', function() { var originalLogExceptions, deferred, errorSpy, exceptionExceptionSpy; diff --git a/test/ng/timeoutSpec.js b/test/ng/timeoutSpec.js index c6209efd93e1..0ca76b4b3ffb 100644 --- a/test/ng/timeoutSpec.js +++ b/test/ng/timeoutSpec.js @@ -165,7 +165,7 @@ describe('$timeout', function() { expect($exceptionHandler.errors).toEqual([]); $timeout.flush(); - expect($exceptionHandler.errors).toEqual(["Test Error"]); + expect($exceptionHandler.errors).toEqual(["Test Error", 'Possibly unhandled rejection: Test Error']); })); @@ -238,7 +238,7 @@ describe('$timeout', function() { $timeout(task2); promise3 = $timeout(task3, 333); promise4 = $timeout(333); - promise3.then(task4); + promise3.then(task4, noop); $timeout.cancel(promise1); $timeout.cancel(promise3); diff --git a/test/ngAnimate/animateCssSpec.js b/test/ngAnimate/animateCssSpec.js index f6ce20ac5266..8f49b7924eb6 100644 --- a/test/ngAnimate/animateCssSpec.js +++ b/test/ngAnimate/animateCssSpec.js @@ -1329,6 +1329,7 @@ describe("ngAnimate $animateCss", function() { animator.end(); expect(element.data(ANIMATE_TIMER_KEY)).toBeUndefined(); + $timeout.flush(); expect(function() {$timeout.verifyNoPendingTasks();}).not.toThrow(); })); diff --git a/test/ngResource/resourceSpec.js b/test/ngResource/resourceSpec.js index 085a15c990b8..d023aa05a08b 100644 --- a/test/ngResource/resourceSpec.js +++ b/test/ngResource/resourceSpec.js @@ -1052,7 +1052,8 @@ describe("basic usage", function() { it('should call the error callback if provided on non 2xx response', function() { $httpBackend.expect('GET', '/CreditCard/123').respond(ERROR_CODE, ERROR_RESPONSE); - CreditCard.get({id:123}, callback, errorCB); + var ccs = CreditCard.get({id:123}, callback, errorCB); + ccs.$promise.then(noop, noop); $httpBackend.flush(); expect(errorCB).toHaveBeenCalledOnce(); expect(callback).not.toHaveBeenCalled(); @@ -1062,7 +1063,8 @@ describe("basic usage", function() { it('should call the error callback if provided on non 2xx response (without data)', function() { $httpBackend.expect('GET', '/CreditCard').respond(ERROR_CODE, ERROR_RESPONSE); - CreditCard.get(callback, errorCB); + var ccs = CreditCard.get(callback, errorCB); + ccs.$promise.then(noop, noop); $httpBackend.flush(); expect(errorCB).toHaveBeenCalledOnce(); expect(callback).not.toHaveBeenCalled(); @@ -1541,7 +1543,8 @@ describe('cancelling requests', function() { } }); - CreditCard.get(); + var ccs = CreditCard.get(); + ccs.$promise.catch(noop); $timeout.flush(); expect($httpBackend.flush).toThrow(new Error('No pending request to flush !')); @@ -1560,7 +1563,9 @@ describe('cancelling requests', function() { } }); - CreditCard.get().$cancelRequest(); + var ccs = CreditCard.get(); + ccs.$promise.catch(noop); + ccs.$cancelRequest(); expect($httpBackend.flush).toThrow(new Error('No pending request to flush !')); CreditCard.get(); @@ -1578,7 +1583,9 @@ describe('cancelling requests', function() { } }); - CreditCard.get().$cancelRequest(); + var ccs = CreditCard.get(); + ccs.$promise.catch(noop); + ccs.$cancelRequest(); expect($httpBackend.flush).toThrow(new Error('No pending request to flush !')); CreditCard.get();