Skip to content
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

Jump to bottom

Fix ENVIRONMENT_IS_WORKER detection on Deno #22778

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+7 −15
Open

Fix ENVIRONMENT_IS_WORKER detection on Deno #22778

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3a11414
Fix `ENVIRONMENT_IS_WORKER` detection on Deno
kleisauke Oct 23, 2024
277cbd7
Fix test expectation
kleisauke Oct 24, 2024
1ea3b80
Merge branch 'main' into deno-worker-compat
kleisauke Oct 25, 2024
4bab494
Remove `WorkerGlobalScope` polyfill for Node
kleisauke Oct 25, 2024
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
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 @@ -18,7 +18,7 @@ var LibraryPThreadStub = {

emscripten_is_main_browser_thread: () =>
#if MINIMAL_RUNTIME
typeof importScripts == 'undefined'
typeof WorkerGlobalScope == 'undefined'
#else
!ENVIRONMENT_IS_WORKER
#endif
Expand Down
11 changes: 3 additions & 8 deletions src/runtime_pthread.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,9 @@ if (ENVIRONMENT_IS_PTHREAD) {

Object.assign(globalThis, {
self: global,
// Dummy importScripts. The presence of this global is used
// to detect that we are running on a Worker.
// TODO(sbc): Find another way?
importScripts: () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a little confused because you say that deno never defines importScripts, but from the looks of this code it seems that node doesn't define it either. Isn't the point of this line to inject it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, you're right. Node.js doesn't define importScripts either.

const { Worker, isMainThread } = require('node:worker_threads');

if (isMainThread) {
  // This re-loads the current file inside a Worker instance.
  new Worker(__filename);
} else {
  console.log(typeof importScripts);  // Prints 'undefined'.
}

Commit 4bab494 removes this altogether, since ENVIRONMENT_IS_WORKER no longer depends on this dummy symbol on Node.js:

emscripten/src/shell.js

Line 144 in 9e618aa

ENVIRONMENT_IS_WORKER = !worker_threads.isMainThread;

(I can separate this change into its own PR if needed)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note that ENVIRONMENT_IS_NODE is false on Deno. IIUC, Deno tends to favor standard Web Platform APIs over proprietary ones and wasn't originally designed to be a drop-in replacement for Node.js.

Copy link
Member

Choose a reason for hiding this comment

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

Deno does have a Node.js compatibility layer these days (they started to compromise on that at some point, though they did start with "pure Web APIs" IIRC). I wonder if maybe we can make IS_NODE also accept deno, for simplicity? Separate from this PR of course. (And if it isn't trivial, then #12203 is probably the best long-term plan instead.)

This PR itself lgtm as is. Unless @sbc100 you had more thoughts?

#if ASSERTIONS
assert(false, 'dummy importScripts called');
#endif
},
// The presence of this global is used to detect
// that we are running on a Worker.
WorkerGlobalScope: global,
postMessage: (msg) => parentPort.postMessage(msg),
});
}
Expand Down
8 changes: 4 additions & 4 deletions src/shell.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ var ENVIRONMENT_IS_AUDIO_WORKLET = typeof AudioWorkletGlobalScope !== 'undefined
var ENVIRONMENT_IS_WEB = {{{ ENVIRONMENT === 'web' }}};
#if PTHREADS && ENVIRONMENT_MAY_BE_NODE
// node+pthreads always supports workers; detect which we are at runtime
var ENVIRONMENT_IS_WORKER = typeof importScripts == 'function';
var ENVIRONMENT_IS_WORKER = typeof WorkerGlobalScope != 'undefined';
#else
var ENVIRONMENT_IS_WORKER = {{{ ENVIRONMENT === 'worker' }}};
#endif
Expand All @@ -93,7 +93,7 @@ var ENVIRONMENT_IS_SHELL = {{{ ENVIRONMENT === 'shell' }}};
#else // ENVIRONMENT
// Attempt to auto-detect the environment
var ENVIRONMENT_IS_WEB = typeof window == 'object';
var ENVIRONMENT_IS_WORKER = typeof importScripts == 'function';
var ENVIRONMENT_IS_WORKER = typeof WorkerGlobalScope != 'undefined';
// N.b. Electron.js environment is simultaneously a NODE-environment, but
// also a web environment.
var ENVIRONMENT_IS_NODE = typeof process == 'object' && typeof process.versions == 'object' && typeof process.versions.node == 'string' && process.type != 'renderer';
Expand Down Expand Up @@ -298,7 +298,7 @@ if (ENVIRONMENT_IS_NODE) {
if (ENVIRONMENT_IS_SHELL) {

#if ENVIRONMENT && ASSERTIONS
if ((typeof process == 'object' && typeof require === 'function') || typeof window == 'object' || typeof importScripts == 'function') throw new Error('not compiled for this environment (did you build to HTML and try to run it not on the web, or set ENVIRONMENT to something - like node - and run it someplace else - like on the web?)');
if ((typeof process == 'object' && typeof require === 'function') || typeof window == 'object' || typeof WorkerGlobalScope != 'undefined') throw new Error('not compiled for this environment (did you build to HTML and try to run it not on the web, or set ENVIRONMENT to something - like node - and run it someplace else - like on the web?)');
#endif

#if ENVIRONMENT_MAY_BE_SHELL
Expand Down Expand Up @@ -398,7 +398,7 @@ if (ENVIRONMENT_IS_WEB || ENVIRONMENT_IS_WORKER) {
}

#if ENVIRONMENT && ASSERTIONS
if (!(typeof window == 'object' || typeof importScripts == 'function')) throw new Error('not compiled for this environment (did you build to HTML and try to run it not on the web, or set ENVIRONMENT to something - like node - and run it someplace else - like on the web?)');
if (!(typeof window == 'object' || typeof WorkerGlobalScope != 'undefined')) throw new Error('not compiled for this environment (did you build to HTML and try to run it not on the web, or set ENVIRONMENT to something - like node - and run it someplace else - like on the web?)');
#endif

#if PTHREADS && ENVIRONMENT_MAY_BE_NODE
Expand Down
2 changes: 1 addition & 1 deletion src/shell_minimal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ function ready() {
#if PTHREADS
// MINIMAL_RUNTIME does not support --proxy-to-worker option, so Worker and Pthread environments
// coincide.
var ENVIRONMENT_IS_WORKER = typeof importScripts == 'function';
var ENVIRONMENT_IS_WORKER = typeof WorkerGlobalScope != 'undefined';
var ENVIRONMENT_IS_PTHREAD = ENVIRONMENT_IS_WORKER && self.name?.startsWith('em-pthread');

#if !MODULARIZE
Expand Down
2 changes: 1 addition & 1 deletion test/test_other.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14919,7 +14919,7 @@ def test_embind_no_duplicate_symbols(self):
def test_no_pthread(self):
self.do_runf('hello_world.c', emcc_args=['-pthread', '-no-pthread'])
self.assertExists('hello_world.js')
self.assertNotContained('Worker', read_file('hello_world.js'))
self.assertNotContained('new Worker(', read_file('hello_world.js'))

def test_sysroot_includes_first(self):
self.do_other_test('test_stdint_limits.c', emcc_args=['-std=c11', '-iwithsysroot/include'])
Expand Down
Loading