Fix TLA: module should wait for its async dependencies #2402

shrinktofit · 2022-07-01T04:18:50Z

I found a BUG: if module b depends on a but a has already been "seen"(postOrderExec-ed), then b won't wait for a's execution if a is an async module.

During bug fixing, I also found test case `./test/system-core.js/Core API/Loading Cases/Top-level await/Should may be incorrect test case(which means equivalent module logic doesn't hold same semantic with ESM). I also updated that expected behaviour.

I've upload my test code here: https://github.com/shrinktofit/systemjs-tla-bug-demo :

case 1 show the BUG this PR wants to resolve.
case 2 show the test case problem as described above.
Both cases are written in ESM/TS and compiled into ESM and SystemJS.
Run them one by one in browser:

shrinktofit · 2022-07-01T04:19:39Z

@joeldenning Please take a look at this

github-actions · 2022-07-01T04:19:41Z

File size impact

Merging tla-bug into main will impact 3 files in 2 groups.

browser (2/2)

File	raw	gzip	brotli	Event
dist/s.min.js	+83 (7,714)	+15 (3,026)	+13 (2,739)	modified
dist/system.min.js	+83 (12,029)	+16 (4,624)	+20 (4,166)	modified
Total size impact	+166 (19,743)	+31 (7,650)	+33 (6,905)

node (1/1)

File	raw	gzip	brotli	Event
dist/system-node.cjs	+110 (519,985)	+24 (126,421)	+22 (84,354)	modified
Total size impact	+110 (519,985)	+24 (126,421)	+22 (84,354)

extras (0/8)

No impact on files in extras group.

_{Generated by file size impact during size-impact#2635661706}

shrinktofit · 2022-07-01T04:22:20Z

src/system-core.js

@@ -242,8 +242,9 @@ var nullContext = Object.freeze(Object.create(null));
 // returns a promise if and only if a top-level await subgraph
 // throws on sync errors
 function postOrderExec (loader, load, seen) {
-  if (seen[load.id])
-    return;
+  if (seen[load.id]) {


If we have seen it. We should return its execution promise or undefined if it's transitively sync.

The problem with this approach, in combination with making load.E the promise for the module and its dependencies, is that it will result in deadlock for async cycles. The reason for this return was to notice when there is a cyclical back edge and exit early.

The problem is that this approach is exiting early for edges are not cyclical back references but parallel dependencies.

I think we can fix this by replacing seen with a parentStack that is unique for each postOrderExec call, which notes the list of parent modules in the execution. We can then change the check to be if (parentStack.includes(load.id)) return; to handle the deadlock cycle back edge.

shrinktofit · 2022-07-01T04:23:49Z

src/system-core.js

@@ -270,9 +271,9 @@ function postOrderExec (loader, load, seen) {
    }
  });
  if (depLoadPromises)
-    return Promise.all(depLoadPromises).then(doExec);
+    return load.E = Promise.all(depLoadPromises).then(doExec);


Previously load.E the module execution promise. I think it should be depLoadPromises then execution promise.

As long as we add the new handling for cyclical back edges that would be fine.

shrinktofit · 2022-07-01T04:24:28Z

test/fixtures/tla/depc.js

@@ -2,7 +2,7 @@ System.register([], function (_export) {
  var reporter;

  var state = 0;
-  var expected = ['c', 'b', 'c', 'b', 'a', 'a', 'main', 'main'];
+  var expected = ['c', 'c', 'b', 'b', 'a', 'a', 'main', 'main'];


You can verify this as I metioned.

shrinktofit · 2022-07-01T04:25:18Z

test/system-core.js

@@ -282,7 +283,7 @@ describe('Loading Cases', function() {
  loader.instantiate = async function (path) {
    const source = await new Promise((resolve, reject) => fs.readFile(path, (err, source) => err ? reject(err) : resolve(source.toString())));
    global.System = loader;
-    eval(source + '//# sourceURL=' + path);
+    eval(source + '//# sourceURL=' + pathToFileURL(path).href);


This allow me to set breakpoints in fixtures in VSCode. If you don't feel right. I can revert it.

nilzona · 2022-07-01T17:42:18Z

Is this the same issue as described in #2395 with the proposed solution in #2396 ?

shrinktofit · 2022-07-03T07:00:40Z

Is this the same issue as described in #2395 with the proposed solution in #2396 ?

I'm not sure. Could you please grab test cases in this PR and test them with your PR?

guybedford · 2022-07-04T19:20:21Z

@shrinktofit it seems we are down a maintainer here, in this case I would suggest one of the two following options:

I can give you maintainership rights to SystemJS, and have you merge the PR once tests are passing. Then as a maintainer you can directly address follow-up concerns with regards to the implementation.
I can provide a thorough review of the implementation, which includes the more detailed edge cases not covered by this PR, in a way that ensures we converge on the correct implementation (this PR implementation is not strictly correct either), but that will require more upfront work to get there.

I can work with either approach, thought I would give you the choice between more work upfront or more work down the road - but there is more work needed either way, and we should consciously decide how we tackle that.

shrinktofit · 2022-07-06T02:17:06Z

@guybedford I'd prefer your thorough review.

guybedford

@shrinktofit I managed to take a look at your case and your approach mostly seems ok to me. There is a deadlock case with your code for the following:

main.js

import './dep.js';
console.log('main loaded');

dep.js

import './main.js';
await new Promise(resolve => setTimeout(resolve, 100));
console.log('dep loaded');

I believe the following will not execute with your PR, and have suggested a fix.

In addition we need to ensure that the tests are fully passing - there seems to be an issue that the CI is flagging that will require fixing before this can land.

guybedford · 2022-07-06T19:17:43Z

src/system-core.js

@@ -242,8 +242,9 @@ var nullContext = Object.freeze(Object.create(null));
 // returns a promise if and only if a top-level await subgraph
 // throws on sync errors
 function postOrderExec (loader, load, seen) {
-  if (seen[load.id])
-    return;
+  if (seen[load.id]) {


The problem with this approach, in combination with making load.E the promise for the module and its dependencies, is that it will result in deadlock for async cycles. The reason for this return was to notice when there is a cyclical back edge and exit early.

The problem is that this approach is exiting early for edges are not cyclical back references but parallel dependencies.

I think we can fix this by replacing seen with a parentStack that is unique for each postOrderExec call, which notes the list of parent modules in the execution. We can then change the check to be if (parentStack.includes(load.id)) return; to handle the deadlock cycle back edge.

guybedford · 2022-07-06T19:18:34Z

src/system-core.js

@@ -270,9 +271,9 @@ function postOrderExec (loader, load, seen) {
    }
  });
  if (depLoadPromises)
-    return Promise.all(depLoadPromises).then(doExec);
+    return load.E = Promise.all(depLoadPromises).then(doExec);


As long as we add the new handling for cyclical back edges that would be fine.

shrinktofit · 2022-07-08T08:07:32Z

I think we can fix this by replacing seen with a parentStack that is unique for each postOrderExec call

It seems this method can not handle dead lock due to dynamic import? Since parentStack only created for each topLevelLoad at a time. For example the following:

// a.js
await import('./b.js');
console.log('b'); // #1

// b.js
await import('./a.js');
console.log('a'); // #2

By the way, I found that both Node.js/Chrome/Firefox behave strangely about the above code -- Node.js siliently terminted the process(exec via node ./a.js). Chrome/Firefox hangle forever(exec via <script type="module">import('./a.js')</script>). In both cases line #1 and #2 have not output.

guybedford · 2022-07-08T18:30:50Z

@shrinktofit yes dynamic import will deadlock, just like any other cyclical promise chain. But in the main static resolver cycles are supported when not using dynamic import reentrancy. Also note that this cycles logic applies to both sync and async. Let me know if you need more information on a graph parent stack approach.

guybedford · 2022-07-24T09:52:33Z

I assume #2404 supersedes this PR?

shrinktofit · 2022-07-25T02:17:48Z

Em.. I'm not sure. Let me test it.

guybedford · 2022-08-10T09:41:23Z

Em.. I'm not sure. Let me test it.

@shrinktofit I'm going ahead with the release now regardless - if there are further issues or this remains unresolved please do follow up further.

shrinktofit · 2022-08-11T10:15:19Z

Em.. I'm not sure. Let me test it.

@shrinktofit I'm going ahead with the release now regardless - if there are further issues or this remains unresolved please do follow up further.

OK. I'll try to go ahead once I finish my works.

qingwabote · 2023-11-28T03:28:34Z

So, is this bug fixed?
I might meet the same bug now.
#2473

ktzhang · 2024-05-11T06:04:33Z

This bug is still valid -- have a PR that resurrects this. #2488

Fix TLA: module should wait for its async dependencies

6ad1995

shrinktofit commented Jul 1, 2022

View reviewed changes

dmail mentioned this pull request Jul 6, 2022

Fix TLA and systemjs jsenv/core#284

Closed

guybedford reviewed Jul 6, 2022

View reviewed changes

Should move load.E in any situation

9f7dc6e

guybedford force-pushed the main branch from 49534c7 to 6d95f11 Compare February 25, 2023 16:07

shrinktofit closed this by deleting the head repository Oct 14, 2023

ktzhang mentioned this pull request May 11, 2024

Fix TLA: module should wait for its async dependencies #2488

Open

Uh oh!

Fix TLA: module should wait for its async dependencies #2402

Fix TLA: module should wait for its async dependencies #2402

Uh oh!

Conversation

shrinktofit commented Jul 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shrinktofit commented Jul 1, 2022

Uh oh!

github-actions bot commented Jul 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

File size impact

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nilzona commented Jul 1, 2022

Uh oh!

shrinktofit commented Jul 3, 2022

Uh oh!

guybedford commented Jul 4, 2022

Uh oh!

shrinktofit commented Jul 6, 2022

Uh oh!

guybedford left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shrinktofit commented Jul 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guybedford commented Jul 8, 2022

Uh oh!

guybedford commented Jul 24, 2022

Uh oh!

shrinktofit commented Jul 25, 2022

Uh oh!

guybedford commented Aug 10, 2022

Uh oh!

shrinktofit commented Aug 11, 2022

Uh oh!

qingwabote commented Nov 28, 2023

Uh oh!

ktzhang commented May 11, 2024

Uh oh!

Uh oh!

shrinktofit commented Jul 1, 2022 •

edited

Loading

github-actions bot commented Jul 1, 2022 •

edited

Loading

shrinktofit commented Jul 8, 2022 •

edited

Loading