Add centralized handling for calling user callbacks #13596

sbc100 · 2021-03-04T01:35:35Z

This change also changes the way outstanding works keeps the runtime
alive.

We now use a counter rather than a single noExitRuntime variable so
that once outstanding works is complete the thread can be shutdown
if needed.

The specific need for this comes the use case of running fetch thread
and wanting to exit the thread once the fetch completes. Without
this change pthread_exit from a fetch callback generated an
uncaught unwind message on the console.

Also if one wants the fetch thread to become joinable there was no
way other than calling pthread_exit which has no parallel in the C++
threads world and has issues around calling C++ destructors on the
stack. With this solution the thread will exit automatically when returning
from the fetch callback.

src/library.js

src/Fetch.js

src/library.js

src/library_browser.js

kripken

How about adding a specific test for the new keepalive functions, that is, without fetch or another high-level API?

src/library_pthread.js

src/postamble.js

tests/emscripten_main_loop_and_blocker.cpp

tests/fetch/from_thread.cpp

dschuff

This LGTM overall, although I'm not as familiar with some of these details. I agree with @kripken that a more targeted test would be good, although it could be done in a followup since we want to start a release cut soon.

sbc100 · 2021-03-05T21:18:54Z

Actually I decided to just go for {{{ }}} approach after all. This means absolutely minimal impact on MINIMAL_RUNTIME builds.

src/Fetch.js

kripken · 2021-03-05T22:29:51Z

src/library_browser.js

+    if (!Browser.mainLoop.timingSet) {
+      {{{ runtimeKeepalivePush(); }}}
+      Browser.mainLoop.timingSet = true;
+    }


this change looks non-obvious. why unset this here, when it wasn't before? and why does that keep the runtime alive? where is the corresponding pop?

This function (emscripten_set_main_loop_timing
) can be used to set the timing of an existing main loop. If the timing already set we don't want to repeated call runtimeKeepalivePush every time the timing is adjusted.

See 'main loop exiting..' below in setMainLoop for where the loops exits ad calls Pop

I still find this confusing. Perhaps some comments could help, if I'm missing something.

It seems like we shouldn't care if the timing was set, but rather whether there was already a main loop running? That is, I'd expect to see a bool that says "I have pushed a runtime keepalive", or "already running" or such?

Yes that is exactly what this is supposed to mean. I think after I investigated this I determined that a has timing been set was the same things as "is running".. but you are correct I can come up with a better name and some comments.

kripken · 2021-03-05T22:30:50Z

src/library_sockfs.js

  $_setNetworkCallback: function(event, userData, callback) {
    function _callback(data) {
+      {{{ runtimeKeepalivePop(); }}}
+      if (!callback) return;


why wasn't this needed before, and why is it now?

src/parseTools.js

tests/test_browser.py

src/library_browser.js

kripken

lgtm % last comment

tests/emscripten_main_loop_and_blocker.cpp

This change also changes the way outstanding works keeps the runtime alive. We now use a counter rather than a single noExitRuntime variable so that once outstanding works is complete the thread can be shutdown if needed. The specific need for this comes the use case of running fetch thread and wanting to exit the thread once the fetch completes. Without this change `pthread_exit` from a fetch callback generated an `uncaught unwind` message on the console. In addition, it doesn't always make sense to call `pthread_exit`, (e.g. in cases where you want C++ descrutors on the stack to be called, or in cases where C++ threads are being used and pthread_exit` does not exist, or at least doesn't makes sense). With this solution the thread will exit automatically when returning from the fetch callback, no explict thread exit call needed.

sbc100 · 2021-03-12T18:06:09Z

Added a new specific test for these new functions .. PTAL

kripken

new test lgtm too

I believe this was an oversight when this code was refactored as part of #13596.

sbc100 force-pushed the runtime_keepalive_slim branch 2 times, most recently from 61cdcb3 to 6b26bdc Compare March 4, 2021 02:22

sbc100 mentioned this pull request Mar 4, 2021

Add new API for keeping runtime alive #13335

Closed

sbc100 force-pushed the runtime_keepalive_slim branch from 6b26bdc to 6b960dc Compare March 4, 2021 02:56

sbc100 requested review from kripken and juj and removed request for kripken March 4, 2021 03:07

sbc100 force-pushed the runtime_keepalive_slim branch from 6b960dc to c4a4415 Compare March 4, 2021 03:12

sbc100 requested a review from kripken March 4, 2021 03:12

sbc100 force-pushed the runtime_keepalive_slim branch from c4a4415 to 6f11253 Compare March 4, 2021 04:58

kripken reviewed Mar 4, 2021

View reviewed changes

src/library.js Show resolved Hide resolved

src/Fetch.js Show resolved Hide resolved

sbc100 force-pushed the runtime_keepalive_slim branch from 6f11253 to 9732171 Compare March 4, 2021 22:40

sbc100 requested a review from dschuff March 5, 2021 17:46