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

Proxy known handlers back to the main thread #17925

Merged
merged 2 commits into from
Nov 4, 2022

Conversation

kleisauke
Copy link
Collaborator

@kleisauke kleisauke commented Sep 25, 2022

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.

This simplifies the previous onAbort pthread logic for a more generic solution that also supports the onExit, print and printErr handlers that can be overridden on the incoming module.

See: #17688.

@kripken
Copy link
Member

kripken commented Sep 26, 2022

Reading this code, I'm not sure how it works 😄 it seems like it renames print => out etc., which seems correct - we moved to those new names at some point, and I guess forgot to update print/printErr. But how does that relate to proxying?

@kleisauke
Copy link
Collaborator Author

The reason for the print => out and printErr => err rename was to fix the test failures when linking with -sASSERTIONS on this PR. In short, Object.defineProperty here:

function unexportedRuntimeSymbol(sym) {
if (!Object.getOwnPropertyDescriptor(Module, sym)) {
Object.defineProperty(Module, sym, {
configurable: true,
get: function() {
var msg = "'" + sym + "' was not exported. add it to EXPORTED_RUNTIME_METHODS (see the FAQ)";
if (isExportedByForceFilesystem(sym)) {
msg += '. Alternatively, forcing filesystem support (-sFORCE_FILESYSTEM) can export this for you';
}
abort(msg);
}
});
}
}

Causes conflicts with the Module.hasOwnProperty check here (on this PR):

if (Module.hasOwnProperty(handler)) {

Since it overwrites Module['print'] and Module['printErr'] with those assertions.

I just split this into a separate commit with f4bf880, but this should probably be included in a seperate PR.

}
#endif
} else if (cmd === 'callHandler') {
Module[d['handler']](...d['args']);
Copy link
Collaborator

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

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.

print: (text) => console.log(text.replace('world', 'earth'))
});
}
main();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this needs need to be using MODULARIZE/EXPORT_NAME? Could it not just use onRuntimeInitialized to run this code?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this bug now also effect non-wasmfs? Any not just write this test without WASMFS at all?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Simplified the test with commit 2be3a53. Note that this bug only occurs when the handler is set outside the module.

@kripken
Copy link
Member

kripken commented Sep 28, 2022

@kleisauke

[..] I just split this into a separate commit with f4bf880, but this should probably be included in a seperate PR.

I see, thanks. Makes sense. Yes, please let's do that part in a separate PR - I am a little concerned about backwards compatibility there, as it seems like it might be a noticeable change for some?

@kleisauke
Copy link
Collaborator Author

I see, thanks. Makes sense. Yes, please let's do that part in a separate PR - I am a little concerned about backwards compatibility there, as it seems like it might be a noticeable change for some?

I just split this change to PR #17955 in a backwards compatible manner.

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.

This simplifies the previous `onAbort` pthread logic for a more
generic solution that also supports the `onExit`, `print` and
`printErr` handlers that can be overridden on the incoming module.

See: emscripten-core#17688.
@kleisauke
Copy link
Collaborator Author

Rebased now that PR #17955 has been merged.

'print',
#endif
#if expectToReceiveOnModule('printErr')
'printErr',
Copy link
Collaborator

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

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.

#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.

// 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

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

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

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.

// 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

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

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:

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?

@kripken kripken merged commit 97b352e into emscripten-core:main Nov 4, 2022
@kleisauke kleisauke deleted the proxy-handlers branch November 8, 2022 18:30
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.

3 participants