fix($http): properly increment/decrement $browser.outstandingRequestCount

wbyoko · petebacondarwin · commit 4f6f2bce4ac9 · 2016-07-18T10:09:29.000+01:00
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 Closes #14921 BREAKING CHANGE: HTTP requests now update the outstanding request count synchronously. Previously the request count would not have been updated until the request to the server is actually in flight. Now the request count is updated before the async interceptor is called. The new behaviour is correct but it may change the expected behaviour in a small number of e2e test cases where an async request interceptor is being used.
diff --git 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
@@ -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
@@ -0,0 +1,11 @@
+<!DOCTYPE html>
+<html ng-app="test">
+  <body>
+    <div ng-controller="TestController">
+      <p>{{text}}</p>
+    </div>
+
+    <script src="angular.js"></script>
+    <script src="script.js"></script>
+  </body>
+</html>
diff --git a/test/e2e/fixtures/http/script.js 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
@@ -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
@@ -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();
+      });
+    });
   });
 
 
