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

Jump to bottom

[Pthreads] Fix worker.js in ES6 module environments #21041

Merged
merged 7 commits into from
Jan 10, 2024
Merged

[Pthreads] Fix worker.js in ES6 module environments #21041

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
86a28fe
fix
kripken Jan 9, 2024
dc7c00f
feedback: import.meta.url
kripken Jan 9, 2024
5435bf4
python lint
kripken Jan 9, 2024
42cf278
fix
kripken Jan 9, 2024
578e3fa
use .worker.mjs when EXPORT_ES6
kripken Jan 9, 2024
49ad508
fix tests
kripken Jan 9, 2024
48dc5ef
fix
kripken Jan 10, 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
10 changes: 9 additions & 1 deletion src/worker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,13 @@ var ENVIRONMENT_IS_NODE = typeof process == 'object' && typeof process.versions
if (ENVIRONMENT_IS_NODE) {
// Create as web-worker-like an environment as we can.

// See the parallel code in shell.js.
#if EXPORT_ES6 && ENVIRONMENT_MAY_BE_WEB
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm confused about ENVIRONMENT_MAY_BE_WEB.. surely require never works on the web does it?

Where does worker.js use other than below in the ENVIRONMENT_MAY_BE_NODE block?

It we only use require for those two lines below maybe we should replace them with dyanmic imports?

e.g.:

#if EXPORT_ES6
var fs = await import('fs');
var vm = await import('vm');
#else
var fs = require('fs');
var vm = require('vm');
#endif

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah in the main JS ENVIRONMENT_MAY_BE_WEB is only used because there are static imports for when we know that environment is Node.js and no other.

In this case there are no static imports to fall back to, so it should either add them or just use await import like you suggested.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it's confusing. The comment this refers to explains more:

emscripten/src/shell.js

Lines 193 to 196 in 7a14e5b

// `require()` is no-op in an ESM module, use `createRequire()` to construct
// the require()` function. This is only necessary for multi-environment
// builds, `-sENVIRONMENT=node` emits a static import declaration instead.
// TODO: Swap all `require()`'s with `import()`'s?

My understanding of the lines is that in a pure node build we don't emit this anyhow. But in a mixed build we can't do that, and so if we build for node or web but end up running in node, then we get to this location, and need something to put here (since we couldn't put the static thing for a pure node build).

(This is all separate from this PR, of course, as it's preexisting.)

Copy link
Member Author

Choose a reason for hiding this comment

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

It turns out I was wrong, this is not separable. This PR was enough to fix the case of using a package.json file to force ES6 module format, but without such a file node.js uses the suffix to decide what to run. And this PR adds await import, which errors in a non-ES6 module.

To fix that the PR now emits .worker.mjs when EXPORT_ES6. That is a breaking change unfortunately, which may break all users that copy files to a production environment that use pthreads+EXPORT_ES6, but I don't see a way around it? Hopefully not many are using EXPORT_ES6, and probably few are because of issues like this, actually...

Thoughts?

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 if you use EXPORT_ES6 when it makes sense to create worker.mjs. I don't think think its a huge deal that we change this now. Maybe just mention it in the changelog.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, yeah, already added to the changelog.

If this breaking change sgty then let me know if you have any other feedback on the PR, otherwise I think it is done.

const { createRequire } = await import('module');
/** @suppress{duplicate} */
var require = createRequire(import.meta.url);
#endif

var nodeWorkerThreads = require('worker_threads');

var parentPort = nodeWorkerThreads.parentPort;
Expand All @@ -32,7 +39,8 @@ if (ENVIRONMENT_IS_NODE) {
require,
Module,
location: {
href: __filename
// __filename is undefined in ES6 modules
href: typeof __filename !== 'undefined' ? __filename : undefined
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is undefined really valid here? I think it should be import.meta.url instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

undefined was enough to pass the test but I have no idea how this is used or what it is 😄 Thanks, I'll change it to that.

Out of curiosity, is it possible to test that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good question, depends if anything in our JS uses location.href to get script's own address - presumably that's why it was being polyfilled here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I also wonder what are the situations where typeof __filename wouldn't be undefined in ES6 mode.

},
Worker: nodeWorkerThreads.Worker,
importScripts: (f) => vm.runInThisContext(fs.readFileSync(f, 'utf8'), {filename: f}),
Expand Down
15 changes: 13 additions & 2 deletions test/test_other.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,11 +400,16 @@ def test_export_es6_allows_export_in_post_js(self):
src = read_file('a.out.js')
self.assertContained('export{doNothing};', src)

@parameterized({
'': (False,),
'package_json': (True,),
})
@parameterized({
'': ([],),
'pthreads': (['-pthread'],),
# load a worker before startup to check ES6 modules there as well
'pthreads': (['-pthread', '-sPTHREAD_POOL_SIZE=1'],),
})
def test_export_es6(self, args):
def test_export_es6(self, args, package_json):
self.run_process([EMCC, test_file('hello_world.c'), '-sEXPORT_ES6',
'-o', 'hello.mjs'] + args)
# In ES6 mode we use MODULARIZE, so we must instantiate an instance of the
Expand All @@ -413,6 +418,12 @@ def test_export_es6(self, args):
import Hello from "./hello.mjs";
Hello();
''')

if package_json:
# This makes node load all files in the directory as ES6 modules,
# including the worker.js file.
create_file('package.json', '{"type":"module"}')

self.assertContained('hello, world!', self.run_js('runner.mjs'))

def test_emcc_out_file(self):
Expand Down
27 changes: 16 additions & 11 deletions tools/link.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1999,12 +1999,26 @@ def phase_memory_initializer(memfile):
final_js += '.mem.js'


# Unmangle previously mangled `import.meta` and `await import` references in
# both main code and libraries.
# See also: `preprocess` in parseTools.js.
def fix_es6_import_statements(js_file):
if not settings.EXPORT_ES6 or not settings.USE_ES6_IMPORT_META:
return

src = read_file(js_file)
write_file(js_file, src
.replace('EMSCRIPTEN$IMPORT$META', 'import.meta')
.replace('EMSCRIPTEN$AWAIT$IMPORT', 'await import'))

def create_worker_file(input_file, target_dir, output_file):
output_file = os.path.join(target_dir, output_file)
input_file = utils.path_from_root(input_file)
contents = shared.read_and_preprocess(input_file, expand_macros=True)
write_file(output_file, contents)

fix_es6_import_statements(output_file)

# Minify the worker JS file, if JS minification is enabled.
if settings.MINIFY_WHITESPACE:
contents = building.acorn_optimizer(output_file, ['minifyWhitespace'], return_output=True)
Expand Down Expand Up @@ -2045,17 +2059,8 @@ def phase_final_emitting(options, state, target, wasm_target, memfile):
# mode)
final_js = building.closure_compiler(final_js, advanced=False, extra_closure_args=options.closure_args)

# Unmangle previously mangled `import.meta` and `await import` references in
# both main code and libraries.
# See also: `preprocess` in parseTools.js.
if settings.EXPORT_ES6 and settings.USE_ES6_IMPORT_META:
src = read_file(final_js)
final_js += '.esmeta.js'
write_file(final_js, src
.replace('EMSCRIPTEN$IMPORT$META', 'import.meta')
.replace('EMSCRIPTEN$AWAIT$IMPORT', 'await import'))
shared.get_temp_files().note(final_js)
save_intermediate('es6-module')
fix_es6_import_statements(final_js)
save_intermediate('es6-module')

# Apply pre and postjs files
if options.extern_pre_js or options.extern_post_js:
Expand Down