feat(http): http requests to implement xhr timeout #36681

daftfox · 2020-04-17T04:10:29Z

Http client allows an optional timeout option to be passed.
An error will be emitted when no response is received within the allotted interval.
This reflects XMLHttpRequest behaviour that is standard in all modern browsers.
https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/timeout

This is behaviour that has been available for 'regular' js xhr requests, but has
not yet been implemented in Angular. Alternatives such as interceptors, RxJS
or custom client implementation don't offer the same behaviour in a neat package.

fixes #34421

PR Checklist

Please check if your PR fulfills the following requirements:

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

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

The HttpClient does not allow developers to pass in a timeout option.

Issue Number: #34421

What is the new behavior?

A timeout option can be passed when creating an Http request. When no response has been received before the time runs out, the observer's error method is called.

Does this PR introduce a breaking change?

Yes
No

Other information

Docs and tests have been added or updated where applicable. In addition, the payload-size limit for the aio main-es2015.js has been increased by 150 bytes to allow for the slight increase in size that happened due to the newly added feature.

daftfox · 2020-04-20T23:27:22Z

Would anyone be able to review this PR?

gkalpak

Thx for updating the docs too 😍
A minor comments - otherwise lgtm ✨

packages/common/http/test/xhr_mock.ts

packages/common/http/src/xhr.ts

aio/content/guide/http.md

aio/content/examples/http/src/app/config/config.service.ts

gkalpak

One suggestion. Otherwise lgtm 👍
BTW, please squash the commit into one (or create fixup commits).

gkalpak · 2020-06-16T17:02:07Z

aio/content/examples/http/src/app/config/config.service.ts

@@ -74,6 +84,10 @@ export class ConfigService {
    if (error.error instanceof ErrorEvent) {
      // A client-side or network error occurred. Handle it accordingly.
      console.error('An error occurred:', error.error.message);
+    } else if (error.status === 0 && error.statusText === 'Request timeout') {


It is no longer certain that error.statusText === 'Request timeout'. How about error.error instanceof ProgressEvent && error.status === 0?

jelbourn

Should there also be documentation on how to fake a request timeout for testing?

jelbourn · 2020-06-17T03:02:30Z

aio/content/guide/http.md


 * The server backend might reject the request, returning an HTTP response with a status code such as 404 or 500. These are error _responses_.

 * Something could go wrong on the client-side such as a network error that prevents the request from completing successfully or an exception thrown in an RxJS operator. These errors produce JavaScript `ErrorEvent` objects.

-`HttpClient` captures both kinds of errors in its `HttpErrorResponse`. You can inspect that response to identify the error's cause.
+* The server took too long to respond, but only when a timeout value has been passed in as an optional parameter with the request object or the `HttpClient`'s request methods.


Perhaps more concise:

Suggested change

* The server took too long to respond, but only when a timeout value has been passed in as an optional parameter with the request object or the `HttpClient`'s request methods.

* The server took too long to respond. This can occur only when the request specifies an explicit `timeout`.

jelbourn · 2020-06-17T03:04:36Z

packages/common/http/src/request.ts

@@ -134,20 +140,23 @@ export class HttpRequest<T> {
    params?: HttpParams,
    responseType?: 'arraybuffer'|'blob'|'json'|'text',
    withCredentials?: boolean,
+    timeout?: number,


@gkalpak perhaps out of scope for this PR, but would it make sense to extract this options object into its own type? The repetition seems unfortunate.

jelbourn · 2020-06-17T03:05:12Z

packages/common/http/src/request.ts

@@ -196,6 +207,10 @@ export class HttpRequest<T> {
        this.headers = options.headers;
      }

+      if (!!options.timeout) {


Using !! is unnecessary in conditionals since it will always evaluate to the same thing as normal truthiness.

jelbourn · 2020-06-17T03:06:31Z

packages/common/http/src/xhr.ts

@@ -93,6 +93,11 @@ export class HttpXhrBackend implements HttpBackend {
        xhr.withCredentials = true;
      }

+      // Set the timeout if set in request object
+      if (req.timeout) {


What happens if xhr.timeout is set to null, undefined, or zero?

jelbourn · 2020-06-17T03:10:45Z

packages/common/http/src/xhr.ts

@@ -234,6 +239,19 @@ export class HttpXhrBackend implements HttpBackend {
        }
      };

+      // The onTimeout callback is called when the request has timed out. This timeout is optional
+      // and can be set by the user.
+      const onTimeout = (error: ProgressEvent) => {


@gkalpak I'm wondering if creating this function for every request will allocate an additional amount of memory that we care about. Would it make a meaningful difference to only define onTimeout when req.timeout is set?

I doubt it will matter in the grand scheme of things, but it definitely wouldn't hurt to only define onTimeout if needed.
Same goes for onDownProgress and onUpProgress.

petebacondarwin · 2021-01-28T13:02:07Z

@daftfox - sorry this PR has been around for a while. Could you respond to @jelbourn's questions and rebase (resolving the conflicts)?

daftfox · 2021-01-29T22:31:40Z

@daftfox - sorry this PR has been around for a while. Could you respond to @jelbourn's questions and rebase (resolving the conflicts)?

Howdy, it's been a while indeed. Will look into resolving the conflicts and answering aforementioned questions at my earliest convenience. Probably 1 Feb. Thanks for the message.

petebacondarwin · 2021-04-17T12:37:21Z

Hi @daftfox - did you manage to find some time to update this PR?

Http client allows an optional timeout option to be passed. An error will be emitted when no response is received within the allotted interval. This reflects XMLHttpRequest behaviour that is standard in all modern browsers. https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/timeout This is behaviour that has been available for 'regular' js xhr requests, but has not yet been implemented in Angular. Alternatives such as interceptors, RxJS or custom client implementation don't offer the same behaviour in a neat package. fixes angular#34421

@gkalpak

Update docs and apply changes requested by @gkalpak

daftfox · 2021-07-07T09:24:56Z

Hey @petebacondarwin. Sorry, it has been a while.
I have rebased my changes on top of master.

petebacondarwin · 2021-08-13T07:47:51Z

Looks like there is a legitimate compilation error:

packages/common/http/test/xhr_mock.ts:58:5 - error TS2300: Duplicate identifier 'timeout'.

58     timeout?: (event: ErrorEvent) => void,
       ~~~~~~~
packages/common/http/test/xhr_mock.ts:62:5 - error TS2300: Duplicate identifier 'timeout'.

62     timeout?: (event: ProgressEvent) => void,
       ~~~~~~~
packages/common/http/test/xhr_mock.ts:62:5 - error TS2717: Subsequent property declarations must have the same type.  Property 'timeout' must be of type '((event: ErrorEvent) => void) | undefined', but here has type '((event: ProgressEvent<EventTarget>) => void) | undefined'.

62     timeout?: (event: ProgressEvent) => void,
       ~~~~~~~

  packages/common/http/test/xhr_mock.ts:58:5
    58     timeout?: (event: ErrorEvent) => void,
           ~~~~~~~
    'timeout' was also declared here.
packages/common/http/test/xhr_mock.ts:126:3 - error TS2393: Duplicate function implementation.

126   mockTimeoutEvent(): void {
      ~~~~~~~~~~~~~~~~
packages/common/http/test/xhr_mock.ts:130:30 - error TS2345: Argument of type 'ProgressEvent<EventTarget>' is not assignable to parameter of type 'ErrorEvent'.
  Type 'ProgressEvent<EventTarget>' is missing the following properties from type 'ErrorEvent': colno, error, filename, lineno, message

130       this.listeners.timeout(mockProgressEvent as ProgressEvent);
                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
packages/common/http/test/xhr_mock.ts:146:3 - error TS2393: Duplicate function implementation.

146   mockTimeoutEvent(error: any): void {

petebacondarwin · 2021-09-15T13:55:55Z

@daftfox - are you able to clean up the failures and finish dealing with the comments on this PR?

alxhub · 2022-02-04T00:27:09Z

(Closing due to no response)

terencehonles · 2022-02-04T18:35:11Z

I think I might be willing to pick this up if that's OK, not sure if there are any issues with that (it looks like the CLA has been signed). I can see what bandwidth I have to pick this up and open a new PR, but I would like to know if that would be OK. I've been watching this PR for a bit but was also waiting for a response and then was busy coming upon the end of the year.

angular-automatic-lock-bot · 2022-03-07T16:11:13Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

jessicajaniuk · 2022-03-08T00:59:37Z

@terencehonles Feel free to pick this up and create a new PR with the fixes addressed. We'd love to land it. If you could mention the related issue #34421 in the PR, that'd be great.

googlebot added the cla: yes label Apr 17, 2020

pullapprove bot requested review from alxhub, gkalpak, IgorMinar and kara April 17, 2020 04:10

gkalpak requested changes Apr 21, 2020

View reviewed changes

daftfox requested a review from gkalpak April 21, 2020 22:36

daftfox force-pushed the http-timeout branch from 2bdc99e to 8adc5cc Compare April 21, 2020 23:07

matsko added area: common/http Issues related to HTTP and HTTP Client feature Issue that requests a new feature labels Apr 23, 2020

ngbot bot modified the milestone: needsTriage Apr 23, 2020

gkalpak approved these changes Jun 16, 2020

View reviewed changes

pullapprove bot requested review from jelbourn and pkozlowski-opensource June 16, 2020 17:05

gkalpak added the action: review The PR is still awaiting reviews from at least one requested reviewer label Jun 16, 2020

kara removed their request for review June 16, 2020 17:27

jelbourn reviewed Jun 17, 2020

View reviewed changes

pullapprove bot requested a review from jelbourn June 17, 2020 03:14

petebacondarwin added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jan 28, 2021

ngbot bot modified the milestones: needsTriage, Backlog Jan 28, 2021

petebacondarwin removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Jan 28, 2021

petebacondarwin marked this pull request as draft May 4, 2021 15:41

petebacondarwin removed request for alxhub and IgorMinar May 4, 2021 15:41

petebacondarwin removed request for pkozlowski-opensource and jelbourn May 4, 2021 15:41

petebacondarwin added the target: minor This PR is targeted for the next minor release label May 4, 2021

daftfox added 2 commits July 7, 2021 15:21

feat(http): change formatting to comply with standards

cc895c1

Update docs and apply changes requested by @gkalpak

daftfox force-pushed the http-timeout branch from 8adc5cc to cc895c1 Compare July 7, 2021 03:25

alxhub closed this Feb 4, 2022

angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(http): http requests to implement xhr timeout #36681

feat(http): http requests to implement xhr timeout #36681

daftfox commented Apr 17, 2020 •

edited

Loading

daftfox commented Apr 20, 2020

gkalpak left a comment

gkalpak left a comment

gkalpak Jun 16, 2020

jelbourn left a comment

jelbourn Jun 17, 2020

jelbourn Jun 17, 2020

gkalpak Jun 17, 2020

jelbourn Jun 17, 2020

jelbourn Jun 17, 2020

jelbourn Jun 17, 2020

gkalpak Jun 17, 2020

petebacondarwin commented Jan 28, 2021

daftfox commented Jan 29, 2021

petebacondarwin commented Apr 17, 2021

daftfox commented Jul 7, 2021

petebacondarwin commented Aug 13, 2021

petebacondarwin commented Sep 15, 2021

alxhub commented Feb 4, 2022

terencehonles commented Feb 4, 2022

angular-automatic-lock-bot bot commented Mar 7, 2022

jessicajaniuk commented Mar 8, 2022

	* The server took too long to respond, but only when a timeout value has been passed in as an optional parameter with the request object or the `HttpClient`'s request methods.
	* The server took too long to respond. This can occur only when the request specifies an explicit `timeout`.

feat(http): http requests to implement xhr timeout #36681

feat(http): http requests to implement xhr timeout #36681

Conversation

daftfox commented Apr 17, 2020 • edited Loading

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

daftfox commented Apr 20, 2020

gkalpak left a 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

jelbourn 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

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

petebacondarwin commented Jan 28, 2021

daftfox commented Jan 29, 2021

petebacondarwin commented Apr 17, 2021

daftfox commented Jul 7, 2021

petebacondarwin commented Aug 13, 2021

petebacondarwin commented Sep 15, 2021

alxhub commented Feb 4, 2022

terencehonles commented Feb 4, 2022

angular-automatic-lock-bot bot commented Mar 7, 2022

jessicajaniuk commented Mar 8, 2022

daftfox commented Apr 17, 2020 •

edited

Loading