Skip to content

fix(zone.js): support Timeout.refresh in Node.js#56852

Closed
alan-agius4 wants to merge 1 commit intoangular:mainfrom
alan-agius4:zone-patch-refresh
Closed

fix(zone.js): support Timeout.refresh in Node.js#56852
alan-agius4 wants to merge 1 commit intoangular:mainfrom
alan-agius4:zone-patch-refresh

Conversation

@alan-agius4
Copy link
Copy Markdown
Contributor

@alan-agius4 alan-agius4 commented Jul 4, 2024 •
edited
Loading

The Timeout object in Node.js has a refresh method, used to restart setTimeout/setInterval timers. Before this commit, Timeout.refresh was not handled, leading to memory leaks when using fetch in Node.js. This issue arose because undici (the Node.js fetch implementation) uses a refreshed setTimeout for cleanup operations.

For reference, see: https://github.com/nodejs/undici/blob/1dff4fd9b1b2cee97c5f8cf44041521a62d3f133/lib/util/timers.js#L45

Fixes: #56586

@alan-agius4 alan-agius4 force-pushed the zone-patch-refresh branch from dc69e62 to 20a2ab6 Compare July 4, 2024 17:50
alan-agius4
alan-agius4 commented Jul 4, 2024
View reviewed changes
Comment thread packages/zone.js/lib/common/timers.ts Outdated
@alan-agius4 alan-agius4 force-pushed the zone-patch-refresh branch 2 times, most recently from 40e9877 to 9871071 Compare July 4, 2024 17:59
@alan-agius4 alan-agius4 added state: WIP area: zones Issues related to zone.js labels Jul 4, 2024
@ngbot ngbot Bot modified the milestone: Backlog Jul 4, 2024
@alan-agius4 alan-agius4 force-pushed the zone-patch-refresh branch 2 times, most recently from edbaf45 to e2a0f06 Compare July 4, 2024 19:20
@alan-agius4 alan-agius4 changed the title fix(zone): handle Timeout.refresh fix(zone.js): handle Timeout.refresh Jul 4, 2024
@alan-agius4 alan-agius4 force-pushed the zone-patch-refresh branch 2 times, most recently from 0b65259 to 0221ac8 Compare July 4, 2024 19:44
@alan-agius4 alan-agius4 closed this Jul 4, 2024
@alan-agius4 alan-agius4 force-pushed the zone-patch-refresh branch from 0221ac8 to 97d718d Compare July 4, 2024 19:46
@alan-agius4 alan-agius4 reopened this Jul 4, 2024
@alan-agius4 alan-agius4 force-pushed the zone-patch-refresh branch from 2f1c3e2 to 60a74c6 Compare July 4, 2024 20:01
alan-agius4
alan-agius4 commented Jul 4, 2024
View reviewed changes
Comment thread packages/zone.js/lib/common/timers.ts Outdated
Copy link
Copy Markdown
Contributor Author

@alan-agius4 alan-agius4 Jul 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@JiaLiPassion, can you elaborate a bit more about the need of this below checks? Why when it’s period and the state is not notScheduled the task is not removed from the tasks list?

Copy link
Copy Markdown
Contributor

@JiaLiPassion JiaLiPassion Jul 11, 2024

Choose a reason for hiding this comment

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

This is a very very old needs angular/zone.js@76c6ebf
zone.js support reschedule the task in a different zoneSpec, I believe there are some edge cases using different zoneSpecs in this way, so here the check is needed. I am not sure there are any such use cases now, if it can pass tests in g3, I think it can be removed

@alan-agius4 alan-agius4 force-pushed the zone-patch-refresh branch from 60a74c6 to d5a52f3 Compare July 5, 2024 08:45
@alan-agius4 alan-agius4 changed the title fix(zone.js): handle Timeout.refresh fix(zone.js): support Timeout.refresh in Node.js Jul 5, 2024
@alan-agius4 alan-agius4 force-pushed the zone-patch-refresh branch from d5a52f3 to 1c9cc54 Compare July 5, 2024 08:46
@alan-agius4 alan-agius4 added target: patch This PR is targeted for the next patch release action: global presubmit The PR is in need of a google3 global presubmit and removed state: WIP action: global presubmit The PR is in need of a google3 global presubmit labels Jul 5, 2024
@alan-agius4
fix(zone.js): support Timeout.refresh in Node.js
827ff1b 
The `Timeout` object in Node.js has a `refresh` method, used to restart `setTimeout`/`setInterval` timers. Before this commit, `Timeout.refresh` was not handled, leading to memory leaks when using `fetch` in Node.js. This issue arose because `undici` (the Node.js fetch implementation) uses a refreshed `setTimeout` for cleanup operations.

For reference, see: https://github.com/nodejs/undici/blob/1dff4fd9b1b2cee97c5f8cf44041521a62d3f133/lib/util/timers.js#L45

Fixes: angular#56586
@alan-agius4 alan-agius4 force-pushed the zone-patch-refresh branch from 1c9cc54 to 827ff1b Compare July 5, 2024 08:53
@alan-agius4
Copy link
Copy Markdown
Contributor Author

alan-agius4 commented Jul 5, 2024 •
edited
Loading

TGP
TGP deflake ✅

@alan-agius4 alan-agius4 added the action: review The PR is still awaiting reviews from at least one requested reviewer label Jul 5, 2024
@alan-agius4 alan-agius4 marked this pull request as ready for review July 5, 2024 16:27
@pullapprove pullapprove Bot requested a review from JiaLiPassion July 5, 2024 16:27
JiaLiPassion
JiaLiPassion approved these changes Jul 11, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@JiaLiPassion JiaLiPassion left a comment

Choose a reason for hiding this comment

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

LGTM!

@alan-agius4 alan-agius4 added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jul 12, 2024
@alan-agius4
Copy link
Copy Markdown
Contributor Author

alan-agius4 commented Jul 12, 2024

Caretaker note: whilst the global presubmit is green, considering the nature of the changes, I would suggest to sync this into G3 as an individual CL.

@Talb2005
Copy link
Copy Markdown

Talb2005 commented Jul 14, 2024

@alan-agius4
This PR also solves the Ping Timeout issue which occurs when using Angular SSR with socket.io
Thank you! Looking forward for this PR to be merged.

@alan-agius4
Copy link
Copy Markdown
Contributor Author

alan-agius4 commented Jul 14, 2024

@Talb2005, glad to hear that.

@Talb2005
Copy link
Copy Markdown

Talb2005 commented Jul 15, 2024

@alan-agius4
Any estimate as to when this would be merged and released?

@Talb2005 Talb2005 mentioned this pull request Jul 15, 2024
Closed
@alan-agius4
Copy link
Copy Markdown
Contributor Author

alan-agius4 commented Jul 15, 2024

@Talb2005, it should be available on NPM this week.

@atscott
Copy link
Copy Markdown
Contributor

atscott commented Jul 16, 2024

This PR was merged into the repository by commit 982f1b1.

The changes were merged into the following branches: main, 18.1.x

@atscott atscott closed this in 982f1b1 Jul 16, 2024
atscott pushed a commit that referenced this pull request Jul 16, 2024
@alan-agius4 @atscott
fix(zone.js): support Timeout.refresh in Node.js (#56852)
4f11d6e 
The `Timeout` object in Node.js has a `refresh` method, used to restart `setTimeout`/`setInterval` timers. Before this commit, `Timeout.refresh` was not handled, leading to memory leaks when using `fetch` in Node.js. This issue arose because `undici` (the Node.js fetch implementation) uses a refreshed `setTimeout` for cleanup operations.

For reference, see: https://github.com/nodejs/undici/blob/1dff4fd9b1b2cee97c5f8cf44041521a62d3f133/lib/util/timers.js#L45

Fixes: #56586

PR Close #56852
@alan-agius4 alan-agius4 deleted the zone-patch-refresh branch July 16, 2024 19:48
@alan-agius4
Copy link
Copy Markdown
Contributor Author

alan-agius4 commented Jul 18, 2024

@Talb2005, zone.js 0.14.8 has been released and contains this fix.

@angular-automatic-lock-bot
Copy link
Copy Markdown

angular-automatic-lock-bot Bot commented Aug 18, 2024

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 Aug 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@JiaLiPassion JiaLiPassion JiaLiPassion approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker area: zones Issues related to zone.js merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: patch This PR is targeted for the next patch release

Projects

None yet

Milestone

Backlog

Development

Successfully merging this pull request may close these issues.

Memory leakage in SSR while using native node fetch

4 participants

@alan-agius4 @Talb2005 @atscott @JiaLiPassion