Set emscripten_is_main_browser_thread based on ENVIRONMENT_IS_WEB

[sbc100]

The this is more accurate that !ENVIRONMENT_IS_WORKER, and holds
true, for example, under node where there is no main browser thread,
only a main runtime thread.

[sbc100]

This is also consistent with other places in the JS code where ENVIRONMENT_IS_WEB is used to signal "is main browser thread". e.g.:

emscripten/src/library_pthread.js

Lines 847 to 848 in aca650f

	// We can do a normal blocking wait anywhere but on the main browser thread.
	if (!ENVIRONMENT_IS_WEB) {

[kleisauke]

LGTM, thanks for taking care of this! I left a small suggestion inline. Also, the comment here needs also an update (i.e. !ENVIRONMENT_IS_WORKER -> ENVIRONMENT_IS_WEB):

emscripten/system/lib/pthread/emscripten_thread_state.S

Lines 50 to 51 in aca650f

	# Semantically the same as testing "!ENVIRONMENT_IS_WORKER" in JS
	.globl emscripten_is_main_browser_thread

[kleisauke]

Suggested change

	return typeof importScripts === 'undefined';
	return typeof window === 'object';

[sbc100]

Why is this better? This can go in a separate change I think. do you feel like uploading one?

[kleisauke]

IIUC, when not doing this, emscripten_is_main_browser_thread will still return true on Node when linking with -sMINIMAL_RUNTIME. For example, notice how currently ENVIRONMENT_IS_WEB and ENVIRONMENT_IS_WORKER are auto-detected:

emscripten/src/shell.js

Lines 95 to 97 in 0884624

	// Attempt to auto-detect the environment
	var ENVIRONMENT_IS_WEB = typeof window === 'object';
	var ENVIRONMENT_IS_WORKER = typeof importScripts === 'function';

(note that this comment wasn't made on the actual change, but on the MINIMAL_RUNTIME guard)

[kleisauke]

With the test program from #15503 (comment), I see this (on the main branch):

$ emcc test.c -o test.js
$ node test.js
emscripten_is_main_runtime_thread() = 1
emscripten_is_main_browser_thread() = 0
$ emcc -sMINIMAL_RUNTIME test.c -o test-minimal-runtime.js
$ node test-minimal-runtime.js
emscripten_is_main_runtime_thread() = 1
emscripten_is_main_browser_thread() = 1

We could also just remove this minimal runtime guard in favor of ENVIRONMENT_IS_WEB, since that should already be defined, see:

emscripten/src/shell_minimal.js

Line 45 in 9297133

var ENVIRONMENT_IS_WEB = !ENVIRONMENT_IS_NODE;

[juj]

In my mental model emscripten_is_main_browser_thread() should return true in Node.js main VM thread, otherwise a lot of the threading subsystem would be broken(?)

[kripken]

Lgtm although I worry about unintended consequences with code that relied on the old / less accurate logic...

[sbc100]

Lgtm although I worry about unintended consequences with code that relied on the old / less accurate logic...

Good point. I think its at least worth a mention in the changelog.

[sbc100]

Added a changelog entry...

[juj]

emscripten_is_main_browser_thread() should certainly return true if run on the main node.js thread context? It is unfortunate that the function has the name "browser" in it, but its intent is to signify the main thread context of the JS VM. I'd expect this kind of change would break a number of things on node.js threading?

[sbc100]

None of our tests broke though, and we do a lot of thread testing under node (we run the whole posixtest suite).

Perhaps stuff would have broken if I hadn't landed #15503 before hand.

Can you explain a little more why you think we want emscripten_is_main_browser_thread to return true on the main node VM thread? As far as I can tell emscripten_is_main_browser_thread is mostly used to mean "do I need to avoid atomics.wait", and this is not true on the main node thread.

Actually it looks like since #15503 only has two places it gets called from: system/lib/fetch/emscripten_fetch.cpp and system/lib/fetch/asmfs.cpp. In these cases we are checking if we can do syncXHR and/or atomics.wait... under node we can do those things on the main threads IIUC.

[juj]

emscripten_is_main_browser_thread is a public API offered to users in system/include/emscripten/threading.h.

Users may be using it to detect the main VM thread, switching its meaning from beneath them could cause regressions.

If you want to have a function specifically for detecting "is atomics wait supported", then a function emscripten_current_thread_supports_atomics_wait() or similar would sound fine - but changing public API behavior like this is not - it would be best to revert this.

[sbc100]

Since node threading itself in emscripten is relatively new (I imagine most of the usage is in the emscripten test suite rather than in production) I seems reasonable to assume that the impact of this change will be minor, no? Perhaps we could advertise the change more widely, or ask if anyone is actually using node threading in production? (I don't know of any users so far, do you?).

Can you explain a little more why you think we want emscripten_is_main_browser_thread to return true on the main node VM thread? It is just because this was previously true, or is there some other reason you want to conflate the main node thread with the main browser thread?

[sbc100]

Actually, I guess there is no harm in reverting this change.. I will add some docs to indicate that this API returns true even outside the browser.

[juj]

Can you explain a little more why you think we want emscripten_is_main_browser_thread to return true on the main node VM thread?

Like I mentioned before, the intent of this function has been to mean whether running on the primary thread of the execution environment. Also, like stated above, it is a bit awkward that it did receive the name 'browser' in it - but the intent of the function was to be interpreted with the emphasis on the word main thread rather than the word browser.

The original purpose of this function was to enable users to distinguish between all kinds of APIs that are available on the main thread versus not available in a Worker.

There should not be any users out there who would have used this function to generally identify whether current execution environment is a browser or node.js, especially since this function is offered in the multithreading header. (and if there were, they would from the get-go have reported emscripten_is_main_browser_thread() as buggy and not properly detecting node.js)

So all of a sudden changing its purpose because of the name changes it to mean something that nobody is using it for.

I agree that the function name is not good, in hindsight it would have been nicer to have this function named emscripten_is_main_vm_thread() instead. If something is to be changed here, it would be either to follow name, or follow the intent.

Following name (like this PR did), has 1) the issue that it potentially breaks existing users, but 2) it also diminishes the value of the function, since it will now detect a mix of two things: "if browser and main vm thread". It is very rare to want to do these two things at the same time. It would be better to have two orthogonal functions "if browser"/"if node" and "if main vm thread"/"if worker" for such detection.

Following the intent has the benefit of retaining orthogonality and not breaking users, but has the issue that the name will not be as accurate for the node.js case. To fix that, we can add a function emscripten_is_main_vm_thread() that is an alias to emscripten_is_main_browser_thread() and then mark emscripten_is_main_browser_thread() as deprecated?

No intentional conflation going on here, but an intent to keep things orthogonal and not break anyone.

[sbc100]

I guess my thinking was that, unlike on the web, the main VM thread on node its not special or limited like it is on the web (I could be wrong about this). None of the same limits that apply to the main VM thread on the web apply the main VM thread under node, so it s not really analogous.

Isn't the main VM thread under node more like a worker on the web? (i.e blocking calls and atomics.wait are both available). Should we consider running under node more like the case where an application was run directly from worker and the main web thread is simply not involved (in this case emscripten_is_main_browser_thread() always returns false)?

Another way of putting it: Are there valid uses for emscripten_is_main_vm_thread() have under node? Is there anything special about it or is it just like a worker?

[kripken]

emscripten_is_main_browser_thread is a public API offered to users in system/include/emscripten/threading.h.

I agree a breaking change is risky, but a change for Node.js seems less risky.

We also offer emscripten_is_main_runtime_thread, and most users should use that, unless they actually worry about blocking on the main thread, which is specific to the web?