Compatibility with comlink for workers

[lgarron]

What is the problem this feature would solve?

I'm excited to see worker support land in #3645 — the tradeoffs look pretty great!

However, I've so far been unable to use the bun v0.6.15 canary with comlink. Adapting the comlink README:

// index.js
import { wrap } from "comlink";

async function init() {
  const worker = new Worker(await import.meta.resolve("./worker.js"), {"type": "module"});
  // WebWorkers use `postMessage` and therefore work with Comlink.
  const obj = wrap(worker);
  console.log(`Counter: ${await obj.counter}`);
  await obj.inc();
  console.log(`Counter: ${await obj.counter}`);
}
await init(); // Note: `await` was missing here in the original report; see issue conversation

// worker.js
import { expose } from "comlink";

const obj = {
  counter: 0,
  inc() {
    this.counter++;
  },
};

expose(obj);
console.log("Worker initialized.")

This prints "Worker initialized." but not the counters. In fact, there is no error output, so I'm a bit stuck trying to debug.

What is the feature you are proposing to solve the problem?

Due to the lack of an error, I'm not 100% sure what would be needed to be compatible with comlink.

• The fact that comlink has hardcoded assumptions about browsers and node may be causing unexpected behaviour.
• This may be due to MessagePort, but I'm not sure (since some postMessage(…) functionality is already supported).

Given that comlink is the de facto library for exposing an async API over a worker boundary, I think it would be pretty valuable to be compatible out of the box.

What alternatives have you considered?

I'm not aware of any viable alternatives to comlink (which has ≈10K stars on GitHub). If bun were not compatible, I'd probably have to figure out how to implement custom workarounds instead of consolidating around ecosystem standards and conventions. I have a lot of experience with this, but it doesn't scale to other projects.

It's also possible that small fixes to comlink would make it work with bun — I don't have enough information to draft a potential PR, though.

[Jarred-Sumner]

Workers have a lifetime similar to Node rather than Web

Try calling .ref() on the Worker. Or pass { bun: { ref: true } } to the Worker constructor if you want to stick with browser compatible code

[Jarred-Sumner]

Also see https://bun.sh/docs/api/workers

[lgarron]

Workers have a lifetime similar to Node rather than Web

Try calling .ref() on the Worker. Or pass { bun: { ref: true } } to the Worker constructor if you want to stick with browser compatible code

Hmm, interesting. Normally, to get node's worker_threads working like web workers I have to call .unref(). However, if the main thread is awaiting on a Promise that requires a message from the worker to resolve, the program shouldn't be exiting early either way, right?

If I ref the worker, I see the output "Counter: 0" but the program exits (with exit code 0) before await obj.inc() resolves. But thanks, this gives me a place to start debugging!

EDIT: actually, it seems to be a race condition whether "Counter: 0" prints or whether the program exits before that line as well.

[lgarron]

If I ref the worker, I see the output "Counter: 0" but the program exits (with exit code 0) before await obj.inc() resolves. But thanks, this gives me a place to start debugging!

EDIT: actually, it seems to be a race condition whether "Counter: 0" prints or whether the program exits before that line as well.

More specifically, the following program exits without waiting 10 seconds and without printing an error. Somehow the execution of the main thread is being halted due to the presence of await obj.inc(); without throwing an error. 🤔

import { wrap } from "comlink";

async function init() {
  try {
    const worker = new Worker(await import.meta.resolve("./worker.js"), {"type": "module", bun: { ref: true }});
    worker.ref()
    // WebWorkers use `postMessage` and therefore work with Comlink.
    const obj = wrap(worker);
    console.log(`Counter: ${await obj.counter}`);
    await obj.inc();
  } catch (e) {
    console.log("Error!", e);
  } finally {
    await new Promise((resolve) => setTimeout(resolve, 10000)); // sleep 10 seconds
  }
}
init();

[Jarred-Sumner]

However, if the main thread is awaiting on a Promise that requires a message from the worker to resolve, the program shouldn't be exiting early either way, right?

normally yes, but init isn’t awaited.

[lgarron]

normally yes, but init isn’t awaited.

Ah, I'm sorry. Good catch — I was trying to stay close to the sample code but I'm not normally used to testing code using a named async wrapper like this. 🤦

[lgarron]

Okay, I dug into this for an hour and unfortunately all I could determine is that the "message" listener on the worker side executes the first time it receives a postMessage(…) from requestResponseMessage(…) to the worker object, but it does not execute on subsequent times (in the current bun canary).

The worker remains alive and has not removed the "message" listener, so I don't know why it would stop receiving messages. I still have no idea if this might be due to bun's implementation or due to a bad assumption in comlink.

[Jarred-Sumner]

there's probably a bug in bun here, due to lack of test coverage. sorry for taking up your time on this

[Jarred-Sumner]

After 777ee4e, back and forth messages work as expected

[Jarred-Sumner]

There is a GC bug, I suggest waiting a couple more days or until v0.7 is released

[lgarron]

Thanks for the fix, I'm glad to see it! 😻

I can confirm that the latest canary prints out all the expected counters.

However, it now seems to have the opposite problem, where the sample program in the first comment doesn't exit, and I'm unable to fix that by trying to explicity unref the worker. (From other projects, I know that comlink doesn't prevent the program from exiting, as long as the worker is unrefed.)

This isn't necessarily a big issue, but I thought I'd note it.

[paperdave]

I did some work on Workers and their lifetime+event loop to bring them closer to how they work in Node.js, and those unref issues shouldnt exist anymore; I think we can close this issue now. If there are any new inconsistencies, we can re-open this.