Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(http): Add timeout option to HTTP requests #57194

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

wartab
Copy link
Contributor

@wartab wartab commented Jul 30, 2024

Add timeout option to both XHR and fetch backends.

POTENTIAL BREAKING CHANGE: Aborted requests with fetch backend no longer throw DOMException

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:

Does this PR introduce a breaking change?

  • Yes
  • No

I am not certain it is very relevant, but the old behaviour would throw a DOMException due to not setting the reason option to the AbortController.
If you think it's too complicated, we can simplify it, by just calling abort() in both manual abort and timeouts without the reason.

Other information

Not quite satisfied with how the test for the fetch backend was done, but I'm not sure how to mock the timeout properly. The previous test for request aborting didn't seem to be very flexible.

@angular-robot angular-robot bot added the detected: feature PR contains a feature commit label Jul 30, 2024
@JeanMeche
Copy link
Member

I'm a bit conflicted about this.
How would justify expending the API surface when this can already be addressed via the rxjs timeout operator ?

@wartab
Copy link
Contributor Author

wartab commented Jul 30, 2024

That's a fair point, but this was mostly in reaction to this PR getting nowhere and seemingly being wanted by people:
#36681 (and also our own needs for this)

With this issue: #34421

@alan-agius4
Copy link
Contributor

While I agree with adding a timeout to eliminate the need for an RxJs timeout, especially since fetch handles this natively, I would expect an aborted fetch request to result in a standard TimeoutError, DOMException. This would also simplify handling TimeoutError errors differently, such as implementing a retry mechanism.

@wartab
Copy link
Contributor Author

wartab commented Jul 30, 2024

Just to avoid any confusion, since RXJs also has a TimeoutError interface. You mean throwing new DOMException('', 'TimeoutError')?
https://developer.mozilla.org/en-US/docs/Web/API/DOMException#timeouterror

@alan-agius4
Copy link
Contributor

Yes, when not using fetch. With fetch this is be handled out of the box.

@wartab
Copy link
Contributor Author

wartab commented Jul 30, 2024

I don't know how Angular works internally when it comes to polyfills, but after looking into it, I'd need to use AbortSignal.any() in order to support the Observable unsubscription and the timeout at the same time. But looking at caniuse, that function only has a 83% support rate. Is that acceptable?

https://caniuse.com/mdn-api_abortsignal_any_static

@JeanMeche
Copy link
Member

JeanMeche commented Jul 30, 2024

Angular doesn't provide any polyfills also we need to support at least the last 2 major versions of the browsers. Safari implemented it in the current major which makes it a no-go.

@wartab
Copy link
Contributor Author

wartab commented Jul 30, 2024

So until then, we'd need to use the setTimeout method for fetch instead of combining the two signals.
And when the signal is aborted from the setTimeout callback, it would be constructed as such new DOMException("signal timed out", "TimeoutError").

According to MDN TimeoutError is a legacy error code: https://developer.mozilla.org/en-US/docs/Web/API/DOMException#timeouterror
And hypothetically, even if a DOMError is given "unsupported" error names, they are still being constructed.

@thePunderWoman thePunderWoman added the area: common/http Issues related to HTTP and HTTP Client label Jul 30, 2024
@ngbot ngbot bot added this to the Backlog milestone Jul 30, 2024
@JeanMeche
Copy link
Member

JeanMeche commented Jul 31, 2024

That looks like the correct alternative.

Beside the pending review, you'll need to update the "golden files". You can do this by running yarn bazel run //packages/common:common_api.accept.

@JeanMeche JeanMeche added the action: review The PR is still awaiting reviews from at least one requested reviewer label Jul 31, 2024
@wartab
Copy link
Contributor Author

wartab commented Jul 31, 2024

I updated the code with the suggestions, however that command fails for me. I'm currently on Windows and am getting this error (Bazel always gives me some trouble):
java.io.IOException: ERROR: src/main/native/windows/processes-jni.cc(408): NativeProcess:WriteStdin(17472): Le canal de communication est sur le point d?Ωtre fermΘ.

I'll be away for a bit more than two weeks and won't be able to address this until then, when I can be on my normal machine.

@wartab
Copy link
Contributor Author

wartab commented Aug 19, 2024

Got back from holidays and got the command to work on my non-Windows machine (I'm not sure if that's supposed to only work on Unix-like systems).
Is there anything else you need me to do besides squashing the commits when we're done?

@wartab
Copy link
Contributor Author

wartab commented Aug 19, 2024

I had to remove the test for what's in the DOMException. I have no idea why, but during unit tests, the information Exception doesn't get treated like in actual browsers and NodeJS.

expect((err.error as DOMException).name).toBe('TimeoutError'); Would incorrectly say it's undefined instead of 'TimeoutError'. I'm fairly confident this isn't an issue with my code, since the test that checks for instanceof DOMException does actually pass.

@JeanMeche
Copy link
Member

JeanMeche commented Aug 19, 2024

Our tests run both in NodeJS and in actual browsers. You could guard your test like we already do in some other tests.
Here is an example:

it('should create `<link>` element when the image priority attr is true', () => {
// Only run this test in a browser since the Node-based DOM mocks don't
// allow to override `HTMLImageElement.prototype.setAttribute` easily.
if (!isBrowser) return;

@JeanMeche
Copy link
Member

Btw, the CLA job is red because you authored commits with 2 different addresses : <vi****t​@lycoops.be>
& <vinc********ops​@horus-software.be>

@wartab
Copy link
Contributor Author

wartab commented Aug 19, 2024

Yes, I know that, I'm still not on the same two computers. I'll squash the commits when everything is okay with my private account.

However I'm not sure why DOMException gets mocked in NodeJS. Is there a difference between DOMException in Node and in browsers? It seems to me that naive tests give always the same results when using it.

@JeanMeche
Copy link
Member

This seems like a known issue:

@wartab
Copy link
Contributor Author

wartab commented Aug 19, 2024

I stumbled upon that issue as well, but from my understanding it seems to not be related.

When I run in Node.Js

const e = new DOMException('', 'AbortError');

console.log(e.name);

I still get "AbortError". But undefined in tests. So it somehow gets eaten at some point.

That said, I don't know how important it is to you guys at the end of the day.

Add timeout option to both XHR and fetch backends.

POTENTIAL BREAKING CHANGE: Aborted requests with fetch backend no longer throw DOMException
@angular-robot angular-robot bot removed the area: common/http Issues related to HTTP and HTTP Client label Oct 24, 2024
@ngbot ngbot bot removed this from the Backlog milestone Oct 24, 2024
@wartab
Copy link
Contributor Author

wartab commented Oct 24, 2024

Sorry I had missed @ajitzero's comment

@pkozlowski-opensource pkozlowski-opensource added the area: common/http Issues related to HTTP and HTTP Client label Oct 31, 2024
@ngbot ngbot bot added this to the Backlog milestone Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action: review The PR is still awaiting reviews from at least one requested reviewer area: common/http Issues related to HTTP and HTTP Client detected: feature PR contains a feature commit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants