src, loader: return promises from link by devsnek · Pull Request #18394 · nodejs/node · GitHub

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

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

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.

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

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.

comments above addressed

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>

Landed in edbcf7c

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

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>

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

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

@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>

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 outstanding review comments, 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 outstanding review comments, 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

                   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();

src, loader: return promises from link by devsnek · Pull Request #18394 · nodejs/node · GitHub

Uh oh!

Uh oh!

src, loader: return promises from link - #18394

src, loader: return promises from link#18394devsnek wants to merge 2 commits intonodejs:masternodejs/node:masterfrom devsnek:fix/module-link-timingdevsnek/node:fix/module-link-timingCopy head branch name to clipboard

Conversation

devsnek commented Jan 26, 2018

Checklist

Affected core subsystem(s)

Uh oh!

Fishrock123 commented Jan 26, 2018

Uh oh!

TimothyGu Jan 26, 2018

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

TimothyGu Jan 27, 2018

Choose a reason for hiding this comment

Uh oh!

devsnek commented Jan 28, 2018

Uh oh!

BridgeAR commented Jan 31, 2018

Uh oh!

BridgeAR commented Feb 1, 2018

Uh oh!

TimothyGu commented Feb 1, 2018

Uh oh!

iamstolis Feb 1, 2018

Choose a reason for hiding this comment

Uh oh!

devsnek Feb 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimothyGu Feb 1, 2018

Choose a reason for hiding this comment

Uh oh!

MylesBorins commented Feb 20, 2018

Uh oh!

addaleax commented Feb 20, 2018

Uh oh!

MylesBorins commented Feb 21, 2018

Uh oh!

MylesBorins commented Mar 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

src, loader: return promises from link#18394
devsnek wants to merge 2 commits into
nodejs:masterfrom
devsnek:fix/module-link-timing

TimothyGu commented Jan 26, 2018 •
edited
Loading

devsnek Feb 1, 2018 •
edited
Loading