Skip to content

Set emscripten_is_main_browser_thread based on ENVIRONMENT_IS_WEB #15630

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
sbc100 merged 1 commit into from
Nov 24, 2021
Merged

Set emscripten_is_main_browser_thread based on ENVIRONMENT_IS_WEB #15630

Show file tree
Hide file tree
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
3 changes: 3 additions & 0 deletions ChangeLog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ See docs/process.md for more on how version tagging works.

3.0.1
-----
- The return value of `emscripten_is_main_browser_thread` was fixed such that
it no longer returns true when the application is started outside of the
main browser thread (.e.g. in a worker, or under node). (#15630)
Copy link
Collaborator

@juj juj Nov 25, 2021

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

@sbc100 sbc100 Nov 29, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@juj juj Nov 30, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@sbc100 sbc100 Nov 30, 2021

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

@sbc100 sbc100 Nov 30, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@juj juj Nov 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@sbc100 sbc100 Nov 30, 2021

Choose a reason for hiding this comment

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

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?

Copy link
Member

@kripken kripken Nov 30, 2021

Choose a reason for hiding this comment

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

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?


3.0.0 - 11/22/2021
------------------
Expand Down
2 changes: 1 addition & 1 deletion src/library_pthread.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -598,7 +598,7 @@ var LibraryPThread = {
// Pass the thread address to the native code where they stored in wasm
// globals which act as a form of TLS. Global constructors trying
// to access this value will read the wrong value, but that is UB anyway.
__emscripten_thread_init(tb, /*isMainBrowserThread=*/!ENVIRONMENT_IS_WORKER, /*isMainRuntimeThread=*/1);
__emscripten_thread_init(tb, /*isMainBrowserThread=*/ENVIRONMENT_IS_WEB, /*isMainRuntimeThread=*/1);
#if ASSERTIONS
PThread.mainRuntimeThread = true;
#endif
Expand Down
2 changes: 1 addition & 1 deletion src/library_pthread_stub.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ var LibraryPThreadStub = {
#if MINIMAL_RUNTIME
return typeof importScripts === 'undefined';
Copy link
Collaborator

@kleisauke kleisauke Nov 24, 2021

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@sbc100 sbc100 Nov 24, 2021

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@kleisauke kleisauke Nov 24, 2021

Choose a reason for hiding this comment

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

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)

Copy link
Collaborator

@kleisauke kleisauke Nov 25, 2021

Choose a reason for hiding this comment

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

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;

Copy link
Collaborator

@juj juj Nov 25, 2021

Choose a reason for hiding this comment

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

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(?)

#else
return !ENVIRONMENT_IS_WORKER;
return ENVIRONMENT_IS_WEB;
#endif
},
};
Expand Down
3 changes: 1 addition & 2 deletions system/lib/fetch/emscripten_fetch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,7 @@ emscripten_fetch_t* emscripten_fetch(emscripten_fetch_attr_t* fetch_attr, const
const bool writeToIndexedDB = (fetch_attr->attributes & EMSCRIPTEN_FETCH_PERSIST_FILE) != 0 ||
!strncmp(fetch_attr->requestMethod, "EM_IDB_", strlen("EM_IDB_"));
const bool performXhr = (fetch_attr->attributes & EMSCRIPTEN_FETCH_NO_DOWNLOAD) == 0;
const bool isMainBrowserThread = emscripten_is_main_browser_thread() != 0;
if (isMainBrowserThread && synchronous && (performXhr || readFromIndexedDB || writeToIndexedDB)) {
if (emscripten_is_main_browser_thread() && synchronous && (performXhr || readFromIndexedDB || writeToIndexedDB)) {
#ifdef FETCH_DEBUG
emscripten_console_errorf("emscripten_fetch('%s') failed! Synchronous blocking XHRs and IndexedDB operations are not supported on the main browser thread. Try dropping the EMSCRIPTEN_FETCH_SYNCHRONOUS flag, or run with the linker flag --proxy-to-worker to decouple main C runtime thread from the main browser thread.", url);
#endif
Expand Down
2 changes: 1 addition & 1 deletion system/lib/pthread/emscripten_thread_state.S
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ emscripten_is_main_runtime_thread:
global.get is_runtime_thread
end_function

# Semantically the same as testing "!ENVIRONMENT_IS_WORKER" in JS
# Semantically the same as testing "ENVIRONMENT_IS_WEB" in JS
.globl emscripten_is_main_browser_thread
emscripten_is_main_browser_thread:
.functype emscripten_is_main_browser_thread () -> (i32)
Expand Down
8 changes: 5 additions & 3 deletions tests/other/test_pthread_self_join_detach.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,14 @@
#endif

int main() {
#ifdef __EMSCRIPTEN__
/*
* When running in PROXY_TO_PTHREAD mode the main thread
* is already detached
* is already detached. We detect PROXY_TO_PTHREAD mode
* by noticing that the main() function is not running
* in the main runtime thread.
*/
#ifdef __EMSCRIPTEN__
int is_detached = !emscripten_is_main_browser_thread();
int is_detached = !emscripten_is_main_runtime_thread();
#else
int is_detached = 0;
#endif
Expand Down