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

GRPC within service worker / web worker #587

Open
peteringram0 opened this issue Jul 8, 2019 · 11 comments
Open

GRPC within service worker / web worker #587

peteringram0 opened this issue Jul 8, 2019 · 11 comments

Comments

@peteringram0
Copy link

Im trying to run GRPC from inside a service worker. It works fine from inside the main application thread and only throws an error from inside a service worker.

const media = new MediaServiceClient(process.env.VUE_APP_GRPC)
media.index(pagination, headers)

Error:

Uncaught (in promise) TypeError: e.serializeBinary is not a function

Im assuming this is due to how GRPC communicates to the backend services and assuming this is not possible to do at the moment?

Thanks

@iuria21
Copy link

iuria21 commented Jul 9, 2019

same problem here

@wenbozhu
Copy link
Member

We have a patch in google-closure to support service workers (via fetch) and will try to get the change to github soon.

@travikk
Copy link
Contributor

travikk commented Oct 6, 2019

@wenbozhu has this been resolved yet?

@markns
Copy link

markns commented Oct 6, 2019

@travikk if it's any use to hear, we've been using grpc-web in web workers without a problem.

@edyu
Copy link

edyu commented Jun 2, 2020

@travikk if it's any use to hear, we've been using grpc-web in web workers without a problem.

How so? The generated code references window object which is not available in web worker. @markns

@altryne
Copy link

altryne commented Sep 19, 2022

Same problem, I moved forward a bit with using
transport: grpc.FetchReadableStreamTransport({}),
since workers support fetch,

but I'm getting:

Fetch.catch Fetch API cannot load: grpc:
and then Response closed without headers

ugh an no way of knowing how else to proceed

@sampajano
Copy link
Collaborator

@altryne FYI there's a recent discussion going on in #1277 that will probably bring a solution here. You could consider follow it :)

@tjhorner
Copy link

It looks like the gRPC client accepts an instance of XhrIo:

this.xhrIo_ = xhrIo || null;

Which accepts an XmlHttpFactory, and there is an implementation of that using the fetch API.

It seems to me that if one can get an instance of XhrIo initialized with FetchXmlHttpFactory, it will work. Unless there's some big part of the implementation that is inconsistent with XHRs. I can see that at one point this was planned for development, but never finished:

/**
* The Worker global scope. Once this option is specified, gRPC-Web will
* also use 'fetch' API as the underlying transport instead of native
* XmlHttpRequest.
* @type {!WorkerGlobalScope|undefined}
*/
this.workerScope;

After finishing the implementation so that it reads workerScope from the options and instantiates XhrIo with FetchXmlHttpFactory using that workerScope, it appears to work just fine. A simple unary RPC call is finished with no problem inside of a service worker:

image

However, I haven't tested server-side streaming using this mode.

If I open a PR to upstream this change, would that be helpful to push this along? Even if there are some features not supported using fetch, those could be noted as a caveat and with helpful error messages explaining why.

@sampajano
Copy link
Collaborator

@tjhorner Hi! Thanks for your interest and digging here! Much appreciated!

Actually, i've realized that this feature was implemented on the internal fork but not open sourced..

It should actually be quite an easy change!

However, I haven't tested server-side streaming using this mode.

There's another option called useFetchDownloadStreams as below:

* This is an experimental feature to reduce memory consumption
* during high throughput server-streaming calls by using
* 'streamBinaryChunks' mode FetchXmlHttpFactory.
* @type {boolean|undefined}
*/
this.useFetchDownloadStreams;

Our internal implementation is quite simple too. Basically replacing the following line:

const xhr = this.xhrIo_ ? this.xhrIo_ : new XhrIo();

with something like this:

  newXhr(isUnary) {
    const useBinaryChunks = this.chunkedServerStreaming_ && !isUnary;
    if (this.workerScope_ || useBinaryChunks) {
      const xmlHttpFactory = new FetchXmlHttpFactory({
        worker: this.workerScope_,
        streamBinaryChunks: useBinaryChunks,
      });
      return new XhrIo(xmlHttpFactory);
    }
    return new XhrIo();
  }

If I open a PR to upstream this change, would that be helpful to push this along? Even if there are some features not supported using fetch, those could be noted as a caveat and with helpful error messages explaining why.

Thank you for the offering here! Appreciate it! I think it can certainly help move things along here!! 😃

The only thing is, we're currently in the process of doing a Typescript migration, so it might not be a great timing to take PRs if this takes an extended amount of time. In that case it would be better to wait until the migration is over before we do this. But if it's a really simple change, i don't mind taking it up sooner too! 😃

@tjhorner
Copy link

tjhorner commented Mar 1, 2024

@sampajano Thank you for the very quick response!

My solution was remarkably similar, minus the streamBinaryChunks option:

createXhrIo_() {
if (this.workerScope) {
return new XhrIo(new FetchXmlHttpFactory({
worker: this.workerScope
}));
}
return new XhrIo();
}

I can definitely submit a PR if you think it's a small enough change. But if I do, something I would like more info on is testing strategy. Not sure how thorough you'd want the tests for this (e.g., should fetch also be mocked similar to the existing tests with XHR, etc.)

There's no rush on my end, however. For the project we're using this for at work, we are vendoring the library with these changes, so I'm ok with waiting for a more official release in the meantime.

Let me know what you think!

@sampajano
Copy link
Collaborator

My solution was remarkably similar, minus the streamBinaryChunks option:

Oh great! Glad to note! :)

Could you try passing the useFetchDownloadStreams option as well (which passes streamBinaryChunks to the underlying factory)? I think it's more efficient. If it works i don't mind just leave it on by default :)


I can definitely submit a PR if you think it's a small enough change. But if I do, something I would like more info on is testing strategy. Not sure how thorough you'd want the tests for this ...

Really appreciate the considerations! Yes def agreed that we should ensure it's tested 😃

Luckily we have an interop test already i think you can probably easily reuse! (no need for mocking! :)

I think you can just:

  1. Copy the following block and add the --use_fetch option to npm test

  2. And add a new option in the test (around here) that looks like this:

if (argv.use_fetch) { ... }

Where you'd set a global constant or something to pass the new fetch option to all new TestServiceClient(...) calls.


If you don't mind, could you try the above and see how it works? Lemme know if you run into any issues.

Thanks! 😃

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

No branches or pull requests

9 participants