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

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

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

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

 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: #18509Fixes: #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#18509Fixes: 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: #18509Fixes: #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: #18509Fixes: #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#18509Fixes: 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 993b716Compare January 26, 2018 22:03

cjihrig force-pushed the master branch from 993b716 to 082f952Compare 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 8e1fb42Compare 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 f936321Compare 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

Uh oh!

Uh oh!

Conversation

devsnek commented Jan 26, 2018

Checklist

Affected core subsystem(s)

Uh oh!

Fishrock123 commented Jan 26, 2018

Uh oh!

TimothyGuJan 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!

TimothyGuJan 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!

iamstolisFeb 1, 2018

Choose a reason for hiding this comment

Uh oh!

devsnekFeb 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!

TimothyGuFeb 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

TimothyGu commented Jan 26, 2018•
edited
Loading

devsnekFeb 1, 2018•
edited
Loading