Fix worker event loop ref/unref + leak #4114

paperdave · 2023-08-11T02:21:13Z

What does this PR do?

Fixes worker tests but it makes these changes to WebWorker.

was reimplemented a little on aug 11:

A worker's event loop impacts the main process' event loop. Meaning if you setTimeout in a worker/listen for messages, it keeps main alive.
Worker is by default .ref(), but workers will exit if their event loop dies.
If you do addEventListener("message") on a message port or the global, this refs the event loop.
If you do worker.unref() this no longer makes the worker's event loop affect the caller's. This is what esbuild depends on.
We were not freeing WebWorker struct. This was because its a little complex. ~~I solved it by assigning vm to null when the vm is done with, then the while(true) loop for the worker (the source of the UAF) would call real deinit.~~ It's not really that complex with the changes to the worker event loop. .terminate sets a flag to tell the worker to exit the event loop, and the deinit is called from within spin() on the Worker's thread.

I'd like a close review to make sure i did everything right. I triple checked my uses of event loop refs and memory usage in the worker zig file.

Documentation or TypeScript types (it's okay to leave the rest blank in this case)
Code changes

How did you verify your code works?

I ran unit tests. For testing large scale worker event loop stuff.

import { Worker, isMainThread, workerData } from "worker_threads";

if (isMainThread) {
  for (let i = 0; i < 1000; i++) {
    const w = new Worker("/Users/dave/code/bun/test/js/web/worker-demo.mjs", { ref: false, workerData: i });
    let recieved = false;
    w.on("message", msg => {
      if (msg === "initial message") {
        recieved = true;
      } else {
        console.log("WHAT?", msg);
      }
    });
    setTimeout(() => {
      if (!recieved) {
        w.terminate();
        console.log("worker", i, "did not respond");
      }
    }, 5000 + 500);
  }
} else {
  setTimeout(() => {
    self.postMessage("initial message");
    console.log("workerid=", workerData);
  }, Math.random() * 5000 + 500);
}

Pipe this to a file to see that exactly 1000 lines are output.

Jarred-Sumner · 2023-08-11T02:25:43Z

src/bun.js/module_loader.zig

-                            worker.queueInitialTask();
-                        }
-                    }
+                    // defer {


is this intentional?

worker.queueInitialTask was removing an eventloop ref way too early. since the ref was now based on the worker's event loop, the initial task to unref isn't needed i think. did not mean to leave as commented code.

code here was removed instead of commented

src/bun.js/web_worker.zig

Jarred-Sumner · 2023-08-11T02:27:20Z

src/bun.js/web_worker.zig

-                    continue;
+        while (true) {
+            while (vm.eventLoop().tasks.count > 0 or vm.active_tasks > 0 or vm.uws_event_loop.?.active > 0) {
+                this.eventloop_poll_ref.refConcurrently(this.parent);


this should not be here

i meant to have this at the end of the while(true) (didnt push that commit because was looking into other bugs)

It should not call .refConcurrently() on every tick of the event loop? It should only need to call it once while there are pending tasks.

Jarred-Sumner · 2023-08-11T02:28:55Z

src/bun.js/web_worker.zig

-            this.onTerminate();
+            this.eventloop_poll_ref.unrefConcurrently(this.parent);
+            vm.eventLoop().tickPossiblyForever();
+            this.parent.eventLoop().wakeup();


there is a race condition here where the parent event loop is unref'd

for the case where the parent event loop is not the main thread

oh i did not think about workers starting other workers

github-actions · 2023-08-11T02:49:40Z

❌ @paperdave 2 files with test failures on bun-darwin-aarch64:

test/js/bun/test/test-test.test.ts
test/js/node/dns/node-dns.test.js

View test output

^{#706591f8fb828e8de364e047f8b461c90ea10673}

github-actions · 2023-08-11T02:49:47Z

❌ @paperdave 1 files with test failures on linux-x64:

test/js/third_party/prisma/prisma.test.ts

View test output

^{#706591f8fb828e8de364e047f8b461c90ea10673}

github-actions · 2023-08-11T02:50:45Z

❌ @paperdave 1 files with test failures on linux-x64-baseline:

test/js/third_party/prisma/prisma.test.ts

View test output

^{#706591f8fb828e8de364e047f8b461c90ea10673}

github-actions · 2023-08-11T03:08:02Z

❌ @paperdave 4 files with test failures on bun-darwin-x64-baseline:

test/js/bun/spawn/spawn-streaming-stdin.test.ts
test/js/bun/sqlite/sqlite.test.js
test/js/third_party/webpack/webpack.test.ts
test/js/web/timers/setTimeout.test.js

View test output

^{#706591f8fb828e8de364e047f8b461c90ea10673}

paperdave · 2023-08-12T02:59:31Z

src/bun.js/base.zig

@@ -2122,6 +2122,11 @@ pub export fn MarkedArrayBuffer_deallocator(bytes_: *anyopaque, _: *anyopaque) v
    // zig's memory allocator interface won't work here
    // mimalloc knows the size of things
    // but we don't
+    if (comptime Environment.allow_assert) {


[existing bug reproduced by sveltekit build. this assertion makes it fail consistantly]

paperdave · 2023-08-12T03:02:38Z

test/js/third_party/esbuild/esbuild-test.js

@@ -82,5 +82,3 @@ import { build, buildSync, transform, transformSync } from "esbuild";
    throw new Error("Test failed.");
  }
 }
-
-process.exit(0);


probably was not needed, but a reason to keep this is that the process should exit since there are no ref'd polls.

paperdave · 2023-08-12T06:58:19Z

bug with terminate() but usable

Douile · 2023-08-13T17:13:30Z

This PR adds documentation about compiling a specific version of ZLS that works better which is after the Install zig part of the docs. But the version of zig installed by the install zig section (0.11.0-dev.4004+a57608217) is too old to compile the specific version of ZLS (v0.11.0-dev.4332+43b830415 is required). Perhaps its worth adding something that mentions this if this is intended.

78defe7#diff-f0c08b47e6be8e8b574a71f652c3c2c66f49d157b392f3091c58e8585d411010R183-R202

Jarred-Sumner reviewed Aug 11, 2023

View reviewed changes

src/bun.js/web_worker.zig Outdated Show resolved Hide resolved

Jarred-Sumner reviewed Aug 11, 2023

View reviewed changes

src/bun.js/web_worker.zig Outdated Show resolved Hide resolved

Jarred-Sumner reviewed Aug 11, 2023

View reviewed changes

paperdave added 7 commits August 11, 2023 17:53

make more tests pass

15ad319

worker changes

8c4fece

fix some bugs

afcc182

remove this

e6beae9

progress

802ab03

uh

032cb7d

okay

e9733e6

paperdave force-pushed the dave/workertests branch from d6cf039 to e9733e6 Compare August 12, 2023 02:58

remove console log

54abcab

paperdave commented Aug 12, 2023

View reviewed changes

paperdave added 2 commits August 11, 2023 20:04

a

b4b5ee9

comment assert for later

2b50f87

Jarred-Sumner approved these changes Aug 12, 2023

View reviewed changes

paperdave added 3 commits August 11, 2023 23:56

mergable state

20601ac

remove test

472be27

remove test

ea4fd3f

paperdave mentioned this pull request Aug 12, 2023

worker.terminate() does not force code to halt. #4134

Open

Jarred-Sumner merged commit 78defe7 into main Aug 12, 2023
15 of 20 checks passed

Jarred-Sumner deleted the dave/workertests branch August 12, 2023 20:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix worker event loop ref/unref + leak #4114

Fix worker event loop ref/unref + leak #4114

paperdave commented Aug 11, 2023 •

edited

Jarred-Sumner Aug 11, 2023

paperdave Aug 11, 2023

paperdave Aug 12, 2023

Jarred-Sumner Aug 11, 2023

paperdave Aug 11, 2023 •

edited

Jarred-Sumner Aug 11, 2023

Jarred-Sumner Aug 11, 2023

Jarred-Sumner Aug 11, 2023

paperdave Aug 11, 2023

github-actions bot commented Aug 11, 2023 •

edited

github-actions bot commented Aug 11, 2023 •

edited

github-actions bot commented Aug 11, 2023 •

edited

github-actions bot commented Aug 11, 2023 •

edited

paperdave Aug 12, 2023

paperdave Aug 12, 2023

paperdave commented Aug 12, 2023

Douile commented Aug 13, 2023

Fix worker event loop ref/unref + leak #4114

Fix worker event loop ref/unref + leak #4114

Conversation

paperdave commented Aug 11, 2023 • edited

What does this PR do?

How did you verify your code works?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paperdave Aug 11, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 11, 2023 • edited

github-actions bot commented Aug 11, 2023 • edited

github-actions bot commented Aug 11, 2023 • edited

github-actions bot commented Aug 11, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paperdave commented Aug 12, 2023

Douile commented Aug 13, 2023

paperdave commented Aug 11, 2023 •

edited

paperdave Aug 11, 2023 •

edited

github-actions bot commented Aug 11, 2023 •

edited

github-actions bot commented Aug 11, 2023 •

edited

github-actions bot commented Aug 11, 2023 •

edited

github-actions bot commented Aug 11, 2023 •

edited