Disallow blocking on the main thread by default #9579

kripken · 2019-10-05T00:16:52Z

If ALLOW_BLOCKING_ON_MAIN_THREAD is not set by the user, throw an error if pthread_join or pthread_cond_wait are called on the main browser thread, which is bad for responsiveness and at risk of deadlocks. If the error is shown it points to our docs, which explain the issue and mention the flag that can allow blocking at your own risk.

dschuff · 2019-10-07T16:13:53Z

Thinking about this a little more, it might makes sense to get some more eyes/discussion on this issue before we land it, since it is potentially a fairly-disruptive change. Also it might make sense to have our hypothesized try_join feature available before we ask everyone to reconsider how they are managing their threads.

sbc100 · 2019-10-07T16:17:49Z

try_join is real (albeit non-portable) thing already: https://linux.die.net/man/3/pthread_tryjoin_np

sbc100

I think this is a great idea.

I'm surprised how many tests rely on ALLOW_BLOCKING_ON_MAIN_THREAD.

tests/pthread/main_thread_join.cpp

dschuff

Other than the part about ALLOW_BLOCKING_ON_MAIN THREAD, there's probably no reason to wait on this documentation though.

It's probably even worth expanding a bit more, to include e.g. to distinguish between the Module thread (or main runtime thread, or ??) vs the browser main thread?

site/source/docs/porting/pthreads.rst

VirtualTim · 2019-10-08T01:46:27Z

src/library_pthread.js

@@ -790,7 +790,16 @@ var LibraryPThread = {
    if (canceled == 2) throw 'Canceled!';
  },

-  {{{ USE_LSAN ? 'emscripten_builtin_' : '' }}}pthread_join__deps: ['_cleanup_thread', '_pthread_testcancel_js', 'emscripten_main_thread_process_queued_calls', 'emscripten_futex_wait'],
+  emscripten_block_on_main_thread: function() {


Maybe a nit pick, but I don't really like the name of this function. To me it sounds like this blocks on the main thread, when it actually either aborts of does nothing.

I agree emscripten_block_on_main_thread is not the best, but I couldn't find a better one - ideas?

I'm not sure. The only thing I can think of would be emscripten_block_on_main_thread_or_die (in the same vain as functions like malloc_or_die), but I'm not sure if that's actually any better. Either way it's not that big of a deal in my opinion.

kripken · 2019-10-14T21:20:31Z

Thanks for the feedback, should be addressed in those commits. In particular this adds pthread_tryjoin_fd + docs, and adds docs on the browser vs application main thread etc.

RReverser · 2019-10-25T12:30:48Z

I was actually wondering - rather than disallowing blocking on the main thread and giving an escape hatch in form of a setting that still allows it, could we transform pthread_join and friends via Asyncify instead? This way the main browser thread wouldn't really be blocked, but the code would be still backwards-compatible and properly wait for thread to finish execution.

Such Asyncify transform should also be perfectly compatible with the new Atomics.waitAsync replacement of Atomics.wait which is intended specifically for such usage (asynchronous "blocking").

UPD: I've read a bit further and see you already mentioned similar option in the docs 🤦‍♂️

It really seems preferable to do this automatically IMO both from developer's perspective (no change to the code) and from browser/user perspective (no escape hatch to do real blocking).

sbc100 · 2019-10-25T17:36:22Z

@RReverser I agree that is good solution, but I think it should certainly be opt-in in it, at least by adding -s ASYNCIFY. ASYNCIFY adds a overhead and complexity and I think we want developers to opt into using and know what they are signing up for.

Mentioning this solution in the doc seems fine to me.

RReverser · 2019-10-25T17:44:22Z

ASYNCIFY adds a overhead and complexity

I'd argue not as much overhead as blocking the main thread. That is, I agree it should be an opt-in, but I'm suggesting it as a potentially better fallback instead of ALLOW_BLOCKING_ON_MAIN_THREAD, so that either way user doesn't end up blocking the actual main thread.

Mentioning this solution in the doc seems fine to me.

For start it would need to be implemented (AFAIK it's not yet).

kripken · 2019-10-28T19:52:39Z

@RReverser interesting idea, yeah, Asyncify integration sounds worth looking into.

A slightly-weird idea might even be to create two wasm files, one with Asyncify and one without, and the Asyncify one would only be used on the main thread where it is needed... anyhow, something to think about in followups.

kripken · 2019-10-28T20:11:47Z

@surma Sounds good, updated to just warn by default. We can consider flipping the flag added here to make it an error later on.

site/source/docs/porting/pthreads.rst

tests/pthread/main_thread_join.cpp

gabrielcuvillier · 2019-11-27T15:24:41Z

@kripken , actually, it looks like the warning only appears when compiling in debug mode (that is, with ASSERTIONS=1) and not in any other modes (when optimizations are set, ASSERTIONS=0).

Maybe the intent would have been to display the message in all cases, otherwise people might never see it !
Reason is simple: most developers will not be really aware of the existence of the ALLOW_BLOCKING_ON_MAIN_THREAD flag because it is unset by default (for good reason of course,to prevent annoying breaks), BUT it is possible developers are never working in debug mode when working with Emscripten - in particular if it is a project that have already been worked out, or simply by developer habit (my case: I only use debugging when compiling on Native platform. On the Web, -O0 -g3 -s ASYNCIFY=1 binaries are simply too big !).

So this new flag might not bring any new information :/

I am just saying this because I was wondering why I did not see the warning, even though I knew I did dangerous things on purpose :)

Just a suggestion

kripken · 2019-11-27T17:55:24Z

Hmm, that seems odd - the code is

  emscripten_check_blocking_allowed: function() {
#if ASSERTIONS
    assert(ENVIRONMENT_IS_WEB);
    warnOnce('Blocking on the main thread is very dangerous, see https://emscripten.org/docs/porting/pthreads.html#blocking-on-the-main-browser-thread');
#endif
#if !ALLOW_BLOCKING_ON_MAIN_THREAD
    abort('Blocking on the main thread is not allowed by default. See https://emscripten.org/docs/porting/pthreads.html#blocking-on-the-main-browser-thread');
#endif
  },

That should abort in a release build. Is that code not even reached for you?

kripken · 2019-11-27T17:55:54Z

(also that warnOnce looks redundant, maybe we should remove it...)

gabrielcuvillier · 2019-11-27T20:43:37Z

No, for some reason that is not reached! So maybe there is a bug (?)

In my code, ASSERTIONS are turned off (due to -O2/-Os build), and ALLOW_BLOCKING_ON_MAIN_THREADS is not set (and so, according to settings.js, it is set to "1").
When doing some pthread_join from browser thread, I don't have any warning message nor an abort. It is only if I add ALLOW_BLOCKING_ON_MAIN_THREADS=0 that I have the abort.

But this behavior looks like to be consistent with the code. If ASSERTIONS=0, then no warning messages, if ALLOW_BLOCKING_ON_MAIN_THREAD=1, then no abort.

Or I am missing something ?

kripken · 2019-11-27T20:46:26Z

Oh, no, that is the intended behavior: by default we do allow blocking (we decided it was too disruptive to disallow by default). And also by default we do not show extra warnings when ASSERTIONS are off (in production builds people don't want those, usually).

gabrielcuvillier · 2019-11-27T20:57:23Z

Ok, got it. Then that's what my suggestion was about: to enable the warning even on production builds (so that developers not working in Debug have an opportunity to see it at least once), but still letting them disabling the warning if they judge it is OK for their use case. So I imagined having the default value of the flag to be something like =2 (show the warning in all cases, even on production, but do not abort).

Well, nothing dramatic, I can live without this :)

kripken · 2019-11-27T21:14:26Z

Maybe we should have different levels of ASSERTIONS, yeah... but in general, I think it's a good idea to test with ASSERTIONS builds, even if not with actual debug builds. Those builds are almost 100% optimized, but may show useful warnings and errors. Perhaps we should also document this some more...

gabrielcuvillier · 2019-11-27T21:45:45Z

Ok, that's right. I'll now enable emscripten ASSERTIONS on non-debug builds, only disabling them for production. Definitively looks like to be good practice.

Add a new ALLOW_BLOCKING_ON_MAIN_THREAD option, set to 1 by default. If unset, it will throw when blocking on the main thread. If 1 as in the default case, we warn in the console about possible issues and link to the docs about that. This ignores things like mutexes which are usually brief, and checks for pthread_join or pthread_cond_wait operations, which tend to be longer and run the risk of Web-specific proxying-related deadlocks. Add pthread_tryjoin_np which does a non-blocking join, which always works on the main thread. Improve the docs in this area.

kripken added 15 commits October 2, 2019 09:27

wip [ci skip] need link to good length docs page

0263dae

Merge remote-tracking branch 'origin/incoming' into pj

b8f47c0

[ci skip]

9197b79

Merge remote-tracking branch 'origin/incoming' into pj

ac6d56c

wip [ci skip]

37d78fb

[ci skip]

68e1a88

test

27b72cc

more tests

2e5e63e

more

905e668

fix

b24df05

wip [ci skip]

37f275e

fix

12c1915

Merge remote-tracking branch 'origin/incoming' into pj

f254984

text

479058c

fix

b366dc8

kripken requested a review from dschuff October 5, 2019 00:16

fix typo

5ae5bbe

sbc100 reviewed Oct 7, 2019

View reviewed changes

tests/pthread/main_thread_join.cpp Outdated Show resolved Hide resolved

dschuff reviewed Oct 7, 2019

View reviewed changes

site/source/docs/porting/pthreads.rst Outdated Show resolved Hide resolved

site/source/docs/porting/pthreads.rst Outdated Show resolved Hide resolved

site/source/docs/porting/pthreads.rst Show resolved Hide resolved

VirtualTim reviewed Oct 8, 2019

View reviewed changes

kripken added 5 commits October 14, 2019 13:36

Merge remote-tracking branch 'origin/incoming' into pj

586cc93

feedback

b21d2c7

docs

9d8c79b

support pthread_tryjoin_np

7a0dc20

document pthread_tryjoin_np

ec5c4ad

Merge remote-tracking branch 'origin/incoming' into pj

6247193

sbc100 mentioned this pull request Oct 18, 2019

thread function don't excute #9667

Closed

kripken added 2 commits October 28, 2019 12:53

Merge remote-tracking branch 'origin/incoming' into pj

18ffe28

just warn for now

fa2882f

flake8

3158218

dschuff approved these changes Oct 30, 2019

View reviewed changes

dschuff reviewed Oct 30, 2019

View reviewed changes

tests/pthread/main_thread_join.cpp Show resolved Hide resolved

kripken added 6 commits October 30, 2019 13:20

Merge remote-tracking branch 'origin/incoming' into pj

a59669f

feedback [ci skip]

a0ebf2f

tryjoin without a pool

b3bfc83

Make sure we test failed tryjoins

345e0e7

text nit [ci skip]

2bc7ce4

more

24318b2

kripken merged commit 3dc4011 into incoming Oct 31, 2019

kripken deleted the pj branch October 31, 2019 22:13

RReverser mentioned this pull request Nov 27, 2019

Make use of Asyncify for pthread_join #9910

Open

sbc100 mentioned this pull request Jan 13, 2022

Warn before busy waiting on futex #15957

Open

sbc100 mentioned this pull request Feb 28, 2023

Make emscripten_check_blocking_allowed() no-op in release builds #18871

Merged

Disallow blocking on the main thread by default #9579

Disallow blocking on the main thread by default #9579

Uh oh!

Conversation

kripken commented Oct 5, 2019

Uh oh!

dschuff commented Oct 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbc100 commented Oct 7, 2019

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dschuff left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

VirtualTim Oct 8, 2019

Choose a reason for hiding this comment

Uh oh!

kripken Oct 14, 2019

Choose a reason for hiding this comment

Uh oh!

VirtualTim Oct 15, 2019

Choose a reason for hiding this comment

Uh oh!

kripken commented Oct 14, 2019

Uh oh!

RReverser commented Oct 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbc100 commented Oct 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RReverser commented Oct 25, 2019

Uh oh!

kripken commented Oct 28, 2019

Uh oh!

kripken commented Oct 28, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gabrielcuvillier commented Nov 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kripken commented Nov 27, 2019

Uh oh!

kripken commented Nov 27, 2019

Uh oh!

gabrielcuvillier commented Nov 27, 2019

Uh oh!

kripken commented Nov 27, 2019

Uh oh!

gabrielcuvillier commented Nov 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kripken commented Nov 27, 2019

Uh oh!

gabrielcuvillier commented Nov 27, 2019

Uh oh!

Uh oh!

dschuff commented Oct 7, 2019 •

edited

Loading

RReverser commented Oct 25, 2019 •

edited

Loading

sbc100 commented Oct 25, 2019 •

edited

Loading

gabrielcuvillier commented Nov 27, 2019 •

edited

Loading

gabrielcuvillier commented Nov 27, 2019 •

edited

Loading