Skip to content

Remove duplicate threadExit code in worker.js #12931

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 1 commit into from
Dec 1, 2020
Merged

Remove duplicate threadExit code in worker.js #12931

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 7 additions & 9 deletions src/worker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,24 +203,22 @@ this.onmessage = function(e) {
if (ex === 'Canceled!') {
Module['PThread'].threadCancel();
} else if (ex != 'unwind') {
#if MINIMAL_RUNTIME
// ExitStatus not present in MINIMAL_RUNTIME
Atomics.store(Module['HEAPU32'], (threadInfoStruct + {{{ C_STRUCTS.pthread.threadExitCode }}} ) >> 2, -2 /*A custom entry specific to Emscripten denoting that the thread crashed.*/);
#else
Atomics.store(Module['HEAPU32'], (threadInfoStruct + {{{ C_STRUCTS.pthread.threadExitCode }}} ) >> 2, (ex instanceof Module['ExitStatus']) ? ex.status : -2 /*A custom entry specific to Emscripten denoting that the thread crashed.*/);
#endif

Atomics.store(Module['HEAPU32'], (threadInfoStruct + {{{ C_STRUCTS.pthread.threadStatus }}} ) >> 2, 1); // Mark the thread as no longer running.
#if ASSERTIONS
// FIXME(sbc): Figure out if this is still needed or useful. Its not
// clear to me how this check could ever fail. In order to get into
// this try/catch block at all we have already called bunch of
// functions on `Module`.. why is this one special?
if (typeof(Module['_emscripten_futex_wake']) !== "function") {
err("Thread Initialisation failed.");
throw ex;
}
Copy link
Member

Choose a reason for hiding this comment

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

I believe this assertion was useful. It showed a better error than would otherwise appear if thread initialization fails. May be worth keeping it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm trying to figure out if these still makes sense.

I think you changed the way this works in #9569.. I just can't see how its possible this wouldn't exist.

Do you know why this function (_emscripten_futex_wake) was selected in #8481? And what condition exactly its trying to check for? We call many functions in the Module during this exceptions handler... so we should to check that each of them exists first?

Maybe @VirtualTim can help here... what do you think Tim? Can you remember how this might not be a function? Do we need to wory about anything other Module functions that we are calling?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added the assert back with a comment and a FIXME. Hopefully we can just remove this.. but I'm not sure.

Copy link
Member

Choose a reason for hiding this comment

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

I think any function would be ok. It's just that if the pthread fails to start up, that is, if it crashes before doing the importScripts, then I think it will fail to get the exports added to the global scope. That function is just an example export.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But this catch block is attached to a try that happens after importScripts. The only thing in the try is basically the running of the main entry point. The module is already loaded before the try IIUC.

Copy link
Collaborator

@VirtualTim VirtualTim Dec 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The issue I was having was that a thread could throw during initialisation, and when that occurred the thread would signal to the other thread to resume using this line:
{{{ makeAsmGlobalAccessInPthread('_emscripten_futex_wake') }}}(threadInfoStruct + 0 /*C_STRUCTS.pthread.threadStatus*/, 0x7FFFFFFF/*INT_MAX*/); // Wake all threads waiting on this thread to finish.

However if it threw during initialisation then _emscripten_futex_wake may not be defined, resulting in an exception during exception handling. (see #8481)

But that code has since changed, and it doesn't look like threads are being signalled in the same way. I don't think think this needed any more.

The simplest way I found to test this issue was by having a program just do

std::async(std::launch::async, [&]{
  exit(-1);
});

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm confused because this catch block does not catch errors that occur during initialization of the Module object itself, it catches errors that occur in the main thread entry point..

By the time the try block is started the Module already exists and is initialized. Are you talking bout the case where the Module actually is created but doesn't export the _emscripten_futex_wake symbol?

If the Module itself failed to be initilized I think the failure would happen way earlier and we could not get to this try at all.

We call several exports on the Module before entering this try right?

Copy link
Collaborator

@VirtualTim VirtualTim Dec 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

From what I remember, Module['dynCall_ii'](e.data.start_routine, e.data.arg); could throw, and if if did _emscripten_futex_wake would be called, which under some circumstances may not be defined. So the code was throwing an exception during the exception handling, resulting in the original exception being lost, and Module._emscripten_futex_wake is not a function being thrown instead.

But this was nearly two years ago, and that code has changed a lot.

emscripten/src/worker.js

Lines 191 to 206 in a51cda8

var result = Module['dynCall_ii'](e.data.start_routine, e.data.arg);
#if STACK_OVERFLOW_CHECK
{{{ makeAsmGlobalAccessInPthread('checkStackCookie') }}}();
#endif
} catch(e) {
if (e === 'Canceled!') {
PThread.threadCancel();
return;
} else if (e === 'SimulateInfiniteLoop' || e === 'pthread_exit') {
return;
} else {
Atomics.store(HEAPU32, (threadInfoStruct + 4 /*C_STRUCTS.pthread.threadExitCode*/ ) >> 2, (e instanceof {{{ makeAsmGlobalAccessInPthread('ExitStatus') }}}) ? e.status : -2 /*A custom entry specific to Emscripten denoting that the thread crashed.*/);
Atomics.store(HEAPU32, (threadInfoStruct + 0 /*C_STRUCTS.pthread.threadStatus*/ ) >> 2, 1); // Mark the thread as no longer running.
{{{ makeAsmGlobalAccessInPthread('_emscripten_futex_wake') }}}(threadInfoStruct + 0 /*C_STRUCTS.pthread.threadStatus*/, 0x7FFFFFFF/*INT_MAX*/); // Wake all threads waiting on this thread to finish.

Copy link
Collaborator Author

@sbc100 sbc100 Dec 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

As far as I can tell if Module['dynCall_ii'] exists then so should Module['_emscripten_futex_wake']... I just don't see how one method could exist on the module but not the other.

Both these methods are exports in the same way.

I think the only way this could happen is if the module simply didn't contain _emscripten_futex_wake at all.. so maybe thats it? Hopefully if that was the case we should have seen a linker failure, and not a runtime failure.. but I'll confirm that before making any more changes to this code.

#endif
Module['_emscripten_futex_wake'](threadInfoStruct + {{{ C_STRUCTS.pthread.threadStatus }}}, 0x7FFFFFFF/*INT_MAX*/); // Wake all threads waiting on this thread to finish.
#if MINIMAL_RUNTIME
// ExitStatus not present in MINIMAL_RUNTIME
Module['PThread'].threadExit(-2);
throw ex; // ExitStatus not present in MINIMAL_RUNTIME
#else
Module['PThread'].threadExit((ex instanceof Module['ExitStatus']) ? ex.status : -2);
if (!(ex instanceof Module['ExitStatus'])) throw ex;
#endif
#if ASSERTIONS
Expand Down