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

-pthread leads to circular dependency #14329

Closed
xaroth8088 opened this issue May 30, 2021 · 6 comments
Closed

-pthread leads to circular dependency #14329

xaroth8088 opened this issue May 30, 2021 · 6 comments

Comments

@xaroth8088
Copy link

xaroth8088 commented May 30, 2021

The short version is that the generated .mjs module includes the .worker.js file so that it can instantiate the worker(s), and the .worker.js worker includes the .mjs module so that it has access to important module internals such as the wasmModule instance.

This circular dependency can lead to a problem where the worker cannot be inlined into a final distribution, which complicates life for any developer trying to publish or use multithreaded web assembly functionality contained within a published npm module. For example, webpack gets stuck in an infinite loop when trying to do this (simple repo that demonstrates this problem).

This circular dependency is likely a blocker for this feature request.

@kripken
Copy link
Member

kripken commented Jun 1, 2021

cc @RReverser for thoughts

@RReverser
Copy link
Collaborator

RReverser commented Jun 1, 2021

where the worker cannot be inlined into a final distribution

The worker cannot be inlined into a final distribution by bundlers anyway, because they need to be treated as two different entry points (one for the main thread's script tag, and one for new Worker).

Inlining like in your feature request requires a separate mode (like SINGLE_FILE) to emit code that will do something very different than one used for bundler support, so they won't intersect anyway.

As for the circular dependency warning, yeah, as you noted in your README, it can be safely ignored. It's one of the cases where bundlers try to be overprotective in case you did this by mistake, but such circular dependency is perfectly valid part of ES modules, and it is necessary by design in our case, as we want to instantiate worker JS from the main JS, and to import main JS from worker JS. As long as ordering of loading is correct (and it is), it's not an actual issue.

I suggest this particular issue should be closed in favour of bug #14309 + the feature request #9796 you linked.

@xaroth8088
Copy link
Author

xaroth8088 commented Jun 1, 2021

The worker cannot be inlined into a final distribution by bundlers anyway, because they need to be treated as two different entry points (one for the main thread's script tag, and one for new Worker).

Webpack's worker-loader does web worker inlining via construction of a Blob (see the second example here), and it works fine with the simple workers I've tried out in other projects. Because of this I believe the end goal to be possible, so for me it's just a matter of figuring out the best way to get there.

...we want to instantiate worker JS from the main JS, and to import main JS from worker JS.

Can you say more about why the main JS needs to be instantiated from the worker JS? I have some ideas on how to maybe achieve the same result without the circular dependency, but would love your advice on any design considerations that I wouldn't know about.

@RReverser
Copy link
Collaborator

Webpack's worker-loader does web worker inlining via construction of a Blob

You don't need to use worker-loader for Emscripten's Worker, Webpack should find it automatically. The reason I've pointed to #14309 in my previous comment is that you're using SINGLE_FILE + EXPORT_ES6 + default USE_ES6_IMPORT_META=1 in your sample repo which is already known to be unsupported combo and is tracked separately.

More generally, using Blob for Workers is not a great approach. It might be fine for tiny one-liners, but it's essentially equivalent to eval or new Function and would be 1) non-cacheable by engines, 2) prohibited in CSP-sensitive contexts, 3) statically non-analysable by any downstream tooling since it's now essentially a string.

Much better approach would be to combine worker & main JS in actually a single JS that would detect whether it's currently in the main thread or in a Worker, and this is the reason I've linked to your feature request as 2nd part of this issue as it's something we could do under SINGLE_FILE mode.

^ I believe this also answers "I have some ideas on how to maybe achieve the same result without the circular dependency".

Meanwhile

Can you say more about why the main JS needs to be instantiated from the worker JS?

the reason is that main JS is the one with all the JS bindings for interacting with outside world, whereas Worker is merely a loader for initialising pthreads. Main JS needs to be accessible by both main thread and Worker threads, whereas Worker glue needs to be used only in pthread context, hence the sharing. In most cases it's the most efficient way to share code between them, since one is strictly a dependency of the other, but, as I noted above, we can do something different for SINGLE_FILE and trade the file size for keeping things in one place.

@RReverser
Copy link
Collaborator

Probably also worth noting that it's not even a circular dependency in ESM sense, only from point of view of bundlers since they don't have a good model fit for Workers and treat them as just another kind of modules. This helps with code splitting implementations, but does produce those kinds of spurious warnings.

@RReverser
Copy link
Collaborator

Closing as this is by design.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants