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.

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

More ergonomic access to modules that are in workers #72

Closed
josephrocca opened this issue Jul 22, 2022 · 2 comments
Closed

More ergonomic access to modules that are in workers #72

josephrocca opened this issue Jul 22, 2022 · 2 comments
Labels
question Further information is requested

Comments

@josephrocca
Copy link
Contributor

(Note: This seems like something that would be handled in a separate, follow-up proposal, but I'm just posting here to make sure the door is left open to allowing this sort of thing, if possible.)


Some relevant discussion: whatwg/html#6911 (comment) (not required reading for this issue, but useful for context)

As I mentioned in that thread, a big pain point with workers is the postMessage/onmessage wrangling required to take some functions off the main thread and put them in a worker (e.g. tracking message IDs so I can get a simple promisified API to the functions that are now in the worker). This can of course be handled by a library like comlink, but it seems like a common enough pattern to warrant consideration here.

Some example/pseudo code from that thread:

const worker = new Worker();
const workerModule = await worker.addModule(module { export function sum(a, b) { return a+b } });
const sum = await workerModule.sum(1, 2);

Please consider that as very-pseudo code - I'm not proposing any concrete API here, just giving an example of an ergonomic way of using the in-worker module from the main thread. There would of course also be technical details to iron out (e.g. regarding behavior for non-serializable stuff).

I'm wondering if this proposal is in a position to potentially allow something like that in the future?

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Jul 22, 2022

Yes! That HTML issue proposes an addModule built-in function, but it's already possible to implement something exactly like that code you showed just using this proposal (as shown below). However, while this proposal is in a position to allow it, the addModule API is ultimately a decision made by the HTML standard and that we cannot make in this proposal.

Example implementation
class ModulesWorker {
  #worker;
  #modules = new Map();
  #nextModuleId = 0;
  #calls = new Map();
  #nextCallId = 0;

  constructor() {
    this.#worker = new Worker(new URL("./modules-worker-runner.js", import.meta.url));
    this.#worker.addEventListener("message", ({ data }) => {
      if (data.action === "REGISTER") this.#finishRegistration(data.id, data.error);
      if (data.action === "CALL") this.#finishCall(data.id, data.result, data.error);
    });
  }

  addModule(mod) {
    let id = this.#nextModuleId++;
    worker.postMessage({ action: "REGISTER", id, mod });
    return ModulesWorker.#promiseWithFinalizers(this.#modules, id);
  }

  static #promiseWithFinalizers(map, id) {
    let resolve, reject;
    const promise =  new Promise((res, rej) => {
      resolve = res, rejected = rej;
    });
    map.set(id, { resolve, reject });
    return promise;
  }

  #finishRegistration(id, error) {
    if (error != null) {
      this.#modules.get(id).reject(error);
    } else {
      this.#modules.get(id).resolve(new Proxy({}, {
        get: (target, key) => (...args) =>  this.#call(id, key, args);
      }));
    }
    this.#modules.delete(id);
  }

  #call(moduleId, name, args) {
    let id = this.#nextCallId++;
    worker.postMessage({ action: "CALL", id, moduleId, name, args });
    return ModulesWorker.#promiseWithFinalizers(this.#calls, id);
  }

  #finishCall(id, result, error) {
    if (error != null) {
      this.#calls.get(id).reject(error);
    } else {
      this.#calls.get(id).resolve(result);
    }
    this.#calls.delete(id);
  }
}

Where modules-worker-runner.js is implemented like this:

const modules = new Map();

addEventListener("message", async ({ data }) => {
  if (data.action === "REGISTER") {
    import(data.mod).then(
      ns => { modules.set(data.id, ns); postMessage({ action: "REGISTER", id: data.id, error: null }) },
      error => postMessage({ action: "REGISTER", id: data.id, error })
    );
  } else if (data.action === "CALL") {
    const ns = modules.get(data.moduleId);
    if (typeof ns[name] !== "function") return postMessage({ action: "CALL", id: data.id, result: null, error: new TypeError() })
    Promise.resolve(ns[name].apply(null, data.args)).then(
      result => postMessage({ action: "CALL", id: data.id, result, error: null })
      error => postMessage({ action: "CALL", id: data.id, result: null, error })
    );
  }
});

Usage:

const worker = new ModulesWorker();
const workerModule = await worker.addModule(module { export function sum(a, b) { return a+b } });
const sum = await workerModule.sum(1, 2);

@nicolo-ribaudo nicolo-ribaudo added the question Further information is requested label Jul 22, 2022
@josephrocca
Copy link
Contributor Author

Ahh, I noobily thought that the usage of addModule in the README was somehow tied into this proposal. But makes sense that this proposal wouldn't deal that. I made a suggestion for the README so the code snippet doesn't imply a this return value for addModule, but of course this repo isn't even about addModule, so completely understand if you'd rather not merge that.

Thanks for your reply and your example implementation - I'm extremely excited about this proposal!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants