Skip to content

Proxy known handlers back to the main thread #17925

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

Merged
kripken merged 2 commits into from
Nov 4, 2022
Merged

Proxy known handlers back to the main thread #17925

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
096a5de
Proxy known handlers back to the main thread
kleisauke Sep 25, 2022
34bdd28
Merge remote-tracking branch 'upstream/main'
kleisauke Nov 4, 2022
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
32 changes: 26 additions & 6 deletions src/library_pthread.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,12 +283,8 @@ var LibraryPThread = {
// Worker wants to postMessage() to itself to implement setImmediate()
// emulation.
worker.postMessage(d);
#if expectToReceiveOnModule('onAbort')
} else if (cmd === 'onAbort') {
if (Module['onAbort']) {
Module['onAbort'](d['arg']);
}
#endif
} else if (cmd === 'callHandler') {
Module[d['handler']](...d['args']);
Copy link
Collaborator

@sbc100 sbc100 Sep 28, 2022

Choose a reason for hiding this comment

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

What if the handler is not present? (expectToReceiveOnModule just means that it could be present)

Copy link
Collaborator Author

@kleisauke kleisauke Sep 30, 2022

Choose a reason for hiding this comment

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

The Module[d['handler']] check is not necessary here since it's checked below with Module.hasOwnProperty(handler), and likewise for those expectToReceiveOnModule checks. Thus, this handler is called only if it's present in INCOMING_MODULE_JS_API and if it's explicitly set on the module.

} else if (cmd) {
// The received message looks like something that should be handled by this message
// handler, (since there is a e.data.cmd field present), but is not one of the
Expand Down Expand Up @@ -329,9 +325,33 @@ var LibraryPThread = {
assert(wasmModule instanceof WebAssembly.Module, 'WebAssembly Module should have been loaded by now!');
#endif

// When running on a pthread, none of the incoming parameters on the module
// object are present. Proxy known handlers back to the main thread if specified.
var handlers = [];
var knownHandlers = [
#if expectToReceiveOnModule('onExit')
'onExit',
#endif
#if expectToReceiveOnModule('onAbort')
'onAbort',
#endif
#if expectToReceiveOnModule('print')
'print',
#endif
#if expectToReceiveOnModule('printErr')
'printErr',
Copy link
Collaborator

@sbc100 sbc100 Oct 14, 2022

Choose a reason for hiding this comment

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

So print and printErr were previously not proxied? What happened when they were called from the thread previously?

Copy link
Collaborator Author

@kleisauke kleisauke Oct 15, 2022

Choose a reason for hiding this comment

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

Indeed, these module handlers were previously not proxied causing the Module['print']/Module['printErr'] handlers to be skipped (i.e. it's always printed on the terminal) on pthreads. See for example the current implementation of out/err:

var out = Module['print'] || defaultPrint;
var err = Module['printErr'] || defaultPrintErr;

emscripten/src/shell.js

Lines 428 to 429 in 30f50a9

{{{ makeModuleReceiveWithVar('out', 'print', 'defaultPrint', true) }}}
{{{ makeModuleReceiveWithVar('err', 'printErr', 'defaultPrintErr', true) }}}

For simple printf() usage (without using WasmFS), however, this issue did not occur, since almost all syscalls, including fd_write(), are proxied back to the main thread.

emscripten/src/library_syscall.js

Lines 1087 to 1100 in 30f50a9

#if USE_PTHREADS
// Most syscalls need to happen on the main JS thread (e.g. because the
// filesystem is in JS and on that thread). Proxy synchronously to there.
// There are some exceptions, syscalls that we know are ok to just run in
// any thread; those are marked as not being proxied with
// __proxy: false
// A syscall without a return value could perhaps be proxied asynchronously
// instead of synchronously, and marked with
// __proxy: 'async'
// (but essentially all syscalls do have return values).
if (library[x + '__proxy'] === undefined) {
library[x + '__proxy'] = 'sync';
}
#endif

This issue occurred with WasmFS, as that implements stdout/stderr using _emscripten_out/_emscripten_err, which doesn't have this proxy logic.

emscripten/system/lib/wasmfs/special_files.cpp

Lines 110 to 116 in 30f50a9

// Node and worker threads issue in Emscripten:
// https://github.com/emscripten-core/emscripten/issues/14804.
// Issue filed in Node: https://github.com/nodejs/node/issues/40961
// This is confirmed to occur when running with EXIT_RUNTIME and
// PROXY_TO_PTHREAD. This results in only a single console.log statement
// being outputted. The solution for now is to use out() and err() instead.
return writeToJS(buf, len, &_emscripten_out, writeBuffer);

emscripten/src/library.js

Lines 3364 to 3369 in 30f50a9

_emscripten_out: function(str) {
#if ASSERTIONS
assert(typeof str == 'number');
#endif
out(UTF8ToString(str));
},

(+ it would be a performance penalty if these were always proxied to the main thread, as noticed in #17688 (comment))

Copy link
Collaborator Author

@kleisauke kleisauke Oct 15, 2022

Choose a reason for hiding this comment

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

BTW, this bug only occurs when the handler is set outside the module. For example, it did not occur when emitting the handlers via --pre-js, since the pthread worker(s) will import() that.

Copy link
Collaborator

@sbc100 sbc100 Oct 17, 2022

Choose a reason for hiding this comment

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

Can we avoid the needless proxying in the case when the handler is defined in the --pre-js maybe?

Copy link
Collaborator Author

@kleisauke kleisauke Nov 4, 2022

Choose a reason for hiding this comment

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

Can we avoid the needless proxying in the case when the handler is defined in the --pre-js maybe?

I'm not sure this can be done without too much hassle. For example, if someone does this in --pre-js:

if (!Module.hasOwnProperty('print')) {
  Module['print'] = function(x) {
    console.log(x);
  };
}

if (!Module.hasOwnProperty('printErr')) {
  Module['printErr'] = function(x) {
    console.error(x);
  };
}

(i.e. set the default print/printErr module handlers, if it is not already overridden outside the module - commit 5c2d544 might be relevant here)

It implies that Module.hasOwnProperty('print') and Module.hasOwnProperty('printErr') both return true after that.

#endif
];
for (var handler of knownHandlers) {
if (Module.hasOwnProperty(handler)) {
handlers.push(handler);
}
}

// Ask the new worker to load up the Emscripten-compiled page. This is a heavy operation.
worker.postMessage({
'cmd': 'load',
'handlers': handlers,
// If the application main .js file was loaded from a Blob, then it is not possible
// to access the URL of the current script that could be passed to a Web Worker so that
// it could load up the same file. In that case, developer must either deliver the Blob
Expand Down
16 changes: 2 additions & 14 deletions src/preamble.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -455,20 +455,8 @@ function removeRunDependency(id) {
/** @param {string|number=} what */
function abort(what) {
#if expectToReceiveOnModule('onAbort')
#if USE_PTHREADS
// When running on a pthread, none of the incoming parameters on the module
// object are present. The `onAbort` handler only exists on the main thread
// and so we need to proxy the handling of these back to the main thread.
// TODO(sbc): Extend this to all such handlers that can be passed into on
// module creation.
if (ENVIRONMENT_IS_PTHREAD) {
postMessage({ 'cmd': 'onAbort', 'arg': what});
} else
#endif
{
if (Module['onAbort']) {
Module['onAbort'](what);
}
if (Module['onAbort']) {
Module['onAbort'](what);
}
#endif

Expand Down
8 changes: 8 additions & 0 deletions src/worker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,14 @@ self.onmessage = (e) => {
Module['dynamicLibraries'] = e.data.dynamicLibraries;
#endif

// Use `const` here to ensure that the variable is scoped only to
// that iteration, allowing safe reference from a closure.
for (const handler of e.data.handlers) {
Module[handler] = function() {
Copy link
Collaborator

@sbc100 sbc100 Oct 17, 2022

Choose a reason for hiding this comment

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

How about only setting this if handler doesn't already exist as property of the module? If the handler is already defined on the module there should be no need to proxy it right?

Copy link
Collaborator Author

@kleisauke kleisauke Nov 4, 2022

Choose a reason for hiding this comment

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

Those handlers (onExit, onAbort, print and printErr) are only proxied if it's present in INCOMING_MODULE_JS_API and if it's explicitly set on the module. The latter is controlled by this:

emscripten/src/library_pthread.js

Lines 346 to 348 in 096a5de

if (Module.hasOwnProperty(handler)) {
handlers.push(handler);
}

So, in most cases, e.data.handlers is an empty array. Perhaps I should clarify this with a unit test?

postMessage({ cmd: 'callHandler', handler, args: [...arguments] });
}
}

{{{ makeAsmImportsAccessInPthread('wasmMemory') }}} = e.data.wasmMemory;

#if LOAD_SOURCE_MAP
Expand Down
31 changes: 31 additions & 0 deletions test/test_other.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5598,6 +5598,37 @@ def test_require_modularize(self):
self.assertFalse(output.stderr)
self.assertEqual(output.stdout, 'hello, world!\nhello, world!\n')

@node_pthreads
def test_pthread_print_override_modularize(self):
self.set_setting('EXPORT_NAME', 'Test')
self.set_setting('PROXY_TO_PTHREAD')
self.set_setting('EXIT_RUNTIME')
self.set_setting('MODULARIZE')
create_file('main.c', '''
#include <emscripten/console.h>

int main() {
_emscripten_out("hello, world!");
return 0;
}
''')
create_file('main.js', '''
const Test = require('./test.js');

async function main() {
await Test({
// world -> earth
print: (text) => console.log(text.replace('world', 'earth'))
});
}
main();
''')

self.emcc('main.c', output_filename='test.js')
output = self.run_js('main.js')
self.assertNotContained('hello, world!', output)
self.assertContained('hello, earth!', output)

def test_define_modularize(self):
self.run_process([EMCC, test_file('hello_world.c'), '-sMODULARIZE', '-sASSERTIONS=0'])
src = 'var module = 0; ' + read_file('a.out.js')
Expand Down