Lifecycle recursion by marclaval · Pull Request #28239 · angular/angular

marclaval · 2019-01-18T16:36:06Z

This PR fixes FW-832.

googlebot · 2019-01-18T16:36:12Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

marclaval · 2019-01-18T16:39:36Z

More details about the fix. The integration test that is enabled here shows a very interesting property of the ViewEngine: the init hooks (OnInit, AfterContentInit, and AfterViewInit) are always executed once and only once, even if they recurse or throw.

In fact, this logic is documented here:

angular/packages/core/src/view/provider.ts

Lines 465 to 507 in cabf1c7

    
           // This function calls the ngAfterContentCheck, ngAfterContentInit, 
        
           // ngAfterViewCheck, and ngAfterViewInit lifecycle hooks (depending on the node 
        
           // flags in lifecycle). Unlike ngDoCheck, ngOnChanges and ngOnInit, which are 
        
           // called during a pre-order traversal of the view tree (that is calling the 
        
           // parent hooks before the child hooks) these events are sent in using a 
        
           // post-order traversal of the tree (children before parents). This changes the 
        
           // meaning of initIndex in the view state. For ngOnInit, initIndex tracks the 
        
           // expected nodeIndex which a ngOnInit should be called. When sending 
        
           // ngAfterContentInit and ngAfterViewInit it is the expected count of 
        
           // ngAfterContentInit or ngAfterViewInit methods that have been called. This 
        
           // ensure that despite being called recursively or after picking up after an 
        
           // exception, the ngAfterContentInit or ngAfterViewInit will be called on the 
        
           // correct nodes. Consider for example, the following (where E is an element 
        
           // and D is a directive) 
        
           //  Tree:       pre-order index  post-order index 
        
           //    E1        0                6 
        
           //      E2      1                1 
        
           //       D3     2                0 
        
           //      E4      3                5 
        
           //       E5     4                4 
        
           //        E6    5                2 
        
           //        E7    6                3 
        
           // As can be seen, the post-order index has an unclear relationship to the 
        
           // pre-order index (postOrderIndex === preOrderIndex - parentCount + 
        
           // childCount). Since number of calls to ngAfterContentInit and ngAfterViewInit 
        
           // are stable (will be the same for the same view regardless of exceptions or 
        
           // recursion) we just need to count them which will roughly correspond to the 
        
           // post-order index (it skips elements and directives that do not have 
        
           // lifecycle hooks). 
        
           // 
        
           // For example, if an exception is raised in the E6.onAfterViewInit() the 
        
           // initIndex is left at 3 (by shouldCallLifecycleInitHook() which set it to 
        
           // initIndex + 1). When checkAndUpdateView() is called again D3, E2 and E6 will 
        
           // not have their ngAfterViewInit() called but, starting with E7, the rest of 
        
           // the view will begin getting ngAfterViewInit() called until a check and 
        
           // pass is complete. 
        
           // 
        
           // This algorthim also handles recursion. Consider if E4's ngAfterViewInit() 
        
           // indirectly calls E1's ChangeDetectorRef.detectChanges(). The expected 
        
           // initIndex is set to 6, the recusive checkAndUpdateView() starts walk again. 
        
           // D3, E2, E6, E7, E5 and E4 are skipped, ngAfterViewInit() is called on E1. 
        
           // When the recursion returns the initIndex will be 7 so E1 is skipped as it 
        
           // has already been called in the recursively called checkAnUpdateView().

In Ivy, we don't manage these cases at all.

The recursion one can be easily solved by adding a few flags to guard against it.
The throw one is more challenging. Within a view, the execution on all the init hooks is actually a sequence. If one of them throws, then the exception is not caught by the framework and the current change detection is stopped. During the next change detection, the sequence will resume where it stopped (and all the check hooks will be executed as usual). This means that:
- the state of this sequence is kept in the view
- init hooks may be executed in any view pass, not only the first one
- collected ngOnInit, ngAfterContentInit, ngAfterViewInit are special hooks that needs to be flagged as such, as oppposed to ngDoCheck, ngAfterViewChecked, etc. Executing a special hook changes the state, while executing the other ones don't.

googlebot · 2019-01-24T09:37:58Z

CLAs look good, thanks!

googlebot · 2019-01-25T01:05:02Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

mhevery

Made some changes cda7b03993

googlebot · 2019-01-25T01:05:20Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

mhevery · 2019-01-25T01:06:31Z

http://test/OCL:230818332:BASE:230818351:1548378384444:28a48373

kara · 2019-01-25T03:03:37Z

@Mlaval Looks like this needs a symbol tests updated

googlebot · 2019-01-25T09:08:20Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

marclaval · 2019-01-25T12:30:46Z

@kara I've updated the symbols, it should be good to go

googlebot · 2019-01-25T17:37:32Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

googlebot · 2019-01-25T21:23:13Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

googlebot · 2019-01-25T21:23:40Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

…ould not go infinite loop

googlebot · 2019-01-25T22:30:32Z

CLAs look good, thanks!

PR Close #28239

…ould not go infinite loop (angular#28239) PR Close angular#28239

PR Close angular#28239

angular-automatic-lock-bot · 2019-09-14T15:21:42Z

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.}

marclaval requested review from a team January 18, 2019 16:36

googlebot added the cla: no label Jan 18, 2019

marclaval added action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release comp: ivy labels Jan 18, 2019

ngbot Bot added this to the needsTriage milestone Jan 18, 2019

marclaval force-pushed the lifecycleRecursion branch from fa29a97 to 59b5a2d Compare January 24, 2019 09:37

googlebot added cla: yes and removed cla: no labels Jan 24, 2019

googlebot added cla: no and removed cla: yes labels Jan 25, 2019

mhevery approved these changes Jan 25, 2019

View reviewed changes

mhevery added cla: yes and removed cla: no labels Jan 25, 2019

mhevery added the action: merge The PR is ready for merge by the caretaker label Jan 25, 2019

kara added 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 Jan 25, 2019

googlebot added cla: no and removed cla: yes labels Jan 25, 2019

kara added cla: yes and removed cla: no labels Jan 25, 2019

kara added merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Jan 25, 2019

kara force-pushed the lifecycleRecursion branch from 648356f to 1bf6460 Compare January 25, 2019 21:23

googlebot added cla: no and removed cla: yes labels Jan 25, 2019

kara added cla: yes and removed cla: no labels Jan 25, 2019

marclaval added 2 commits January 25, 2019 14:30

fix(ivy): calling ChangeDetectorRef.detectChanges() from onChanges sh…

f4fb420

…ould not go infinite loop

fix(ivy): init hooks should be called once and only once

466073b

kara force-pushed the lifecycleRecursion branch from 1bf6460 to 466073b Compare January 25, 2019 22:30

jasonaden closed this in 8737506 Jan 25, 2019

jasonaden pushed a commit that referenced this pull request Jan 25, 2019

fix(ivy): init hooks should be called once and only once (#28239)

d83307a

PR Close #28239

vetom pushed a commit to vetom/angular that referenced this pull request Jan 31, 2019

fix(ivy): calling ChangeDetectorRef.detectChanges() from onChanges sh…

189144d

…ould not go infinite loop (angular#28239) PR Close angular#28239

vetom pushed a commit to vetom/angular that referenced this pull request Jan 31, 2019

fix(ivy): init hooks should be called once and only once (angular#28239)

01be817

PR Close angular#28239

marclaval mentioned this pull request Feb 19, 2019

[Ivy] @ViewChild doesn't read me ElementRef #28643

Closed

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 14, 2019

Conversation

marclaval commented Jan 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

googlebot commented Jan 18, 2019

Uh oh!

marclaval commented Jan 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

googlebot commented Jan 24, 2019

Uh oh!

googlebot commented Jan 25, 2019

Uh oh!

mhevery left a comment

Choose a reason for hiding this comment

Uh oh!

googlebot commented Jan 25, 2019

Uh oh!

mhevery commented Jan 25, 2019

Uh oh!

kara commented Jan 25, 2019

Uh oh!

googlebot commented Jan 25, 2019

Uh oh!

marclaval commented Jan 25, 2019

Uh oh!

googlebot commented Jan 25, 2019

Uh oh!

googlebot commented Jan 25, 2019

Uh oh!

googlebot commented Jan 25, 2019

Uh oh!

googlebot commented Jan 25, 2019

Uh oh!

angular-automatic-lock-bot Bot commented Sep 14, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

marclaval commented Jan 18, 2019 •

edited

Loading

marclaval commented Jan 18, 2019 •

edited

Loading