Modules: Fix indirect circular children #4583

rhuanjl · 2018-01-20T00:11:28Z

This re-fixes #4482

Notes:

Test case was previously dependent on load order. Load order was changed which made the issue appear to be fixed (and test pass) when it wasn't actually fixed. - Test case revised to trigger issue irrespective of load order.
Have to complete all ModuleDeclarationInstantiation before we generate any root functions to ensure this issue can never occur.

Conscious the test case may need moving to fit with #4582

… have been instantiated before we start generating any root functions.

obastemur · 2018-01-20T00:14:17Z

@rhuanjl initially looks good to me. Let's see what CI is going to say

boingoing

👍

jackhorton · 2018-01-20T00:21:59Z

lib/Runtime/Language/SourceTextModuleRecord.cpp

 #endif
+        if (childrenModuleSet != nullptr)
+        {
+            childrenModuleSet->EachValue([=](SourceTextModuleRecord* childModuleRecord)


I know this was copied from before, but since this doesnt need to capture anything in the outer scope, much less everything, could we remove the [=] to do so?

It needs to access the childrenModuleSet within the EachValue function - which in turn uses it to populate the childModuleRecord.

Except in the lambda body, only childModuleRecord is referenced, which is passed as a parameter to the lambda. Nothing from the lexical scope is used. With that being said, I just looked again at http://en.cppreference.com/w/cpp/language/lambda and it looks like the capture-all groups still only capture what is required in the lambda body, so there ends up being no actual runtime effect here -- my bad.

jackhorton · 2018-01-20T00:25:17Z

test/es6/module_4482_dep2.js

 //-------------------------------------------------------------------------------------------------------

 //thing1.js
-import { Thing2 } from './module_4482_dep1.js';


Not super familiar with this bug, but is removing the ./ relevant or important?

The "./" isn't important, I just cleaned it up whilst doing this.

fatcerberus · 2018-01-20T01:43:06Z

Have to complete all ModuleDeclarationInstantiation before we generate any root functions to ensure this issue can never occur.

I thought this is what we already do since #4551?

obastemur · 2018-01-20T02:55:40Z

@fatcerberus not exactly . The import order was reverse :/ that was fixed with #4574 .. what we had was a false positive. I wouldn’t be surprised if there are other edge cases show up.

fatcerberus · 2018-01-20T05:43:44Z

I can confirm that this indeed fixes #4482. 😃

@rhuanjl I should point out that the revised testcase here, as written, represents infinite mutual recursion so it's important to ensure that neither Thing1() nor Thing2() ever actually get called during execution of the test suite or the test would inevitably fail.

rhuanjl · 2018-01-20T09:31:33Z

@fatcerberus I'm quite aware of the infinite recursion if anything is called - the functions are not called and don't need to be - the point was to trigger the crash in either direction we needed the references to everything to be there and this seemed to be the neatest way to do it.

Merge pull request #4583 from rhuanjl:moduleAgain This re-fixes #4482 Notes: 1. Test case was previously dependent on load order. Load order was changed which made the issue appear to be fixed (and test pass) when it wasn't actually fixed. - Test case revised to trigger issue irrespective of load order. 2. Have to complete all ModuleDeclarationInstantiation before we generate any root functions to ensure this issue can never occur. Conscious the test case may need moving to fit with #4582

…ircular children Merge pull request #4583 from rhuanjl:moduleAgain This re-fixes #4482 Notes: 1. Test case was previously dependent on load order. Load order was changed which made the issue appear to be fixed (and test pass) when it wasn't actually fixed. - Test case revised to trigger issue irrespective of load order. 2. Have to complete all ModuleDeclarationInstantiation before we generate any root functions to ensure this issue can never occur. Conscious the test case may need moving to fit with #4582

@boingoing

…ing logic Merge pull request #5238 from rhuanjl:loadOrder Fixes #5171 but this may take a bit of explaining. @boingoing please could you take a look at this? Note I've targeted this at master - though would prefer to target 1.8 if allowed as this is fixing a regression that occurred in 1.8 I've targeted master as I'm assuming 1.8 is closed to functionality changes at this point. This PR reverts 2dc946c which apparently fixed some circular module loading bugs by reversing module load order. However the reversed load order resulted in observably different runtime results in some cases - introducing an incompatibility with other runtimes described in issue #5171 **As far as I understand the purpose of the order reversal was to resolve problems where circular imports were not handled correctly, these were:** 1. Issue #4482 But since then #4482 was fixed properly in #4583 - after noting that the order reversal fixed the original POC but did not fix a slightly revised version of the same. 2. A non-specific issue that had resulted in the test-case bug-issue-3257 being disabled - this is an issue that can only occur with either an environment that allows dynamic imports OR an environment that allows multiple root modules to be being loaded at the same time in the same context. This PR includes an alternative fix for the issue that caused bug-issue-3257 to be disabled. And adds a test case that reproduces the same bug regardless of the load order. The revised fix for that was to get rid of the concept of numPendingModules that was used to track when all of a module's children were ready. **Previous logic:** 1. Module is parsed that has children 2. For each child: - if child already parsed do nothing - if child not already parsed increment numPendingModules for the parent and add parent to list of modules to notify when child is parsed 3. When a module is parsed that has no pending children notify any parents in its list 4. When a module is notified by a child decrease numPendingModules for it by 1, when it reaches 0 notify its parent OR if it's a root module or a dynamicly imported module instantiate it and notify the host to execute it **Problem with previous logic** 1. Mod0 parsed, has children Mod1 and Mod2 (schedule async parse job for Mod1 and Mod2) 2. Mod1 also a root module or dynamic module and has Mod0 as a child 3. parse job for Mod1 completes -> notes that Mod0 is already parsed so numPendingModules = 0 4. Host notified to execute Mod1 before Mod2 has been parsed **New logic** 1. Module is parsed that has children 2. For each child: - add parent to list of modules to notify when child is parsed 3. Whenever a module is parsed OR a notification is received: - recursively check if all children and children's children etc. have been parsed - if yes notify parent OR if dynamic/root module -> notify host to execute

rhuanjl added 2 commits January 19, 2018 23:13

Correct test case to be independent of module load order.

8cc46eb

Fixing 4482 again... Ensure that all child modules direct or indirect…

7f45088

… have been instantiated before we start generating any root functions.

obastemur requested a review from boingoing January 20, 2018 00:13

obastemur requested a review from akroshg January 20, 2018 00:19

boingoing approved these changes Jan 20, 2018

View reviewed changes

jackhorton reviewed Jan 20, 2018

View reviewed changes

chakrabot merged commit 7f45088 into chakra-core:release/1.8 Jan 21, 2018

rhuanjl deleted the moduleAgain branch January 30, 2018 22:35

dilijev added the External Contributor label Feb 16, 2018

rhuanjl mentioned this pull request May 26, 2018

Fix module Load order and re-fix circular loading logic #5238

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Modules: Fix indirect circular children #4583

Modules: Fix indirect circular children #4583

Uh oh!

rhuanjl commented Jan 20, 2018

Uh oh!

obastemur commented Jan 20, 2018

Uh oh!

boingoing left a comment

Uh oh!

jackhorton Jan 20, 2018

Uh oh!

rhuanjl Jan 20, 2018

Uh oh!

jackhorton Jan 20, 2018

Uh oh!

jackhorton Jan 20, 2018

Uh oh!

rhuanjl Jan 20, 2018

Uh oh!

fatcerberus commented Jan 20, 2018

Uh oh!

obastemur commented Jan 20, 2018

Uh oh!

fatcerberus commented Jan 20, 2018 •

edited

Loading

Uh oh!

rhuanjl commented Jan 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Modules: Fix indirect circular children #4583

Modules: Fix indirect circular children #4583

Uh oh!

Conversation

rhuanjl commented Jan 20, 2018

Uh oh!

obastemur commented Jan 20, 2018

Uh oh!

boingoing left a comment

Choose a reason for hiding this comment

Uh oh!

jackhorton Jan 20, 2018

Choose a reason for hiding this comment

Uh oh!

rhuanjl Jan 20, 2018

Choose a reason for hiding this comment

Uh oh!

jackhorton Jan 20, 2018

Choose a reason for hiding this comment

Uh oh!

jackhorton Jan 20, 2018

Choose a reason for hiding this comment

Uh oh!

rhuanjl Jan 20, 2018

Choose a reason for hiding this comment

Uh oh!

fatcerberus commented Jan 20, 2018

Uh oh!

obastemur commented Jan 20, 2018

Uh oh!

fatcerberus commented Jan 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rhuanjl commented Jan 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

fatcerberus commented Jan 20, 2018 •

edited

Loading