fix($http): correct xhrStatus of different `timeout` values #16262

Narretz · 2017-10-09T09:04:56Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
bugfix

What is the current behavior? (You can also link to an open issue here)
Incorrect xhrStatus

What is the new behavior (if this is a feature change)?
Correct xhrStatus

Does this PR introduce a breaking change?
No

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

Other information:

@gkalpak @frederikprijck this is something I'd like your review / opinion on

While trying to find out if #14021 was (partially) fixed by 5e2bc5b, I noticed there's a problem with the http tests and the actual behavior:

In

angular.js/test/ng/httpSpec.js

Line 1910 in e5c6174

expect(response.xhrStatus).toBe('timeout');

we expect that the xhrStatus is 'timeout', however it actually is 'abort' in real $http usage. This is not caught by our tests because the tests run against the mocked $httpBackend which must manually be kept in sync with the real $http implementation.

What's more, our implementation of abort / timeout doesn't follow the way normal xhr abort / timeout works:

If you use xhr.abort() then the onabort handler is called.
If you set xhr.timeout and the timeout is exceeded then the ontimeout handler is callded.

(In the browser, both events are marked as "canceled" in the devtools, at least in Chrome).

In $http, in both cases you would get xhrStatus 'abort', because we abort the xhrRequest, but
don't distinguish between these cases.

In the current implementation, the "timeout" handler would only be called in the case of a real
network timeout (presumably, because I couldn't find a way to test this).

What's more, if you pass a $timeout as the timeout parameter, it will currently set xhrStatus "abort" if the time runs out (promise resolved). This case can only be fixed by reading the internal $$timeoutId property on the promise returned by $timeout.

This PR fixes the code in both the mock httpBackend and the regular http to follow the same logic as xhr does:

xhr	http option	xhr result	current http xhrStatus	new http xhrStatus
xhr.abort()	resolving promise	onabort callback is called	abort	abort
xhr.timeout	numerical value	ontimeout callback	abort	timeout
----	$timeout	-----------	abort	timeout

Here's a plunker that shows the difference, with a patched angular.js file (to swit

http://plnkr.co/edit/nsviFV7YyF4TiXCyISMN?p=preview

petebacondarwin · 2017-10-09T10:29:13Z

Not sure why a promise passed as timeout parameter would lead to an abort result rather than a timeout result?

Narretz · 2017-10-09T10:54:14Z

When you (manually) resolve the promise, the request is aborted. This is our equivalent to calling xhr.abort(). If we don't do this, then there's no way to distinguish a manual abort from a timeout (which was the point of 5e2bc5b afaik)

The API is not that great in that respect.

gkalpak

I am not sure what the xhrStatus should be in case of a timeout config. The name suggests it is a timeout, but I guess many people use it as a way to abort a request (e.g. as we use it in $resource) in case a new request (with updated params) needs to be sent. In such cases, it is not that the request timed out or took too long or something; it's just that we don't care about the response any more.

One idea might be to report xhrStatus as timeout for numeric timeouts and as abort for deferreds.

💯 for keeping the mocked service behavior consistent with the actual one.

(The commit message could be more descriptive btw 😛)

gkalpak · 2017-10-09T11:15:55Z

src/ng/httpBackend.js

+      // The timeout behavior follows standard XMLHttpRequest logic.
+      // xhr.timeout = ontimeout (numerical timeout or $timeout)
+      // xhr.abort() = onabort (resolving a promise)
+      xhr.onabort = timeout > 0 || (timeout && timeout.$$timeoutId >= 0) ? requestTimeout : requestAborted;


AFAICT, this will call the wrong method if the request is aborted while there is a timeout set (even if the "abortion" was not related to the timeout), right?

I think we should set a flag in requestTimeout() and adjust the xhrStatus argument in requestAborted() based on the value of that flag.

Not sure if I understand you correctly, but as it goes, at the moment you can only have a numerical timeout and the ability to cancel the request before that if you pass a promise into timeout and wire up your own timeout behind that promise (Because you cannot short-circuit a $timeout). And in that case, you cannot get the correct xhrStatus, because the xhrStatus is based on the value that is passed into the timeout, that is correct.
Based on the resolve value of a promise-like, we could set the correct xhrStatus, that is right.

What I mean is that if something other than the timeout (numeric or promise) causes the request to be aborted (e.g. someone calls xhr.abort() manually or the browser aborts it - not sure if browsers do that), then your current implementation will call requestTimeout(), even if the abort event was not related to a timeout.

Oh, I see.
As we don't expose the xhr object (or do we via $xhrFactory?), I don't think it can happen that a user aborts the request.
I think when the browser unloads a page (because of navigation), then onabort gets called, but in most cases it's too late for the code to react.

But I guess it's safer if the default is abort for onabort.

Yeah, with $xhrFactory() the user can get access to the xhr object 😁

gkalpak

A couple of minor comments.
LGTM otherwise 👍

gkalpak · 2018-03-06T11:22:50Z

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(timeout.$$timeoutId >= 0 ? 'timeout' : 'abort');


Why not keep in-line with the actual implementation and use isDefined(timeout.$$timeoutId)? Does it make a difference?

gkalpak · 2018-03-06T11:27:21Z

test/ng/httpSpec.js

-        var onRejected = jasmine.createSpy('onRejected');
+        var onRejected = jasmine.createSpy('onRejected').and.callFake(function(response) {
+          expect(response.xhrStatus).toBe('timeout');
+          callback();


I don't think you need this. You can just assert that onRejected was called.

gkalpak · 2018-03-06T11:28:00Z

test/ng/httpSpec.js

+        var onFulfilled = jasmine.createSpy('onFulfilled');
+        var onRejected = jasmine.createSpy('onRejected').and.callFake(function(response) {
+          expect(response.xhrStatus).toBe('timeout');
+          callback();


I don't think you need this. You can just assert that onRejected was called.

Narretz · 2018-03-06T14:24:24Z

thanks @gkalpak , I've incorporated your suggestions, and also updated the docs a bit - technically unrelated to the changes, but actually just a bit of re-ordering of info.

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.

Narretz added component: $http frequency: low severity: confusing type: bug labels Oct 9, 2017

Narretz added this to the 1.6.x milestone Oct 9, 2017

Narretz requested a review from gkalpak October 9, 2017 09:04

googlebot added the cla: yes label Oct 9, 2017

gkalpak reviewed Oct 9, 2017

View reviewed changes

Narretz force-pushed the fix-http-status branch from 6a02655 to 38334a5 Compare December 5, 2017 15:22

Narretz force-pushed the fix-http-status branch from 38334a5 to c25de7c Compare December 13, 2017 18:50

gkalpak approved these changes Mar 6, 2018

View reviewed changes

Narretz force-pushed the fix-http-status branch from c25de7c to 4d82206 Compare March 6, 2018 13:36

Narretz force-pushed the fix-http-status branch from 9a7ff07 to 4d1ffd1 Compare March 6, 2018 17:51

Narretz added 2 commits March 6, 2018 19:10

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

b198e11

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.

docs($http): move response object documentation into Usage section

2ac56fa

Narretz force-pushed the fix-http-status branch from 4d1ffd1 to 2ac56fa Compare March 6, 2018 18:28

Narretz merged commit 64c23e4 into angular:master Mar 7, 2018

Narretz deleted the fix-http-status branch March 7, 2018 09:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix($http): correct xhrStatus of different `timeout` values #16262

fix($http): correct xhrStatus of different `timeout` values #16262

Narretz commented Oct 9, 2017 •

edited

Loading

petebacondarwin commented Oct 9, 2017

Narretz commented Oct 9, 2017 •

edited

Loading

gkalpak left a comment •

edited

Loading

gkalpak Oct 9, 2017

Narretz Oct 11, 2017

gkalpak Oct 11, 2017

Narretz Oct 12, 2017

gkalpak Oct 12, 2017

gkalpak left a comment

gkalpak Mar 6, 2018

gkalpak Mar 6, 2018

gkalpak Mar 6, 2018

Narretz commented Mar 6, 2018

fix($http): correct xhrStatus of different timeout values #16262

fix($http): correct xhrStatus of different timeout values #16262

Conversation

Narretz commented Oct 9, 2017 • edited Loading

petebacondarwin commented Oct 9, 2017

Narretz commented Oct 9, 2017 • edited Loading

gkalpak left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Narretz commented Mar 6, 2018

fix($http): correct xhrStatus of different `timeout` values #16262

fix($http): correct xhrStatus of different `timeout` values #16262

Narretz commented Oct 9, 2017 •

edited

Loading

Narretz commented Oct 9, 2017 •

edited

Loading

gkalpak left a comment •

edited

Loading