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

Issues when awaiting proxy methods simultaneously #2

Open
arseneyr opened this issue Jul 29, 2020 · 2 comments · May be fixed by #3
Open

Issues when awaiting proxy methods simultaneously #2

arseneyr opened this issue Jul 29, 2020 · 2 comments · May be fixed by #3

Comments

@arseneyr
Copy link

I'm having issues when calling two methods on the same proxied object and waiting on the results simultaneously. I have a minimal repro here (run yarn start and load the extension in the build folder) but here is the gist of it:

//background script

import * as Comlink from "comlink";
import { createBackgroundEndpoint, isMessagePort } from "comlink-extension";
import { browser } from "webextension-polyfill-ts";

class BackgroundEndpoint {
  getSubProxy() {
    return Comlink.proxy({
      getB: () => {
        return "B";
      },
    });
  }

  getA() {
    return "A";
  }
}

browser.runtime.onConnect.addListener((port) => {
  if (isMessagePort(port)) {
    return;
  }
  Comlink.expose(new BackgroundEndpoint(), createBackgroundEndpoint(port));
});

//content script

import { createEndpoint } from "comlink-extension";
import * as Comlink from "comlink";
import { browser } from "webextension-polyfill-ts";

const obj = Comlink.wrap(createEndpoint(browser.runtime.connect()));

obj
  .getSubProxy()
  .then((s) => s.getB())
  // s.getB() never resolves and "B" is not logged
  .then(console.log);

obj.getA().then(console.log);

The promise returned by s.getB() never resolves. The interesting thing is that changing the order of the promises (i.e. calling getA() before getSubProxy()) works fine. Also this is not a problem when using Comlink and web workers directly. Am I doing something obviously wrong here?

@samdenty
Copy link
Owner

samdenty commented Jul 29, 2020

Should've patched all the port creations, but maybe I left something out

If you want to debug, I'd log the messages that are sent and that should reveal more

@arseneyr arseneyr linked a pull request Jul 29, 2020 that will close this issue
@arseneyr
Copy link
Author

I believe this is a race condition in forward():

(await port).addEventListener("message", ({ data, ports }: any) => {
messagePort.postMessage(data, ports as any);
});

Specifically, the port promise can resolve after a message has already been received and is lost. I'm not too familiar with Javascript internals but I assume it's an implementation detail whether the promise is resolved first or the message is processed.

I've opened #3 to make forward() sync and callers to use callbacks instead.

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

Successfully merging a pull request may close this issue.

2 participants