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(modules): Immediately resolve follow-up dyn imports to a dyn imported module #14958

Merged
merged 4 commits into from
Jun 25, 2022

Conversation

andreubotella
Copy link
Contributor

When a dynamically imported module gets resolved, any code that comes after an await import() to that module will continue running. However, if that is the last code in the evaluation of another dynamically imported module, that second module will not resolve until the next iteration of the event loop, even though it does not depend on the event loop at all.

When the event loop is being blocked by a long-running operation, such as a long-running timer, or by an async op that might never end, such as with workers or BroadcastChannels, that will result in the second dynamically imported module not being resolved for a while, or ever.

This change fixes this by running the dynamic module loading steps in a loop until no more dynamic modules can be resolved.

Fixes #14726.


This change might regress performance. We should run the benchmarks to make sure it doesn't regress them significantly.

…rted module

When a dynamically imported module gets resolved, any code that comes
after an `await import()` to that module will continue running.
However, if that is the last code in the evaluation of another
dynamically imported module, that second module will not resolve until
the next iteration of the event loop, even though it does not depend
on the event loop at all.

When the event loop is being blocked by a long-running operation, such
as a long-running timer, or by an async op that might never end, such
as with workers or `BroadcastChannel`s, that will result in the second
dynamically imported module not being resolved for a while, or ever.

This change fixes this by running the dynamic module loading steps in
a loop until no more dynamic modules can be resolved.

Fixes denoland#14726.
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM! This is a great fix Andreu. This problem originates from my original implementation of dynamic imports - I had a feeling that not using a loop might be a problem, but we never had a test case demonstrating so.

Thanks for investigating and fixing!

@bartlomieju bartlomieju merged commit 38505db into denoland:main Jun 25, 2022
@andreubotella andreubotella deleted the resolve-followup-dyn-imports branch June 25, 2022 18:56
dsherret pushed a commit to dsherret/deno that referenced this pull request Jun 30, 2022
…rted module (denoland#14958)

When a dynamically imported module gets resolved, any code that comes after an
await import() to that module will continue running. However, if that is the
last code in the evaluation of another dynamically imported module, that second
module will not resolve until the next iteration of the event loop, even though
it does not depend on the event loop at all.

When the event loop is being blocked by a long-running operation, such as a
long-running timer, or by an async op that might never end, such as with workers
or BroadcastChannels, that will result in the second dynamically imported module
not being resolved for a while, or ever.

This change fixes this by running the dynamic module loading steps in a loop
until no more dynamic modules can be resolved.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dynamic import blocked after create a worker
2 participants