Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(runtime): refactor ESM code to avoid race condition #11150

Merged
merged 3 commits into from
Mar 5, 2021

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Mar 4, 2021

Summary

This refactor makes sure to only call link and evaluate at the top level module or any dynamic imports. This resolves an issue where we produced an invalid module graph, leading newer versions of Node to throw an error.

This also contains a revert of the implementation added in #10892, but keeps the test as the complexity there is not needed anymore with this more correct way of linking and evaluating.

Fixes #11093.

Test plan

All existing tests pass in addition to the bug report in #11093.

@SimenB SimenB changed the title fix(runtime): refactor ESM code to call link at correct time fix(runtime): refactor ESM code to call avoid race condition Mar 4, 2021
@SimenB SimenB changed the title fix(runtime): refactor ESM code to call avoid race condition fix(runtime): refactor ESM code to avoid race condition Mar 4, 2021
@SimenB SimenB requested review from jeysal and thymikee March 4, 2021 07:25
);
}

await this._esmModuleLinkingMap.get(module);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could also choose to just do await if status is unlinked or linking probably. In other cases the promise will be resolved (or rejected) though, so I don't think it really matters

await this._esmModuleLinkingMap.get(module);

if (module.status === 'linked') {
await module.evaluate();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could do a weakmap here as well, but I think the if is enough and I don't think returning an evaluating module is an error? I.e. node will just wait for it to complete evaluation

return this.loadEsmModule(modulePath, query);
const module = await this.loadEsmModule(modulePath, query);

return this.linkAndEvaluateModule(module);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug fix is mostly here - only call linkAndEvaluateModule on the entry point, plus dynamic imports. The old loadEsmModule now just creates the module and sticks it in the cache

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ESM regression on Node.js 15.9
2 participants