Skip to content

Conversation

ItNoN
Copy link
Contributor

@ItNoN ItNoN commented Feb 9, 2021

Fixes #22324

PR Checklist

Please check if your PR fulfills the following requirements:

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?

Issue Number: #22324

Before this change, when Google Chrome cancels a XMLHttpRequest, an Observable of the response never finishes. This happens, for example, when you put your computer to sleep or just press Ctrl+S to save the browser page.

What is the new behavior?

After this commit, if request is canceled or aborted an appropriate Observable will be completed with an error.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

I was thinking about a new HttpEventType that tracks the abort event, but it seems to me that if forces beyond your control have decided your request is no longer going to continue, it looks more like an error.

I couldn't find a way to reproduce this bug with JSONP. It appears JSONP request is not aborted.

Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ItNoN thanks for the fix, the change looks good to me 👍

This change also looks related to #39807 and this PR will most likely require a rebase once #39807 lands (as they both update the same code).

I'm also adding @alxhub as a reviewer to this PR, since he participated in discussion in the original thread (#22324) and may have more context.

Thank you.

@AndrewKushnir AndrewKushnir requested a review from alxhub February 9, 2021 21:28
@AndrewKushnir AndrewKushnir added action: review The PR is still awaiting reviews from at least one requested reviewer area: common/http Issues related to HTTP and HTTP Client target: patch This PR is targeted for the next patch release labels Feb 9, 2021
@ngbot ngbot bot modified the milestone: Backlog Feb 9, 2021
@ItNoN
Copy link
Contributor Author

ItNoN commented Feb 9, 2021

@AndrewKushnir, thank you for your quick response!
Yes, these PRs are very similar by structure. I'll make a rebase after #39807 is merged.

Thanks!

Copy link
Member

@alxhub alxhub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had some concerns originally, but I've convinced myself that this PR is good to go!

  1. There is a question of whether abort events should be considered errors, or treated as a separate HttpEventType. I was leaning towards the latter, but after looking at the triggers for abort (beyond calling XmlHttpRequest.abort()), I think there's no value in differentiating them for users. Indeed - grouping all possible errors together is very useful.

  2. This has nothing to do with the abort() we do when a request Observable is unsubscribed, because a. there is no possibility to emit values or errors after an unsubscription in general, and b. all event listeners are removed prior to that abort() anyway.

Therefore, I think this fix is good to go.

@AndrewKushnir AndrewKushnir removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Feb 9, 2021
@AndrewKushnir AndrewKushnir removed the request for review from IgorMinar February 9, 2021 22:17
@AndrewKushnir AndrewKushnir added the action: presubmit The PR is in need of a google3 presubmit label Feb 9, 2021
@AndrewKushnir
Copy link
Contributor

@ItNoN, FYI PR #39807 has landed a few mins ago and that caused some merge conflicts for this PR (as expected). Could you please rebase this PR and resolve merge conflicts? Thank you.

@AndrewKushnir AndrewKushnir 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 Feb 9, 2021
Before this change, when Google Chrome cancels a XMLHttpRequest, an Observable of the response
never finishes. This happens, for example, when you put your computer to sleep or just press
Ctrl+S to save the browser page. After this commit, if request is canceled or aborted an
appropriate Observable will be completed with an error.

Fixes angular#22324
@ItNoN ItNoN force-pushed the http-client-abort branch from 9d922da to bda7e37 Compare February 9, 2021 23:26
@ItNoN
Copy link
Contributor Author

ItNoN commented Feb 9, 2021

@AndrewKushnir, done.

@AndrewKushnir AndrewKushnir removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Feb 10, 2021
@AndrewKushnir
Copy link
Contributor

FYI, presubmits are successful for the changes in this PR, adding "merge" label.

@ItNoN thanks for contributing to Angular!

@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker and removed action: presubmit The PR is in need of a google3 presubmit labels Feb 10, 2021
@alxhub alxhub added target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release labels Feb 10, 2021
@alxhub
Copy link
Member

alxhub commented Feb 10, 2021

@ItNoN This PR unfortunately conflicts with the 11.2.x branch, so I can't merge it there. Can you open a separate PR against that branch directly? I'll go ahead and merge this to master.

@alxhub alxhub closed this in 3897265 Feb 10, 2021
@ItNoN ItNoN deleted the http-client-abort branch February 17, 2021 12:58
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: common/http Issues related to HTTP and HTTP Client cla: yes target: minor This PR is targeted for the next minor release type: bug/fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HttpClient doesn't throw on observable if Chrome cancels a request
3 participants