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

Macro Tasks left running after failed http requests with Unknown Error, preventing protractor from completing tests #38795

Closed
wfairclough opened this issue Sep 10, 2020 · 8 comments · Fixed by kubikowski/inanity#26
Assignees
Labels
area: zones freq2: medium regression Indicates than the issue relates to something that worked in a previous version state: confirmed type: bug/fix
Milestone

Comments

@wfairclough
Copy link

🐞 bug report

Affected Package

@angular/common/http

Is this a regression?

Yes.
Worked in @angular/common@9.1.12

Description

A macroTask is left running when certain http errors occur and this prevents protractor for completing since there are tasks left running in the ngZone.

🔬 Minimal Reproduction

https://github.com/wfairclough/AngularHttpMacroTaskMinimalReproduction

Run npm install

Run with npx ng serve

Open Chrome Browser to http://localhost:4200

Reproduce Bug

  1. Click Button Make Successful Request and see successul response.
  2. Click Button Check Task Counts and see the open micro and macro task counts in the ngZone.
  3. Click Button Make Failed Request and see the error response.
  4. Click Button Check Task Counts again and notice that the macroTask count is now 1.
  5. Repeat failed requests as many times as you like and the marcroTask count will just keep increasing.

🔥 Exception or Error

image

No macroTasks should be left running after this failed http request.

🌍 Your Environment

Angular Version:



     _                      _                 ____ _     ___
    / \   _ __   __ _ _   _| | __ _ _ __     / ___| |   |_ _|
   / △ \ | '_ \ / _` | | | | |/ _` | '__|   | |   | |    | |
  / ___ \| | | | (_| | |_| | | (_| | |      | |___| |___ | |
 /_/   \_\_| |_|\__, |\__,_|_|\__,_|_|       \____|_____|___|
                |___/
    

Angular CLI: 10.1.0
Node: 10.20.1
OS: linux x64

Angular: 10.1.1
... animations, common, compiler, compiler-cli, core, forms
... platform-browser, platform-browser-dynamic, router
Ivy Workspace: Yes

Package                           Version
-----------------------------------------------------------
@angular-devkit/architect         0.1001.0
@angular-devkit/build-angular     0.1001.0
@angular-devkit/build-optimizer   0.1001.0
@angular-devkit/build-webpack     0.1001.0
@angular-devkit/core              10.1.0
@angular-devkit/schematics        10.1.0
@angular/cli                      10.1.0
@ngtools/webpack                  10.1.0
@schematics/angular               10.1.0
@schematics/update                0.1001.0
rxjs                              6.6.3
typescript                        4.0.2
webpack                           4.44.1

Anything else relevant?
Error occurs in both browsers I tested with Chrome and Firefox.

@ngbot ngbot bot added this to the needsTriage milestone Sep 10, 2020
@JoostK
Copy link
Member

JoostK commented Sep 10, 2020

Confirmed and identified #37367 to be the cause (reverting that change fixes the issue).

@JoostK JoostK added freq2: medium regression Indicates than the issue relates to something that worked in a previous version type: bug/fix labels Sep 10, 2020
@ngbot ngbot bot modified the milestones: needsTriage, Backlog Sep 10, 2020
@JoostK
Copy link
Member

JoostK commented Sep 13, 2020

@JiaLiPassion could you have a look at zone.js's XHR patching to see if this is something that can be addressed from within zone.js, without needing to revert #37367?

@JiaLiPassion
Copy link
Contributor

JiaLiPassion commented Sep 14, 2020

@JoostK , sure, I just checked this one, I think this can be handled inside zone.js, it is a bug about integrated zone.js with @angular/common/http, I will fix it.

@JiaLiPassion JiaLiPassion self-assigned this Sep 14, 2020
JiaLiPassion added a commit to JiaLiPassion/angular that referenced this issue Sep 14, 2020
Close angular#38795

in XMLHttpRequest patch, when get `readystatechange` event, zone.js try to
invoke `load` event listener first, then call `invokeTask` to finish the
`XMLHttpRequest::send` macroTask, but if the request failed because the
server can not be reached, the `load` event listener will not be invoked,
so the `invokeTask` of the `XMLHttpRequest::send` will not be triggered either,
so we will have a non finished macroTask there which will make the Zone
not stable, also memory leak.

So in this PR, if the `XMLHttpRequest.status = 0` when we get the `readystatechange`
event, that means something wents wrong before we reached the server, we need to
invoke the task to finish the macroTask.
JiaLiPassion added a commit to JiaLiPassion/angular that referenced this issue Sep 14, 2020
Close angular#38795

in the XMLHttpRequest patch, when get `readystatechange` event, zone.js try to
invoke `load` event listener first, then call `invokeTask` to finish the
`XMLHttpRequest::send` macroTask, but if the request failed because the
server can not be reached, the `load` event listener will not be invoked,
so the `invokeTask` of the `XMLHttpRequest::send` will not be triggered either,
so we will have a non finished macroTask there which will make the Zone
not stable, also memory leak.

So in this PR, if the `XMLHttpRequest.status = 0` when we get the `readystatechange`
event, that means something wents wrong before we reached the server, we need to
invoke the task to finish the macroTask.
mhevery pushed a commit that referenced this issue Sep 18, 2020
…rs (#38836)

Close #38795

in the XMLHttpRequest patch, when get `readystatechange` event, zone.js try to
invoke `load` event listener first, then call `invokeTask` to finish the
`XMLHttpRequest::send` macroTask, but if the request failed because the
server can not be reached, the `load` event listener will not be invoked,
so the `invokeTask` of the `XMLHttpRequest::send` will not be triggered either,
so we will have a non finished macroTask there which will make the Zone
not stable, also memory leak.

So in this PR, if the `XMLHttpRequest.status = 0` when we get the `readystatechange`
event, that means something wents wrong before we reached the server, we need to
invoke the task to finish the macroTask.

PR Close #38836
@wfairclough
Copy link
Author

Hi @JiaLiPassion,

Thanks for looking into this issue I was seeing. I have tried updating my minimal reproduction repo to 10.1.4 to see that your commit fixes the issues, although I am still seeing macroTasks left running after requests to non-existent urls. I believe the fix should be in the release, am I mistaken?


     _                      _                 ____ _     ___
    / \   _ __   __ _ _   _| | __ _ _ __     / ___| |   |_ _|
   / △ \ | '_ \ / _` | | | | |/ _` | '__|   | |   | |    | |
  / ___ \| | | | (_| | |_| | | (_| | |      | |___| |___ | |
 /_/   \_\_| |_|\__, |\__,_|_|\__,_|_|       \____|_____|___|
                |___/
    

Angular CLI: 10.1.4
Node: 10.20.1
OS: linux x64

Angular: 10.1.4
... animations, cli, common, compiler, compiler-cli, core, forms
... platform-browser, platform-browser-dynamic, router
Ivy Workspace: Yes

Package                         Version
---------------------------------------------------------
@angular-devkit/architect       0.1001.4
@angular-devkit/build-angular   0.1001.4
@angular-devkit/core            10.1.4
@angular-devkit/schematics      10.1.4
@schematics/angular             10.1.4
@schematics/update              0.1001.4
rxjs                            6.6.3
typescript                      4.0.2 

https://github.com/wfairclough/AngularHttpMacroTaskMinimalReproduction

@JoostK
Copy link
Member

JoostK commented Oct 6, 2020

@wfairclough Could you try with zone.js 0.11.2? It's released separately from the main framework; the changelog can be found here.

@wfairclough
Copy link
Author

@JoostK Thanks for pointing that out. Doesn't look like 0.11.2 has been released to npm as of now, I will try it once it has been published.

@JoostK
Copy link
Member

JoostK commented Oct 6, 2020

@wfairclough Oh, I didn't realize that. But now that you mention it, I remembered there's #39094 for that.

@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 Nov 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.