-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Fix ENVIRONMENT_IS_WORKER
detection on Deno
#22778
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.
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
Conversation
`importScripts()` is always `undefined` on Deno.
Test case: Before: $ cat > test.c << EOF
#include <assert.h>
#include <pthread.h>
#include <stdio.h>
#include <emscripten/em_asm.h>
void* worker_thread(void *arg) {
int is_worker = EM_ASM_INT(return ENVIRONMENT_IS_WORKER);
printf("ENVIRONMENT_IS_WORKER: %d\n", is_worker);
return NULL;
}
int main() {
int is_worker = EM_ASM_INT(return ENVIRONMENT_IS_WORKER);
printf("ENVIRONMENT_IS_WORKER: %d\n", is_worker);
pthread_t thread;
pthread_create(&thread, NULL, worker_thread, NULL);
pthread_join(thread, NULL);
return 0;
}
EOF
$ emcc test.c -o test.js -sASSERTIONS=0 -pthread -sEXIT_RUNTIME
$ node test.js
ENVIRONMENT_IS_WORKER: 0
ENVIRONMENT_IS_WORKER: 1
$ emcc test.c -o test.mjs -sENVIRONMENT=web,worker -sASSERTIONS=0 -pthread
$ echo "import Module from './test.mjs';await Module();Deno.exit()" | deno run --allow-read -
ENVIRONMENT_IS_WORKER: 0 [repeated x times until OOM] After: $ emcc test.c -o test.mjs -sENVIRONMENT=web,worker -sASSERTIONS=0 -pthread
$ echo "import Module from './test.mjs';await Module();Deno.exit()" | deno run --allow-read -
ENVIRONMENT_IS_WORKER: 0
ENVIRONMENT_IS_WORKER: 1 |
Looks reasonable to me. The downside is maybe a few more bytes in size, but I don't see a way around that, and it mostly affects pthreads builds (which are larger anyhow).
|
// Dummy importScripts. The presence of this global is used | ||
// to detect that we are running on a Worker. | ||
// TODO(sbc): Find another way? | ||
importScripts: () => { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
Line 144 in 9e618aa
ENVIRONMENT_IS_WORKER = !worker_threads.isMainThread; |
(I can separate this change into its own PR if needed)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
importScripts()
is alwaysundefined
on Deno.