src, loader: return promises from link #18394

devsnek · 2018-01-26T15:49:15Z

Returns the promises created by link so that they can be awaited to get
rid of race conditions while resolving and loading modules.

closes #18249

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

loader, src

Fishrock123 · 2018-01-26T15:59:39Z

CI: https://ci.nodejs.org/job/node-test-pull-request/12750/

TimothyGu · 2018-01-26T16:13:20Z

src/module_wrap.cc

@@ -173,9 +177,11 @@ void ModuleWrap::Link(const FunctionCallbackInfo<Value>& args) {
    }
    Local<Promise> resolve_promise = resolve_return_value.As<Promise>();
    obj->resolve_cache_[specifier_std].Reset(env->isolate(), resolve_promise);
+
+    USE(promises->Set(mod_context, specifier, resolve_promise));


This should crash if the Set fails (promises->Set().FromJust()). USE is only for when we don’t care about the result of execution, and should be used sparingly.

TimothyGu · 2018-01-26T16:14:36Z

Why is this patch preferred over #18249 (comment)?

Edit: answered on IRC:

@devsnek | @TimothyGu in reference to 18394 this seemed simpler
@devsnek | and made me not have to pass around v8::Object

TimothyGu · 2018-01-27T08:55:18Z

lib/internal/loader/ModuleJob.js

+      const jobs = [];
+      const promises = this.module.link(async (specifier) => {
+        const job = await this.loader.getModuleJob(specifier, url);
+        jobs.push(job);


If we were to keep a jobs array, we would want to store the promises from getModuleJob to ensure determinism in the order of jobs.

Returns the promises created by link so that they can be awaited to get rid of race conditions while resolving and loading modules.

devsnek · 2018-01-28T18:42:00Z

comments above addressed

BridgeAR · 2018-01-31T00:33:56Z

CI https://ci.nodejs.org/job/node-test-pull-request/12826/

Returns the promises created by link so that they can be awaited to get rid of race conditions while resolving and loading modules. PR-URL: nodejs#18394 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

BridgeAR · 2018-02-01T08:34:01Z

Landed in edbcf7c

TimothyGu · 2018-02-01T09:06:42Z

Unfortunately this requires a corresponding change in lib/internal/vm/Module.js, which did not exist when this PR was created...

iamstolis · 2018-02-01T15:23:59Z

src/module_wrap.cc

@@ -173,9 +177,11 @@ void ModuleWrap::Link(const FunctionCallbackInfo<Value>& args) {
    }
    Local<Promise> resolve_promise = resolve_return_value.As<Promise>();
    obj->resolve_cache_[specifier_std].Reset(env->isolate(), resolve_promise);
+
+    promises->Set(mod_context, specifier, resolve_promise).FromJust();


This line does not set array items (i.e. it does not set promises[0], promises[1], etc.) It sets promises[specifier] i.e. some non-array properties of promises object. So, the array returned from this function is not populated properly and the corresponding await SafePromise.all(promises) does not do what was expected. There should be i instead of specifier, i.e., promises->Set(mod_context, i, resolve_promise).FromJust();, right?

yea i actually just caught this while testing my changes for vm.Module. i'm kinda sad this pr landed without tests, as it doesn't work. i have another pr incoming anyway, which will fix this, and includes tests that will actually ensure this behavior does what it is supposed to do.

Should've caught this while reviewing... my bad as well.

This commit fixes up some issues in #18394. * Switch vm.Module internals to use the new link method properly * Fix bug with ModuleWrap::Link * Add tests for ModuleWrap::Link PR-URL: #18509 Fixes: #18249 Refs: #18394 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

MylesBorins · 2018-02-20T23:21:58Z

This lands cleanly, but is causing v9.x to break during compilation. Should it be backported?

../src/module_wrap.cc:154:9: error: unknown type name 'Array'; did you mean 'v8::Array'?
  Local<Array> promises = Array::New(isolate,
        ^~~~~
        v8::Array
../deps/v8/include/v8.h:3553:17: note: 'v8::Array' declared here
class V8_EXPORT Array : public Object {
                ^
../src/module_wrap.cc:154:27: error: use of undeclared identifier 'Array'; did you mean 'v8::Array'?
  Local<Array> promises = Array::New(isolate,
                          ^~~~~
                          v8::Array
../deps/v8/include/v8.h:3553:17: note: 'v8::Array' declared here
class V8_EXPORT Array : public Object {
                ^
2 errors generated.
make[1]: *** [/Users/mborins/code/node/v9.x/out/Release/obj.target/node_lib/src/module_wrap.o] Error 1

addaleax · 2018-02-20T23:24:30Z

@MylesBorins These kinds of errors can be fixed by adding using v8::Array to the top of the file. If you want, you can always do that during backporting – it’s completely harmless, the worst thing that can happen is the linter complaining if the list of using statements isn’t alphabetically sorted.

MylesBorins · 2018-02-21T00:14:13Z

@addaleax thanks, that worked

Returns the promises created by link so that they can be awaited to get rid of race conditions while resolving and loading modules. PR-URL: #18394 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

This commit fixes up some issues in nodejs#18394. * Switch vm.Module internals to use the new link method properly * Fix bug with ModuleWrap::Link * Add tests for ModuleWrap::Link PR-URL: nodejs#18509 Fixes: nodejs#18249 Refs: nodejs#18394 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

Returns the promises created by link so that they can be awaited to get rid of race conditions while resolving and loading modules. PR-URL: #18394 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

This commit fixes up some issues in #18394. * Switch vm.Module internals to use the new link method properly * Fix bug with ModuleWrap::Link * Add tests for ModuleWrap::Link PR-URL: #18509 Fixes: #18249 Refs: #18394 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

Returns the promises created by link so that they can be awaited to get rid of race conditions while resolving and loading modules. PR-URL: #18394 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

This commit fixes up some issues in #18394. * Switch vm.Module internals to use the new link method properly * Fix bug with ModuleWrap::Link * Add tests for ModuleWrap::Link PR-URL: #18509 Fixes: #18249 Refs: #18394 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

MylesBorins · 2018-03-20T16:45:25Z

Should this be backported to v8.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

Returns the promises created by link so that they can be awaited to get rid of race conditions while resolving and loading modules. PR-URL: nodejs#18394 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

This commit fixes up some issues in nodejs#18394. * Switch vm.Module internals to use the new link method properly * Fix bug with ModuleWrap::Link * Add tests for ModuleWrap::Link PR-URL: nodejs#18509 Fixes: nodejs#18249 Refs: nodejs#18394 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jan 26, 2018

TimothyGu reviewed Jan 26, 2018

View reviewed changes

TimothyGu self-assigned this Jan 26, 2018

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:03

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

TimothyGu reviewed Jan 27, 2018

View reviewed changes

devsnek force-pushed the fix/module-link-timing branch from 0fbccf9 to 8e1fb42 Compare January 27, 2018 16:24

devsnek added 2 commits January 28, 2018 12:25

src, loader: return promises from link

b435060

Returns the promises created by link so that they can be awaited to get rid of race conditions while resolving and loading modules.

clean up from comments

f936321

devsnek force-pushed the fix/module-link-timing branch from be04b4a to f936321 Compare January 28, 2018 18:41

TimothyGu approved these changes Jan 28, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 31, 2018

BridgeAR closed this Feb 1, 2018

jdalton mentioned this pull request Feb 1, 2018

Fragile/buggy implementation of ES module loading #18249

Closed

devsnek deleted the fix/module-link-timing branch February 1, 2018 15:14

iamstolis reviewed Feb 1, 2018

View reviewed changes

devsnek mentioned this pull request Feb 3, 2018

module: add changes that should have been in 18394 #18509

Closed

3 tasks

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 4, 2018

MylesBorins added the backport-requested-v9.x label Feb 20, 2018

MylesBorins removed the backport-requested-v9.x label Feb 21, 2018

MylesBorins mentioned this pull request Feb 21, 2018

v9.6.0 proposal #18902

Merged

devsnek added the backported-to-v9.x label Feb 21, 2018

MylesBorins added backport-requested-v8.x labels Mar 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src, loader: return promises from link #18394

src, loader: return promises from link #18394

devsnek commented Jan 26, 2018

Fishrock123 commented Jan 26, 2018

TimothyGu Jan 26, 2018

TimothyGu commented Jan 26, 2018 •

edited

Loading

TimothyGu Jan 27, 2018

devsnek commented Jan 28, 2018

BridgeAR commented Jan 31, 2018

BridgeAR commented Feb 1, 2018

TimothyGu commented Feb 1, 2018

iamstolis Feb 1, 2018

devsnek Feb 1, 2018 •

edited

Loading

TimothyGu Feb 1, 2018

MylesBorins commented Feb 20, 2018

addaleax commented Feb 20, 2018

MylesBorins commented Feb 21, 2018

MylesBorins commented Mar 20, 2018

src, loader: return promises from link #18394

src, loader: return promises from link #18394

Conversation

devsnek commented Jan 26, 2018

Checklist

Affected core subsystem(s)

Fishrock123 commented Jan 26, 2018

TimothyGu Jan 26, 2018

Choose a reason for hiding this comment

TimothyGu commented Jan 26, 2018 • edited Loading

TimothyGu Jan 27, 2018

Choose a reason for hiding this comment

devsnek commented Jan 28, 2018

BridgeAR commented Jan 31, 2018

BridgeAR commented Feb 1, 2018

TimothyGu commented Feb 1, 2018

iamstolis Feb 1, 2018

Choose a reason for hiding this comment

devsnek Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

TimothyGu Feb 1, 2018

Choose a reason for hiding this comment

MylesBorins commented Feb 20, 2018

addaleax commented Feb 20, 2018

MylesBorins commented Feb 21, 2018

MylesBorins commented Mar 20, 2018

TimothyGu commented Jan 26, 2018 •

edited

Loading

devsnek Feb 1, 2018 •

edited

Loading