Skip to content

Fix embind failing in workers when STACK_OVERFLOW_CHECK is enabled #12366

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
merged 3 commits into from
Oct 5, 2020
Merged

Fix embind failing in workers when STACK_OVERFLOW_CHECK is enabled #12366

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
93a78a0
Fix embind failing in workers when STACK_OVERFLOW_CHECK is enabled by…
tklajnscek Sep 29, 2020
c699bcc
Reused an existing test instead of adding a new test (removed pthread…
tklajnscek Oct 2, 2020
95e5169
Fixed test
tklajnscek Oct 2, 2020
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
4 changes: 0 additions & 4 deletions src/library_pthread.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,10 +94,6 @@ var LibraryPThread = {
},
initWorker: function() {
PThread.initShared();
#if EMBIND
// Embind must initialize itself on all threads, as it generates support JS.
Module['___embind_register_native_and_builtin_types']();
#endif // EMBIND
#if MODULARIZE
// The promise resolve function typically gets called as part of the execution
// of the Module `run`. The workers/pthreads don't execute `run` here, they
Expand Down
12 changes: 12 additions & 0 deletions src/worker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@
var threadInfoStruct = 0; // Info area for this thread in Emscripten HEAP (shared). If zero, this worker is not currently hosting an executing pthread.
var selfThreadId = 0; // The ID of this thread. 0 if not hosting a pthread.
var parentThreadId = 0; // The ID of the parent pthread that launched this thread.
#if EMBIND
var initializedJS = false; // Guard variable for one-time init of the JS state (currently only embind types registration)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is wrong with re-initializing the state at the start of each thread?

It seems cleaner to me to clear the state when the thread exits an re-create it on thread start, just like we would do for any per-thread state that lives in the linear memory.

Maybe I'm missing something but preserving JS state and then re-using it on another thread seem, in the general case, wrong.

Copy link
Member

@kripken kripken Oct 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

We could maybe add a mechanism to "clear" the embind data, and then when a pthread exits call that, then call init when one is started. That would be more complex though - in particular I'm not sure how hard the "clear" would be to do. But I agree if it's practical it would be cleaner, as you say.

(Regardless this PR doesn't change this property - the JS was always initialized once and then the Worker could be reused for more pthreads. This just moves the init to the right place.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems cleaner to me to clear the state when the thread exits an re-create it on thread start, just like we would do for any per-thread state that lives in the linear memory.

The weirdness here comes from the fact that ___embind_register_native_and_builtin_types is a native function, but it has JS side-effects so the usual "crt is inited on the main thread and then shared directly via SAB with the workers" doesn't apply because the JS side-effects don't get shared.

I disagree that it needs clearing since it's analogous to one-time-crt-init and the JS legitimately lives on for reuse. I think it makes it extra error prone if we clear the embind specific bits and init again on each run as we're just introducing another thing that can fail (the clear), especially when someone doing changes might not be aware of both.

If you really want the clear/init on every run then I'd suggest making it a very formal thing, much like the ATINIT/ATEXIT, but for the workers. That way it's not something tied to this specific case.

#endif

var Module = {};

Expand Down Expand Up @@ -178,6 +181,15 @@ this.onmessage = function(e) {
Module['PThread'].receiveObjectTransfer(e.data);
Module['PThread'].setThreadStatus(Module['_pthread_self'](), 1/*EM_THREAD_STATUS_RUNNING*/);

#if EMBIND
// Embind must initialize itself on all threads, as it generates support JS.
// We only do this once per worker since they get reused
if (!initializedJS) {
Module['___embind_register_native_and_builtin_types']();
initializedJS = true;
}
#endif // EMBIND

try {
// pthread entry points are always of signature 'void *ThreadMain(void *arg)'
// Native codebases sometimes spawn threads with other thread entry point signatures,
Expand Down
5 changes: 5 additions & 0 deletions tests/test_core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8176,6 +8176,11 @@ def test():
self.emcc_args += ['-DPOOL']
test()

print('with embind and stack overflow checks (see #12356)')
self.set_setting('STACK_OVERFLOW_CHECK', 2)
self.emcc_args += ['--bind']
test()

@node_pthreads
def test_pthread_exceptions(self):
self.set_setting('PTHREAD_POOL_SIZE', '2')
Expand Down