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

feat($q): report promises with non rejection callback #13662

Closed
wants to merge 2 commits into from
Closed

feat($q): report promises with non rejection callback #13662

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8106ebe
feat($q): report promises with non rejection callback
lgalfaso Jan 1, 2016
ac84e47
refactor(ngResource): handle success and error callbacks using the sa…
lgalfaso Jan 8, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions docs/app/e2e/api-docs/service-pages.scenario.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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/);
});

Expand All @@ -19,4 +19,4 @@ describe("service pages", function() {
expect(element.all(by.css('.input-arguments p em')).first().getText()).toContain('(default: 0)');
});

});
});
2 changes: 1 addition & 1 deletion src/ng/compile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2716,7 +2716,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
childBoundTranscludeFn);
}
linkQueue = null;
});
}).catch(noop);

return function delayedNodeLinkFn(ignoreChildLinkFn, scope, node, rootElement, boundTranscludeFn) {
var childBoundTranscludeFn = boundTranscludeFn;
Expand Down
2 changes: 2 additions & 0 deletions src/ng/interval.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand Down
111 changes: 93 additions & 18 deletions src/ng/q.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder where this method shows up in the docs (considering there are no docs for $qProvider).
I think there should "standalone" docs for $qProvider (even if just containing this method), but it can be tackled in a follow-up PR.

* @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;
}
};
}

/**
Expand All @@ -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
Expand Down Expand Up @@ -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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here too it might be better to not call this is errorOnUnhandledRejections is false.
(And why not simply nextTick(processChecks) ?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for $$q, nextTick will add a $timeout, if queueSize != 0 then we know we still have to wait before doing the checks.

}
}
}

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);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, why errorMessage isn't wrapped by new Error() so that stack trace is available?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amalitsky This code path is only used if the value thrown in a promise callback was not an error. We could construct an error object here but it wouldn't be of much value as its stack would end up here and not in the place from where the error comes.

To be useful, only errors need to be thrown in promise callbacks and not other values. Which is a good idea in general.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mgol, wouldn't path also have a reference to the promise invocation?

}
}
}

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() {
Expand Down
2 changes: 2 additions & 0 deletions src/ng/timeout.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
3 changes: 2 additions & 1 deletion src/ngMock/angular-mocks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});

Expand Down Expand Up @@ -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;
Expand Down
20 changes: 14 additions & 6 deletions src/ngResource/resource.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -701,12 +701,9 @@ angular.module('ngResource', ['ng']).
response.resource = value;

return response;
}, function(response) {
(error || noop)(response);
return $q.reject(response);
});

promise['finally'](function() {
promise = promise['finally'](function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OOC, does this make any difference (in this particular case, where the code inside the finally callback is synchronous) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code inside needs to run once the http request is resolved or canceled.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant (assuming that the finally callback does not return a promise and does not throw any errors), does it make any difference to have:

promise.finally(...)
promise.then(...)

vs

promise = promise.finally(...)
promise.then(...)

But thinking about it again, the former has the issue that is creates a new promise that will remain unhandled if $http rejects (as you explained in another comment).

value.$resolved = true;
if (!isInstanceCall && cancellable) {
value.$cancelRequest = angular.noop;
Expand All @@ -721,21 +718,32 @@ angular.module('ngResource', ['ng']).
(success || noop)(value, response.headers);
return value;
},
responseErrorInterceptor);
responseErrorInterceptor || error ?
function(response) {
(error || noop)(response);
(responseErrorInterceptor || noop)(response);
return response;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this changing the current behavior ?
I think we should either return the return value of responseErrorInterceptor or $q.reject(response).

Or maybe something like:

promise = promise.then(
  function(response) {
    var value = responseInterceptor(response);
    (success || noop)(value, response.headers);
    return value;
  }, function(response) {
    (error || noop)(response);
    return $q.reject(response);
  }).catch(responseErrorInterceptor);

(Although this is also subtly different than the current behavior if responseInterceptor or success callback throw an exception...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The proposed alternative has the issue that if responseErrorInterceptor is undefined, then return $q.reject(response); will be unhandled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The alternative would be to change return $q.reject(response); to return responseErrorInterceptor ? $q.reject(response) : response; that I think it is a little bit too much

}
: undefined);

if (!isInstanceCall) {
// we are creating instance / collection
// - set the initial promise
// - return the instance / collection
value.$promise = promise;
value.$resolved = false;
if (cancellable) value.$cancelRequest = timeoutDeferred.resolve;
if (cancellable) value.$cancelRequest = cancelRequest;

return value;
}

// instance call
return promise;

function cancelRequest(value) {
promise.catch(noop);
timeoutDeferred.resolve(value);
}
};


Expand Down
2 changes: 1 addition & 1 deletion test/ng/animateRunnerSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ describe("$$AnimateRunner", function() {
var animationComplete = false;
runner.finally(function() {
animationComplete = true;
});
}).catch(noop);
runner[method]();
$rootScope.$digest();
expect(animationComplete).toBe(true);
Expand Down
6 changes: 3 additions & 3 deletions test/ng/httpSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
Expand Down Expand Up @@ -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();
}

Expand Down Expand Up @@ -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();
Expand Down
Loading