From 01a91d88f8d284a971a3b2688c692f4e1445ed38 Mon Sep 17 00:00:00 2001 From: William Bagayoko Date: Wed, 27 Jan 2016 17:44:47 -0600 Subject: [PATCH 1/4] fix($http): properly increment/decrement `$browser.outstandingRequestCount` Calling `$http` will not increment `$browser.outstandingRequestCount` until after all (potentially) asynchronous request interceptors have been processed and will decrement it before any (potentially) asynchronous response interceptors have been processed. This can lead to `$browser.notifyWhenNoOutstandingRequests` firing prematurely, which can be problematic in end-to-end tests. This commit fixes this, by synchronizing the increment/decrement operations with `$http`'s internal interceptor promise chain. Fixes #13782 Closes #13862 --- src/ng/http.js | 17 ++++++++++++++--- src/ng/httpBackend.js | 2 -- test/e2e/fixtures/http/index.html | 11 +++++++++++ test/e2e/fixtures/http/script.js | 28 ++++++++++++++++++++++++++++ test/e2e/tests/httpSpec.js | 9 +++++++++ test/ng/httpSpec.js | 31 +++++++++++++++++++++++++++++++ 6 files changed, 93 insertions(+), 5 deletions(-) create mode 100644 test/e2e/fixtures/http/index.html create mode 100644 test/e2e/fixtures/http/script.js create mode 100644 test/e2e/tests/httpSpec.js diff --git a/src/ng/http.js b/src/ng/http.js index 351591001673..f02165ad6806 100644 --- a/src/ng/http.js +++ b/src/ng/http.js @@ -374,8 +374,8 @@ function $HttpProvider() { **/ var interceptorFactories = this.interceptors = []; - this.$get = ['$httpBackend', '$$cookieReader', '$cacheFactory', '$rootScope', '$q', '$injector', - function($httpBackend, $$cookieReader, $cacheFactory, $rootScope, $q, $injector) { + this.$get = ['$browser', '$httpBackend', '$$cookieReader', '$cacheFactory', '$rootScope', '$q', '$injector', + function($browser, $httpBackend, $$cookieReader, $cacheFactory, $rootScope, $q, $injector) { var defaultCache = $cacheFactory('$http'); @@ -981,7 +981,7 @@ function $HttpProvider() { }; var chain = [serverRequest, undefined]; - var promise = $q.when(config); + var promise = initiateOutstandingRequest(config); // apply interceptors forEach(reversedInterceptors, function(interceptor) { @@ -1000,6 +1000,8 @@ function $HttpProvider() { promise = promise.then(thenFn, rejectFn); } + promise.finally(completeOutstandingRequest); + if (useLegacyPromise) { promise.success = function(fn) { assertArgFn(fn, 'fn'); @@ -1025,6 +1027,15 @@ function $HttpProvider() { return promise; + function initiateOutstandingRequest(config) { + $browser.$$incOutstandingRequestCount(); + return $q.when(config); + } + + function completeOutstandingRequest() { + $browser.$$completeOutstandingRequest(noop); + } + function transformResponse(response) { // make a copy since the response must be cacheable var resp = extend({}, response); diff --git a/src/ng/httpBackend.js b/src/ng/httpBackend.js index d4a28d76e8f6..51c87ab57ed1 100644 --- a/src/ng/httpBackend.js +++ b/src/ng/httpBackend.js @@ -55,7 +55,6 @@ function $HttpBackendProvider() { function createHttpBackend($browser, createXhr, $browserDefer, callbacks, rawDocument) { // TODO(vojta): fix the signature return function(method, url, post, callback, headers, timeout, withCredentials, responseType, eventHandlers, uploadEventHandlers) { - $browser.$$incOutstandingRequestCount(); url = url || $browser.url(); if (lowercase(method) === 'jsonp') { @@ -162,7 +161,6 @@ function createHttpBackend($browser, createXhr, $browserDefer, callbacks, rawDoc jsonpDone = xhr = null; callback(status, response, headersString, statusText); - $browser.$$completeOutstandingRequest(noop); } }; diff --git a/test/e2e/fixtures/http/index.html b/test/e2e/fixtures/http/index.html new file mode 100644 index 000000000000..8a1551be0b19 --- /dev/null +++ b/test/e2e/fixtures/http/index.html @@ -0,0 +1,11 @@ + + + +
+

{{text}}

+
+ + + + + diff --git a/test/e2e/fixtures/http/script.js b/test/e2e/fixtures/http/script.js new file mode 100644 index 000000000000..ebd69e2d1558 --- /dev/null +++ b/test/e2e/fixtures/http/script.js @@ -0,0 +1,28 @@ +angular. + module('test', []). + config(function($httpProvider) { + $httpProvider.interceptors.push(function($q) { + return { + request: function(config) { + return $q(function(resolve) { + setTimeout(resolve, 100, config); + }); + }, + response: function(response) { + return $q(function(resolve) { + setTimeout(resolve, 100, response); + }); + } + }; + }); + }). + controller('TestController', function($cacheFactory, $http, $scope) { + var url = '/some/url'; + + var cache = $cacheFactory('test'); + cache.put(url, 'Hello, world!'); + + $http. + get(url, {cache: cache}). + then(function(response) { $scope.text = response.data; }); + }); diff --git a/test/e2e/tests/httpSpec.js b/test/e2e/tests/httpSpec.js new file mode 100644 index 000000000000..fe8c7d67afd5 --- /dev/null +++ b/test/e2e/tests/httpSpec.js @@ -0,0 +1,9 @@ +describe('$http', function() { + beforeEach(function() { + loadFixture('http'); + }); + + it('should correctly update the outstanding request count', function() { + expect(element(by.binding('text')).getText()).toBe('Hello, world!'); + }); +}); diff --git a/test/ng/httpSpec.js b/test/ng/httpSpec.js index bd44b1cbbc00..041326459afe 100644 --- a/test/ng/httpSpec.js +++ b/test/ng/httpSpec.js @@ -1896,6 +1896,37 @@ describe('$http', function() { expect(paramSerializer({foo: 'foo', bar: ['bar', 'baz']})).toEqual('bar=bar&bar=baz&foo=foo'); }); }); + + describe('$browser\'s outstandingRequestCount', function() { + var incOutstandingRequestCountSpy, completeOutstandingRequestSpy; + + beforeEach(inject(function($browser) { + incOutstandingRequestCountSpy + = spyOn($browser, '$$incOutstandingRequestCount').andCallThrough(); + completeOutstandingRequestSpy + = spyOn($browser, '$$completeOutstandingRequest').andCallThrough(); + })); + + it('should update $browser outstandingRequestCount on success', function() { + $httpBackend.when('GET').respond(200); + + expect(incOutstandingRequestCountSpy).not.toHaveBeenCalled(); + $http.get(''); + expect(incOutstandingRequestCountSpy).toHaveBeenCalledOnce(); + $httpBackend.flush(); + expect(completeOutstandingRequestSpy).toHaveBeenCalledOnce(); + }); + + it('should update $browser outstandingRequestCount on error', function() { + $httpBackend.when('GET').respond(500); + + expect(incOutstandingRequestCountSpy).not.toHaveBeenCalled(); + $http.get(''); + expect(incOutstandingRequestCountSpy).toHaveBeenCalledOnce(); + $httpBackend.flush(); + expect(completeOutstandingRequestSpy).toHaveBeenCalledOnce(); + }); + }); }); From 3c8b3ccbf17b0f1499127cc45cbed496da5fe763 Mon Sep 17 00:00:00 2001 From: Georgios Kalpakas Date: Thu, 14 Jul 2016 20:32:15 +0300 Subject: [PATCH 2/4] refactor($http): move functions together and order alphabetically --- src/ng/http.js | 64 +++++++++++++++++++++++++------------------------- 1 file changed, 32 insertions(+), 32 deletions(-) diff --git a/src/ng/http.js b/src/ng/http.js index f02165ad6806..bcfd163e7e91 100644 --- a/src/ng/http.js +++ b/src/ng/http.js @@ -957,28 +957,7 @@ function $HttpProvider() { config.headers = mergeHeaders(requestConfig); config.method = uppercase(config.method); config.paramSerializer = isString(config.paramSerializer) ? - $injector.get(config.paramSerializer) : config.paramSerializer; - - var serverRequest = function(config) { - var headers = config.headers; - var reqData = transformData(config.data, headersGetter(headers), undefined, config.transformRequest); - - // strip content-type if data is undefined - if (isUndefined(reqData)) { - forEach(headers, function(value, header) { - if (lowercase(header) === 'content-type') { - delete headers[header]; - } - }); - } - - if (isUndefined(config.withCredentials) && !isUndefined(defaults.withCredentials)) { - config.withCredentials = defaults.withCredentials; - } - - // send request - return sendReq(config, reqData).then(transformResponse, transformResponse); - }; + $injector.get(config.paramSerializer) : config.paramSerializer; var chain = [serverRequest, undefined]; var promise = initiateOutstandingRequest(config); @@ -1036,16 +1015,6 @@ function $HttpProvider() { $browser.$$completeOutstandingRequest(noop); } - function transformResponse(response) { - // make a copy since the response must be cacheable - var resp = extend({}, response); - resp.data = transformData(response.data, response.headers, response.status, - config.transformResponse); - return (isSuccess(response.status)) - ? resp - : $q.reject(resp); - } - function executeHeaderFns(headers, config) { var headerContent, processedHeaders = {}; @@ -1087,6 +1056,37 @@ function $HttpProvider() { // execute if header value is a function for merged headers return executeHeaderFns(reqHeaders, shallowCopy(config)); } + + function serverRequest(config) { + var headers = config.headers; + var reqData = transformData(config.data, headersGetter(headers), undefined, config.transformRequest); + + // strip content-type if data is undefined + if (isUndefined(reqData)) { + forEach(headers, function(value, header) { + if (lowercase(header) === 'content-type') { + delete headers[header]; + } + }); + } + + if (isUndefined(config.withCredentials) && !isUndefined(defaults.withCredentials)) { + config.withCredentials = defaults.withCredentials; + } + + // send request + return sendReq(config, reqData).then(transformResponse, transformResponse); + } + + function transformResponse(response) { + // make a copy since the response must be cacheable + var resp = extend({}, response); + resp.data = transformData(response.data, response.headers, response.status, + config.transformResponse); + return (isSuccess(response.status)) + ? resp + : $q.reject(resp); + } } $http.pendingRequests = []; From 142e90ec056a98f93ef67aa6fd2342f6e97f65df Mon Sep 17 00:00:00 2001 From: Georgios Kalpakas Date: Thu, 14 Jul 2016 20:35:24 +0300 Subject: [PATCH 3/4] refactor($http): clean up code --- src/ng/http.js | 36 ++++++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/src/ng/http.js b/src/ng/http.js index bcfd163e7e91..a8ebdb289b8d 100644 --- a/src/ng/http.js +++ b/src/ng/http.js @@ -959,26 +959,25 @@ function $HttpProvider() { config.paramSerializer = isString(config.paramSerializer) ? $injector.get(config.paramSerializer) : config.paramSerializer; - var chain = [serverRequest, undefined]; - var promise = initiateOutstandingRequest(config); + $browser.$$incOutstandingRequestCount(); + + var requestInterceptors = []; + var responseInterceptors = []; + var promise = $q.when(config); // apply interceptors forEach(reversedInterceptors, function(interceptor) { if (interceptor.request || interceptor.requestError) { - chain.unshift(interceptor.request, interceptor.requestError); + requestInterceptors.unshift(interceptor.request, interceptor.requestError); } if (interceptor.response || interceptor.responseError) { - chain.push(interceptor.response, interceptor.responseError); + responseInterceptors.push(interceptor.response, interceptor.responseError); } }); - while (chain.length) { - var thenFn = chain.shift(); - var rejectFn = chain.shift(); - - promise = promise.then(thenFn, rejectFn); - } - + promise = chainInterceptors(promise, requestInterceptors); + promise = promise.then(serverRequest); + promise = chainInterceptors(promise, responseInterceptors); promise.finally(completeOutstandingRequest); if (useLegacyPromise) { @@ -1006,9 +1005,18 @@ function $HttpProvider() { return promise; - function initiateOutstandingRequest(config) { - $browser.$$incOutstandingRequestCount(); - return $q.when(config); + + function chainInterceptors(promise, interceptors) { + for (var i = 0, ii = interceptors.length; i < ii;) { + var thenFn = interceptors[i++]; + var rejectFn = interceptors[i++]; + + promise = promise.then(thenFn, rejectFn); + } + + interceptors.length = 0; + + return promise; } function completeOutstandingRequest() { From a4c0ac181f7ab896c6d015ad1b6c4096d68ce1a1 Mon Sep 17 00:00:00 2001 From: Georgios Kalpakas Date: Thu, 14 Jul 2016 21:59:25 +0300 Subject: [PATCH 4/4] fix($http): avoid `Possibly Unhandled Rejection` error when the request fails Calling `promise.finally(...)` creates a new promise. If we don't return this promise, the user won't be able to attach an error handler to it and thus won't be able to prevent a potential PUR error. This commit also improves the test coverage for the increment/decrement `outstandingRequestCount` fix. Closes #13869 --- src/ng/http.js | 2 +- test/ng/httpSpec.js | 224 +++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 211 insertions(+), 15 deletions(-) diff --git a/src/ng/http.js b/src/ng/http.js index a8ebdb289b8d..6f9d573baf13 100644 --- a/src/ng/http.js +++ b/src/ng/http.js @@ -978,7 +978,7 @@ function $HttpProvider() { promise = chainInterceptors(promise, requestInterceptors); promise = promise.then(serverRequest); promise = chainInterceptors(promise, responseInterceptors); - promise.finally(completeOutstandingRequest); + promise = promise.finally(completeOutstandingRequest); if (useLegacyPromise) { promise.success = function(fn) { diff --git a/test/ng/httpSpec.js b/test/ng/httpSpec.js index 041326459afe..479ce359bc92 100644 --- a/test/ng/httpSpec.js +++ b/test/ng/httpSpec.js @@ -1896,37 +1896,233 @@ describe('$http', function() { expect(paramSerializer({foo: 'foo', bar: ['bar', 'baz']})).toEqual('bar=bar&bar=baz&foo=foo'); }); }); + }); - describe('$browser\'s outstandingRequestCount', function() { - var incOutstandingRequestCountSpy, completeOutstandingRequestSpy; - beforeEach(inject(function($browser) { - incOutstandingRequestCountSpy - = spyOn($browser, '$$incOutstandingRequestCount').andCallThrough(); - completeOutstandingRequestSpy - = spyOn($browser, '$$completeOutstandingRequest').andCallThrough(); - })); + describe('$browser\'s outstandingRequestCount', function() { + var $http; + var $httpBackend; + var $rootScope; + var incOutstandingRequestCountSpy; + var completeOutstandingRequestSpy; - it('should update $browser outstandingRequestCount on success', function() { - $httpBackend.when('GET').respond(200); + describe('without interceptors', function() { + beforeEach(setupServicesAndSpies); + + + it('should immediately call `$browser.$$incOutstandingRequestCount()`', function() { expect(incOutstandingRequestCountSpy).not.toHaveBeenCalled(); $http.get(''); expect(incOutstandingRequestCountSpy).toHaveBeenCalledOnce(); + }); + + + it('should call `$browser.$$completeOutstandingRequest()` on success', function() { + $httpBackend.when('GET').respond(200); + + $http.get(''); + expect(completeOutstandingRequestSpy).not.toHaveBeenCalled(); $httpBackend.flush(); expect(completeOutstandingRequestSpy).toHaveBeenCalledOnce(); }); - it('should update $browser outstandingRequestCount on error', function() { + + it('should call `$browser.$$completeOutstandingRequest()` on error', function() { $httpBackend.when('GET').respond(500); - expect(incOutstandingRequestCountSpy).not.toHaveBeenCalled(); - $http.get(''); - expect(incOutstandingRequestCountSpy).toHaveBeenCalledOnce(); + $http.get('').catch(noop); + expect(completeOutstandingRequestSpy).not.toHaveBeenCalled(); $httpBackend.flush(); expect(completeOutstandingRequestSpy).toHaveBeenCalledOnce(); }); + + + it('should increment/decrement `outstandingRequestCount` on error in `transformRequest`', + inject(function($exceptionHandler) { + expect(incOutstandingRequestCountSpy).not.toHaveBeenCalled(); + expect(completeOutstandingRequestSpy).not.toHaveBeenCalled(); + + $http.get('', {transformRequest: function() { throw new Error(); }}).catch(noop); + + expect(incOutstandingRequestCountSpy).toHaveBeenCalledOnce(); + expect(completeOutstandingRequestSpy).not.toHaveBeenCalled(); + + $rootScope.$digest(); + + expect(incOutstandingRequestCountSpy).toHaveBeenCalledOnce(); + expect(completeOutstandingRequestSpy).toHaveBeenCalledOnce(); + + expect($exceptionHandler.errors).toEqual([jasmine.any(Error)]); + $exceptionHandler.errors = []; + }) + ); + + + it('should increment/decrement `outstandingRequestCount` on error in `transformResponse`', + inject(function($exceptionHandler) { + expect(incOutstandingRequestCountSpy).not.toHaveBeenCalled(); + expect(completeOutstandingRequestSpy).not.toHaveBeenCalled(); + + $httpBackend.when('GET').respond(200); + $http.get('', {transformResponse: function() { throw new Error(); }}).catch(noop); + + expect(incOutstandingRequestCountSpy).toHaveBeenCalledOnce(); + expect(completeOutstandingRequestSpy).not.toHaveBeenCalled(); + + $httpBackend.flush(); + + expect(incOutstandingRequestCountSpy).toHaveBeenCalledOnce(); + expect(completeOutstandingRequestSpy).toHaveBeenCalledOnce(); + + expect($exceptionHandler.errors).toEqual([jasmine.any(Error)]); + $exceptionHandler.errors = []; + }) + ); }); + + + describe('with interceptors', function() { + var reqInterceptorDeferred; + var resInterceptorDeferred; + var reqInterceptorFulfilled; + var resInterceptorFulfilled; + + beforeEach(module(function($httpProvider) { + reqInterceptorDeferred = null; + resInterceptorDeferred = null; + reqInterceptorFulfilled = false; + resInterceptorFulfilled = false; + + $httpProvider.interceptors.push(function($q) { + return { + request: function(config) { + return (reqInterceptorDeferred = $q.defer()). + promise. + finally(function() { reqInterceptorFulfilled = true; }). + then(valueFn(config)); + }, + response: function() { + return (resInterceptorDeferred = $q.defer()). + promise. + finally(function() { resInterceptorFulfilled = true; }); + } + }; + }); + })); + + beforeEach(setupServicesAndSpies); + + beforeEach(function() { + $httpBackend.when('GET').respond(200); + }); + + + it('should increment/decrement `outstandingRequestCount` before/after async interceptors', + function() { + expect(reqInterceptorFulfilled).toBe(false); + expect(resInterceptorFulfilled).toBe(false); + expect(incOutstandingRequestCountSpy).not.toHaveBeenCalled(); + expect(completeOutstandingRequestSpy).not.toHaveBeenCalled(); + + $http.get(''); + $rootScope.$digest(); + + expect(reqInterceptorFulfilled).toBe(false); + expect(resInterceptorFulfilled).toBe(false); + expect(incOutstandingRequestCountSpy).toHaveBeenCalledOnce(); + expect(completeOutstandingRequestSpy).not.toHaveBeenCalled(); + + reqInterceptorDeferred.resolve(); + $httpBackend.flush(); + + expect(reqInterceptorFulfilled).toBe(true); + expect(resInterceptorFulfilled).toBe(false); + expect(incOutstandingRequestCountSpy).toHaveBeenCalledOnce(); + expect(completeOutstandingRequestSpy).not.toHaveBeenCalled(); + + resInterceptorDeferred.resolve(); + $rootScope.$digest(); + + expect(reqInterceptorFulfilled).toBe(true); + expect(resInterceptorFulfilled).toBe(true); + expect(incOutstandingRequestCountSpy).toHaveBeenCalledOnce(); + expect(completeOutstandingRequestSpy).toHaveBeenCalledOnce(); + } + ); + + + it('should increment/decrement `outstandingRequestCount` on error in request interceptor', + function() { + expect(reqInterceptorFulfilled).toBe(false); + expect(incOutstandingRequestCountSpy).not.toHaveBeenCalled(); + expect(completeOutstandingRequestSpy).not.toHaveBeenCalled(); + + $http.get('').catch(noop); + $rootScope.$digest(); + + expect(reqInterceptorFulfilled).toBe(false); + expect(incOutstandingRequestCountSpy).toHaveBeenCalledOnce(); + expect(completeOutstandingRequestSpy).not.toHaveBeenCalled(); + + reqInterceptorDeferred.reject(); + $rootScope.$digest(); + + expect(reqInterceptorFulfilled).toBe(true); + expect(incOutstandingRequestCountSpy).toHaveBeenCalledOnce(); + expect(completeOutstandingRequestSpy).toHaveBeenCalledOnce(); + } + ); + + + it('should increment/decrement `outstandingRequestCount` on error in response interceptor', + function() { + expect(reqInterceptorFulfilled).toBe(false); + expect(resInterceptorFulfilled).toBe(false); + expect(incOutstandingRequestCountSpy).not.toHaveBeenCalled(); + expect(completeOutstandingRequestSpy).not.toHaveBeenCalled(); + + $http.get('').catch(noop); + $rootScope.$digest(); + + expect(reqInterceptorFulfilled).toBe(false); + expect(resInterceptorFulfilled).toBe(false); + expect(incOutstandingRequestCountSpy).toHaveBeenCalledOnce(); + expect(completeOutstandingRequestSpy).not.toHaveBeenCalled(); + + reqInterceptorDeferred.resolve(); + $httpBackend.flush(); + + expect(reqInterceptorFulfilled).toBe(true); + expect(resInterceptorFulfilled).toBe(false); + expect(incOutstandingRequestCountSpy).toHaveBeenCalledOnce(); + expect(completeOutstandingRequestSpy).not.toHaveBeenCalled(); + + resInterceptorDeferred.reject(); + $rootScope.$digest(); + + expect(reqInterceptorFulfilled).toBe(true); + expect(resInterceptorFulfilled).toBe(true); + expect(incOutstandingRequestCountSpy).toHaveBeenCalledOnce(); + expect(completeOutstandingRequestSpy).toHaveBeenCalledOnce(); + } + ); + }); + + + // Helpers + function setupServicesAndSpies() { + inject(function($browser, _$http_, _$httpBackend_, _$rootScope_) { + $http = _$http_; + $httpBackend = _$httpBackend_; + $rootScope = _$rootScope_; + + incOutstandingRequestCountSpy = + spyOn($browser, '$$incOutstandingRequestCount').and.callThrough(); + completeOutstandingRequestSpy = + spyOn($browser, '$$completeOutstandingRequest').and.callThrough(); + }); + } });