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.

Sign up for GitHub

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

Jump to bottom

fix(zone.js): make sure fakeasync use the same id pool with native #54600

Closed
wants to merge 1 commit into from
Closed

fix(zone.js): make sure fakeasync use the same id pool with native #54600

wants to merge 1 commit into from

Conversation

JiaLiPassion
Copy link
Contributor

Close #54323

fakeAsync should use the same timerId pool with native, so they will not conflict.

@dylhunn dylhunn added area: zones target: minor This PR is targeted for the next minor release labels Feb 27, 2024
@ngbot ngbot bot modified the milestone: Backlog Feb 27, 2024
@dylhunn dylhunn added the action: review The PR is still awaiting reviews from at least one requested reviewer label Feb 27, 2024
jonathan-meier
jonathan-meier reviewed Mar 18, 2024
View reviewed changes
packages/zone.js/lib/zone-spec/fake-async-test.ts Outdated
Comment on lines 65 to 66
nativeSetTimeout: (global.setTimeout as any)[Zone.__symbol__('OriginalDelegate')],
nativeClearTimeout: (global.clearTimeout as any)[Zone.__symbol__('OriginalDelegate')],
Copy link

Choose a reason for hiding this comment

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

Shouldn't these two lines rather be the following:

nativeSetTimeout: global[Zone.__symbol__('setTimeout')],
nativeClearTimeout: global[Zone.__symbol__('clearTimeout')],

If setTimeout or clearTimeout is patched again outside of zone.js but after the patching done by zone.js, the OriginalDelegate symbols will most likely be missing on these functions, so grabbing them from global seems safer.

Copy link
Contributor

Choose a reason for hiding this comment

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

@JiaLiPassion Can you address this comment as well? I believe this is accurate and would be correct. The line where the unpatched methods are assigned directly to global is here:

angular/packages/zone.js/lib/common/utils.ts

Line 403 in ee3bb81

delegate = proto[delegateName] = proto[name];

@alxhub alxhub added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Mar 25, 2024
@alxhub
Copy link
Member

alxhub commented Mar 25, 2024

@JiaLiPassion if you could rebase this, and I think the comment about multiple patches is accurate.

@atscott atscott self-requested a review March 25, 2024 22:26
@JiaLiPassion JiaLiPassion force-pushed the timeout-id-conflict branch 2 times, most recently from 3291c9b to b157845 Compare March 26, 2024 02:38
@JiaLiPassion
fix(zone.js): make sure fakeasync use the same id pool with native
403a113 
Close angular#54323

fakeAsync should use the same timerId pool with native, so they will not
conflict.
@JiaLiPassion
Copy link
Contributor Author

@jonathan-meier
thank you for the review and the comment, I have updated the PR.
@alxhub @atscott
I have addressed the comment from @jonathan-meier , and also rebased the PR, please review.

@atscott atscott 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 Mar 26, 2024
@atscott atscott added requires: TGP This PR requires a passing TGP before merging is allowed 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 labels Mar 26, 2024
@atscott
Copy link
Contributor

atscott commented Mar 27, 2024

caretaker note: sync will require patching in these changes to the 3 failing tests in TGP cl/619480497

atscott added a commit to atscott/angular that referenced this pull request Mar 28, 2024
@atscott
refactor(core): Update method of grabbing native setTimeout and rAF
4ff3edf 
As mentioned in angular#54600 (comment)
a more effective way of getting the unpatched version of a zone-patched
API is to grab it from global[Zone.__symbol__('apiname')].
@atscott atscott mentioned this pull request Mar 28, 2024
Closed
atscott added a commit to atscott/angular that referenced this pull request Mar 28, 2024
@atscott
refactor(core): Update method of grabbing native setTimeout and rAF
f813fa4 
As mentioned in angular#54600 (comment)
a more effective way of getting the unpatched version of a zone-patched
API is to grab it from global[Zone.__symbol__('apiname')].
@dylhunn dylhunn removed the request for review from alxhub March 28, 2024 17:29
@dylhunn dylhunn removed the request for review from jonathan-meier March 28, 2024 17:29
@dylhunn
Copy link
Contributor

dylhunn commented Mar 28, 2024

This PR was merged into the repository by commit ddbf6bb.

@dylhunn dylhunn closed this in ddbf6bb Mar 28, 2024
atscott added a commit to atscott/angular that referenced this pull request Mar 29, 2024
@atscott
refactor(core): Update method of grabbing native setTimeout and rAF
c69033f 
As mentioned in angular#54600 (comment)
a more effective way of getting the unpatched version of a zone-patched
API is to grab it from global[Zone.__symbol__('apiname')].
atscott added a commit to atscott/angular that referenced this pull request Mar 29, 2024
@atscott
refactor(core): Update method of grabbing native setTimeout and rAF
02fd7ea 
As mentioned in angular#54600 (comment)
a more effective way of getting the unpatched version of a zone-patched
API is to grab it from global[Zone.__symbol__('apiname')].
atscott added a commit to atscott/angular that referenced this pull request Apr 1, 2024
@atscott
refactor(core): Update method of grabbing native setTimeout and rAF
bd3bb16 
As mentioned in angular#54600 (comment)
a more effective way of getting the unpatched version of a zone-patched
API is to grab it from global[Zone.__symbol__('apiname')].
@jonathan-meier
Copy link

Thank you all for the help in landing this fix! It seems to work great so far 👍

jessicajaniuk pushed a commit that referenced this pull request Apr 2, 2024
@atscott @thePunderWoman
refactor(core): Update method of grabbing native setTimeout and rAF (#…
58942db 
…55092)

As mentioned in #54600 (comment)
a more effective way of getting the unpatched version of a zone-patched
API is to grab it from global[Zone.__symbol__('apiname')].

PR Close #55092
ilirbeqirii pushed a commit to ilirbeqirii/angular that referenced this pull request Apr 6, 2024
@JiaLiPassion @ilirbeqirii
fix(zone.js): make sure fakeasync use the same id pool with native (a…
71fc63a 
…ngular#54600)

Close angular#54323

fakeAsync should use the same timerId pool with native, so they will not
conflict.

PR Close angular#54600
ilirbeqirii pushed a commit to ilirbeqirii/angular that referenced this pull request Apr 6, 2024
@atscott @ilirbeqirii
refactor(core): Update method of grabbing native setTimeout and rAF (a…
8cfa357 
…ngular#55092)

As mentioned in angular#54600 (comment)
a more effective way of getting the unpatched version of a zone-patched
API is to grab it from global[Zone.__symbol__('apiname')].

PR Close angular#55092
@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 May 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jonathan-meier jonathan-meier jonathan-meier left review comments

@atscott atscott atscott approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: zones merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note PullApprove: disable requires: TGP This PR requires a passing TGP before merging is allowed target: minor This PR is targeted for the next minor release
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

FakeAsyncZone and RootZone used in conjunction can clobber interleaved timers of each other
5 participants
@JiaLiPassion @alxhub @atscott @dylhunn @jonathan-meier