Remove duplicate threadExit code in worker.js

[sbc100]

The code here was basically identical to that in PThread.threadExit

[kripken]

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

[sbc100]

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?

[sbc100]

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

[kripken]

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.

[sbc100]

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.

[VirtualTim]

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);
});

[sbc100]

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?

[VirtualTim]

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.

[sbc100]

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.