fix($http): set correct xhrStatus in response when using 'timeout'

This correctly sets "timeout" if a request fails because the timeout (numerical or $timeout) is exceeded, and "abort" if the request is aborted by resolving a promise that was passed in.
radenrap · Mar 7, 2018 · 7e2e235 · 7e2e235
1 parent 4f4ad3c
commit 7e2e235
Show file tree

Hide file tree

Showing 6 changed files with 111 additions and 20 deletions.
diff --git a/src/ng/http.js b/src/ng/http.js
@@ -845,6 +845,11 @@ function $HttpProvider() {
      *      See {@link $http#caching $http Caching} for more information.
      *    - **timeout** – `{number|Promise}` – timeout in milliseconds, or {@link ng.$q promise}
      *      that should abort the request when resolved.
+     *
+     *      A numerical timeout or a promise returned from {@link ng.$timeout $timeout}, will set
+     *      the `xhrStatus` in the {@link $http#$http-returns response} to "timeout", and any other
+     *      resolved promise will set it to "abort", following standard XMLHttpRequest behavior.
+     *
      *    - **withCredentials** - `{boolean}` - whether to set the `withCredentials` flag on the
      *      XHR object. See [requests with credentials](https://developer.mozilla.org/docs/Web/HTTP/Access_control_CORS#Requests_with_credentials)
      *      for more information.

diff --git a/src/ng/httpBackend.js b/src/ng/httpBackend.js
@@ -70,6 +70,7 @@ function createHttpBackend($browser, createXhr, $browserDefer, callbacks, rawDoc
     } else {
 
       var xhr = createXhr(method, url);
+      var abortedByTimeout = false;
 
       xhr.open(method, url, true);
       forEach(headers, function(value, key) {
@@ -110,7 +111,7 @@ function createHttpBackend($browser, createXhr, $browserDefer, callbacks, rawDoc
       };
 
       var requestAborted = function() {
-        completeRequest(callback, -1, null, null, '', 'abort');
+        completeRequest(callback, -1, null, null, '', abortedByTimeout ? 'timeout' : 'abort');
       };
 
       var requestTimeout = function() {
@@ -120,11 +121,11 @@ function createHttpBackend($browser, createXhr, $browserDefer, callbacks, rawDoc
       };
 
       xhr.onerror = requestError;
-      xhr.onabort = requestAborted;
       xhr.ontimeout = requestTimeout;
+      xhr.onabort = requestAborted;
 
       forEach(eventHandlers, function(value, key) {
-          xhr.addEventListener(key, value);
+        xhr.addEventListener(key, value);
       });
 
       forEach(uploadEventHandlers, function(value, key) {
@@ -155,14 +156,26 @@ function createHttpBackend($browser, createXhr, $browserDefer, callbacks, rawDoc
       xhr.send(isUndefined(post) ? null : post);
     }
 
+    // Since we are using xhr.abort() when a request times out, we have to set a flag that
+    // indicates to requestAborted if the request timed out or was aborted.
+    //
+    // http.timeout = numerical timeout   timeout
+    // http.timeout = $timeout            timeout
+    // http.timeout = promise             abort
+    // xhr.abort()                        abort (The xhr object is normally inaccessible, but
+    //                                    can be exposed with the xhrFactory)
     if (timeout > 0) {
-      var timeoutId = $browserDefer(timeoutRequest, timeout);
+      var timeoutId = $browserDefer(function() {
+        timeoutRequest('timeout');
+      }, timeout);
     } else if (isPromiseLike(timeout)) {
-      timeout.then(timeoutRequest);
+      timeout.then(function() {
+        timeoutRequest(isDefined(timeout.$$timeoutId) ? 'timeout' : 'abort');
+      });
     }
 
-
-    function timeoutRequest() {
+    function timeoutRequest(reason) {
+      abortedByTimeout = reason === 'timeout';
       if (jsonpDone) {
         jsonpDone();
       }

diff --git a/src/ngMock/angular-mocks.js b/src/ngMock/angular-mocks.js
@@ -1378,9 +1378,13 @@ function createHttpBackendMock($rootScope, $timeout, $delegate, $browser) {
     function wrapResponse(wrapped) {
       if (!$browser && timeout) {
         if (timeout.then) {
-          timeout.then(handleTimeout);
+          timeout.then(function() {
+            handlePrematureEnd(angular.isDefined(timeout.$$timeoutId) ? 'timeout' : 'abort');
+          });
         } else {
-          $timeout(handleTimeout, timeout);
+          $timeout(function() {
+            handlePrematureEnd('timeout');
+          }, timeout);
         }
       }
 
@@ -1394,11 +1398,11 @@ function createHttpBackendMock($rootScope, $timeout, $delegate, $browser) {
                  copy(response[3] || ''), copy(response[4]));
       }
 
-      function handleTimeout() {
+      function handlePrematureEnd(reason) {
         for (var i = 0, ii = responses.length; i < ii; i++) {
           if (responses[i] === handleResponse) {
             responses.splice(i, 1);
-            callback(-1, undefined, '', undefined, 'timeout');
+            callback(-1, undefined, '', undefined, reason);
             break;
           }
         }
@@ -2110,7 +2114,11 @@ function MockXhr() {
     return lines.join('\n');
   };
 
-  this.abort = angular.noop;
+  this.abort = function() {
+    if (isFunction(this.onabort)) {
+      this.onabort();
+    }
+  };
 
   // This section simulates the events on a real XHR object (and the upload object)
   // When we are testing $httpBackend (inside the AngularJS project) we make partial use of this

diff --git a/test/ng/httpBackendSpec.js b/test/ng/httpBackendSpec.js
@@ -244,7 +244,7 @@ describe('$httpBackend', function() {
     expect(callback).toHaveBeenCalledOnce();
   });
 
-  it('should abort request on timeout', function() {
+  it('should abort request on numerical timeout', function() {
     callback.and.callFake(function(status, response) {
       expect(status).toBe(-1);
     });
@@ -264,9 +264,10 @@ describe('$httpBackend', function() {
   });
 
 
-  it('should abort request on timeout promise resolution', inject(function($timeout) {
-    callback.and.callFake(function(status, response) {
+  it('should abort request on $timeout promise resolution', inject(function($timeout) {
+    callback.and.callFake(function(status, response, headers, statusText, xhrStatus) {
       expect(status).toBe(-1);
+      expect(xhrStatus).toBe('timeout');
     });
 
     $backend('GET', '/url', null, callback, {}, $timeout(noop, 2000));
@@ -300,6 +301,24 @@ describe('$httpBackend', function() {
   }));
 
 
+  it('should abort request on canceler promise resolution', inject(function($q, $browser) {
+    var canceler = $q.defer();
+
+    callback.and.callFake(function(status, response, headers, statusText, xhrStatus) {
+      expect(status).toBe(-1);
+      expect(xhrStatus).toBe('abort');
+    });
+
+    $backend('GET', '/url', null, callback, {}, canceler.promise);
+    xhr = MockXhr.$$lastInstance;
+
+    canceler.resolve();
+    $browser.defer.flush();
+
+    expect(callback).toHaveBeenCalledOnce();
+  }));
+
+
   it('should cancel timeout on completion', function() {
     callback.and.callFake(function(status, response) {
       expect(status).toBe(200);
@@ -320,6 +339,22 @@ describe('$httpBackend', function() {
   });
 
 
+  it('should call callback with xhrStatus "abort" on explicit xhr.abort() when $timeout is set', inject(function($timeout) {
+    callback.and.callFake(function(status, response, headers, statusText, xhrStatus) {
+      expect(status).toBe(-1);
+      expect(xhrStatus).toBe('abort');
+    });
+
+    $backend('GET', '/url', null, callback, {}, $timeout(noop, 2000));
+    xhr = MockXhr.$$lastInstance;
+    spyOn(xhr, 'abort').and.callThrough();
+
+    xhr.abort();
+
+    expect(callback).toHaveBeenCalledOnce();
+  }));
+
+
   it('should set withCredentials', function() {
     $backend('GET', '/some.url', null, callback, {}, null, true);
     expect(MockXhr.$$lastInstance.withCredentials).toBe(true);

diff --git a/test/ng/httpSpec.js b/test/ng/httpSpec.js
@@ -2,6 +2,8 @@
 
 /* global MockXhr: false */
 
+// The http specs run against the mocked httpBackend
+
 describe('$http', function() {
 
   var callback, mockedCookies;
@@ -1907,7 +1909,7 @@ describe('$http', function() {
             function(response) {
               expect(response.data).toBeUndefined();
               expect(response.status).toBe(-1);
-              expect(response.xhrStatus).toBe('timeout');
+              expect(response.xhrStatus).toBe('abort');
               expect(response.headers()).toEqual(Object.create(null));
               expect(response.config.url).toBe('/some');
               callback();
@@ -1923,17 +1925,45 @@ describe('$http', function() {
       }));
 
 
+      it('should timeout request when numerical timeout is exceeded', inject(function($timeout) {
+        var onFulfilled = jasmine.createSpy('onFulfilled');
+        var onRejected = jasmine.createSpy('onRejected').and.callFake(function(response) {
+          expect(response.xhrStatus).toBe('timeout');
+        });
+
+        $httpBackend.expect('GET', '/some').respond(200);
+
+        $http({
+          method: 'GET',
+          url: '/some',
+          timeout: 10
+        }).then(onFulfilled, onRejected);
+
+        $timeout.flush(100);
+
+        expect(onFulfilled).not.toHaveBeenCalled();
+        expect(onRejected).toHaveBeenCalled();
+      }));
+
+
       it('should reject promise when timeout promise resolves', inject(function($timeout) {
         var onFulfilled = jasmine.createSpy('onFulfilled');
-        var onRejected = jasmine.createSpy('onRejected');
+        var onRejected = jasmine.createSpy('onRejected').and.callFake(function(response) {
+          expect(response.xhrStatus).toBe('timeout');
+        });
+
         $httpBackend.expect('GET', '/some').respond(200);
 
-        $http({method: 'GET', url: '/some', timeout: $timeout(noop, 10)}).then(onFulfilled, onRejected);
+        $http({
+          method: 'GET',
+          url: '/some',
+          timeout: $timeout(noop, 10)
+        }).then(onFulfilled, onRejected);
 
         $timeout.flush(100);
 
         expect(onFulfilled).not.toHaveBeenCalled();
-        expect(onRejected).toHaveBeenCalledOnce();
+        expect(onRejected).toHaveBeenCalled();
       }));
     });
 

diff --git a/test/ngMock/angular-mocksSpec.js b/test/ngMock/angular-mocksSpec.js
@@ -1669,7 +1669,7 @@ describe('ngMock', function() {
 
       canceler();  // simulate promise resolution
 
-      expect(callback).toHaveBeenCalledWith(-1, undefined, '', undefined, 'timeout');
+      expect(callback).toHaveBeenCalledWith(-1, undefined, '', undefined, 'abort');
       hb.verifyNoOutstandingExpectation();
       hb.verifyNoOutstandingRequest();
     });