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.

Sign up for GitHub

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

webpack/bundler Fix: Update worker JS file import URL to be compatible with bundlers #22165

Merged
merged 4 commits into from
Jul 1, 2024
Merged

webpack/bundler Fix: Update worker JS file import URL to be compatible with bundlers #22165

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f95b7b9
Fix URL created for worker JS file import and change test to test it
Twinklebear Jun 29, 2024
bdfe0c2
Single quotes
Twinklebear Jun 29, 2024
698c65f
add note
Twinklebear Jun 29, 2024
9d4c754
Fix failing file diff tests
Twinklebear Jun 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/library_pthread.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -447,13 +447,17 @@ var LibraryPThread = {
var p = trustedTypes.createPolicy(
'emscripten#workerPolicy1',
{
createScriptURL: (ignored) => new URL(import.meta.url)
createScriptURL: (ignored) => new URL("{{{ TARGET_JS_NAME }}}", import.meta.url)
}
);
worker = new Worker(p.createScriptURL('ignored'), workerOptions);
} else
#endif
worker = new Worker(new URL(import.meta.url), workerOptions);
// We need to generate the URL with import.meta.url as the base URL of the JS file
// instead of just using new URL(import.meta.url) because bundler's only recognize
// the first case in their bundling step. The latter ends up producing an invalid
// URL to import from the server (e.g., for webpack the file:// path).
worker = new Worker(new URL('{{{ TARGET_JS_NAME }}}', import.meta.url), workerOptions);
#else
var pthreadMainJs = _scriptName;
#if expectToReceiveOnModule('mainScriptUrlOrBlob')
Expand Down
2 changes: 1 addition & 1 deletion test/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1150,7 +1150,7 @@ def add_on_exit(self, code):
# libraries, for example
def get_emcc_args(self, main_file=False, compile_only=False, asm_only=False):
def is_ldflag(f):
return any(f.startswith(s) for s in ['-sEXPORT_ES6', '-sPROXY_TO_PTHREAD', '-sENVIRONMENT=', '--pre-js=', '--post-js='])
return any(f.startswith(s) for s in ['-sEXPORT_ES6', '-sPROXY_TO_PTHREAD', '-sENVIRONMENT=', '--pre-js=', '--post-js=', '-sPTHREAD_POOL_SIZE='])

args = self.serialize_settings(compile_only or asm_only) + self.emcc_args
if asm_only:
Expand Down
4 changes: 2 additions & 2 deletions test/test_browser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5548,13 +5548,13 @@ def test_error_reporting(self):
def test_webpack(self, es6):
if es6:
shutil.copytree(test_file('webpack_es6'), 'webpack')
self.emcc_args += ['-sEXPORT_ES6']
self.emcc_args += ['-sEXPORT_ES6', '-pthread', '-sPTHREAD_POOL_SIZE=1']
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it should be enough to instead use -sPROXY_TO_PTHREAD. That way not only do we ensure that a thread is started but we actually run some code (the main function) on that thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried -sPROXY_TO_PTHREAD initially but the test hangs instead of explicitly failing even though the same failure case occurs because the exception is caught by callMain:

emscripten/src/postamble.js

Line 115 in 34c1aa3

return handleException(e);
and doesn't seem to bubble up to where it'd crash/fail at the top level of the test JS code. If I build the test with -sPTHREADS_DEBUG, I can see it tries to load the worker from file://, but the exception doesn't make it up to where the test harness can see it failed.

Is there a way I could fix that in the test case to see the exception?

As an semi-related aside, in my own apps I've been building w/ -sINVOKE_RUN=0 because callMain would gobble exceptions/crashes from Wasm making it hard to debug.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you mean by "would gobble exceptions/crashes from Wasm"? callMain should only deal with unwind/exit exceptions and should rethrow all other exceptions (i.e. exceptions that are not specifically supposed to be caught at the entry point).. Are you not seeing that behaviour? Or is it the rethrowning itself that is an issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah exactly, when I step through I see that in handleException it checks for those unwind/exit exceptions and then goes to quit_ where it will rethrow the exception, but after that throw I just end up at this line: https://github.com/emscripten-core/emscripten/blob/main/test/webpack_es6/src/index.mjs#L16 where it prints loaded, and the exception seems to have disappeared? It's pretty strange, since the throw just seems to vanish after that point, when stepping through I don't end up at any other catch block/promise catch handling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Stepping a bit more, I see that it does end up catching the error when calling instantiateAsync and rejecting the promise, but that promise rejection doesn't seem to make it back up the app instantiating the module, so index.mjs doesn't see the rejection

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well that certainly does seem like something worth investigating more. Do you have a global unhandled rejection handler on your page? Is your expectation that the module promise would be rejected? I support that would be the most logical expectation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have a global handler on my page, but the stepping through I'm describing here was actually just on running the test case: https://github.com/emscripten-core/emscripten/blob/main/test/webpack_es6/src/index.mjs . I think that the module promise being rejected if an error was thrown during callMain would be expected behavior, for example if the test code was like below, I'd expect the error to end up in the catch handler and print the error

import Module from './hello.mjs';
Module(params).then((instance) => {
  console.log('loaded');
}).catch((error) => {
  console.error(`Error: ${error}`);
})

The instantiateAsync call:

emscripten/src/preamble.js

Line 1082 in 34c1aa3

instantiateAsync(wasmBinary, wasmBinaryFile, info, receiveInstantiationResult).catch(readyPromiseReject);
has a catch handler that's fired after the exception is rethrown since that's the next spot that catches it, and it calls the promise reject function. However that promise rejection doesn't make it up to the top level of the app so the Module(params) promise is seen as successful instead of rejected (since .then is called instead)

outfile = 'src/hello.mjs'
else:
shutil.copytree(test_file('webpack'), 'webpack')
outfile = 'src/hello.js'
with utils.chdir('webpack'):
self.compile_btest('hello_world.c', ['-sEXIT_RUNTIME', '-sMODULARIZE', '-sENVIRONMENT=web', '-o', outfile])
self.compile_btest('hello_world.c', ['-sEXIT_RUNTIME', '-sMODULARIZE', '-sENVIRONMENT=web,worker', '-o', outfile])
self.run_process(shared.get_npm_cmd('webpack') + ['--mode=development', '--no-devtool'])
shutil.copyfile('webpack/src/hello.wasm', 'webpack/dist/hello.wasm')
self.run_browser('webpack/dist/index.html', '/report_result?exit:0')
Expand Down
4 changes: 2 additions & 2 deletions test/test_other.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ def test_emcc_output_worker_mjs(self, args):
test_file('hello_world.c')] + args)
src = read_file('subdir/hello_world.mjs')
self.assertContained("new URL('hello_world.wasm', import.meta.url)", src)
self.assertContained("new Worker(new URL(import.meta.url), workerOptions)", src)
self.assertContained("new Worker(new URL('hello_world.mjs', import.meta.url), workerOptions)", src)
self.assertContained('export default Module;', src)
self.assertContained('hello, world!', self.run_js('subdir/hello_world.mjs'))

Expand All @@ -386,7 +386,7 @@ def test_emcc_output_worker_mjs_single_file(self):
test_file('hello_world.c'), '-sSINGLE_FILE'])
src = read_file('hello_world.mjs')
self.assertNotContained("new URL('data:", src)
self.assertContained("new Worker(new URL(import.meta.url), workerOptions)", src)
self.assertContained("new Worker(new URL('hello_world.mjs', import.meta.url), workerOptions)", src)
self.assertContained('hello, world!', self.run_js('hello_world.mjs'))

def test_emcc_output_mjs_closure(self):
Expand Down
Loading