fix(common): abort HTTP requests once application is destroyed #59575
Conversation
0d2ee56 to
c6662f0
Compare
c6662f0 to
2d176f6
Compare
cdc5b27 to
be512d3
Compare
be512d3 to
230ea2c
Compare
230ea2c to
31ad250
Compare
|
@AndrewKushnir updated the nit comment. |
31ad250 to
315d577
Compare
315d577 to
92932e7
Compare
92932e7 to
1a934fe
Compare
|
@krivanek06 worth checking if it will be a breaking change for the service you described in your article "Simple User Event Tracker". |
|
Hey @arturovt, I think this fix is in the right spirit - cancelling outstanding work on destruction makes sense. But I suspect it should be implemented differently. Right now, this cancels at the backend, and specifically aborts the Instead, we should cancel requests at the frontend of It should then be straightforward to test that an ongoing request is cancelled when the What do you think? |
|
@alxhub your comments make sense. Before we move forward — just out of curiosity — should we still care about XHR-based applications? @JeanMeche has a PR where fetch will be used by default, with XHR as a fallback. |
|
We haven't decided yet if we should change the default backend. We should still assume XHR is the default. |
|
@arturovt Do you still intend to apply the suggested changes ? |
|
Yes, I'll proceed with the proposed implementation. |
1a934fe to
0ab9a86
Compare
0ab9a86 to
e9a0c27
Compare
packages/common/http/src/client.ts
Outdated
| // Attempt to retrieve a `DestroyRef` optionally. | ||
| // For backwards compatibility reasons, this cannot be required. | ||
| if (isInInjectionContext()) { | ||
| inject(DestroyRef, {optional: true})?.onDestroy(() => this.destroyed$.next()); | ||
| } |
There was a problem hiding this comment.
This is indeed the unfortunate reality we're in.
I would suggest that we log a warning if the HttpClient wasn't created in an injection context (which it arguable should) and that cleanup won't be ensured.
7cf3bcc to
09afa1d
Compare
In this commit, we abort any pending HTTP requests when the application is destroyed to prevent memory leaks and the execution of operations that should not occur once resources are released. For example, if an HTTP response is returned after the application is destroyed and someone calls `runInInjectionContext`, it would throw an error indicating that the injector has already been destroyed. This ensures a graceful cleanup, ensuring that no requests proceed once the root injector is destroyed.
09afa1d to
2e8acd1
Compare
In this commit, we abort any pending HTTP requests when the application is destroyed to prevent memory leaks and the execution of operations that should not occur once resources are released. For example, if an HTTP response is returned after the application is destroyed and someone calls
runInInjectionContext, it would throw an error indicating that the injector has already been destroyed. This ensures a graceful cleanup, ensuring that no requests proceed once the root injector is destroyed.