grpc-web icon indicating copy to clipboard operation
grpc-web copied to clipboard

GRPC within service worker / web worker

Open peteringram0 opened this issue 4 years ago • 11 comments

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

peteringram0 avatar Jul 08 '19 09:07 peteringram0

same problem here

iuria21 avatar Jul 09 '19 08:07 iuria21

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

wenbozhu avatar Jul 12 '19 00:07 wenbozhu

@wenbozhu has this been resolved yet?

travikk avatar Oct 06 '19 04:10 travikk

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

markns avatar Oct 06 '19 09:10 markns

@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

edyu avatar Jun 02 '20 08:06 edyu

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

altryne avatar Sep 19 '22 03:09 altryne

@altryne FYI there's a recent discussion going on in https://github.com/grpc/grpc-web/issues/1277 that will probably bring a solution here. You could consider follow it :)

sampajano avatar Sep 19 '22 22:09 sampajano

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

https://github.com/grpc/grpc-web/blob/0350052fab4d6a01945526c91ab1c4a4ad89999f/javascript/net/grpc/web/grpcwebclientbase.js#L96

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:

https://github.com/grpc/grpc-web/blob/0350052fab4d6a01945526c91ab1c4a4ad89999f/javascript/net/grpc/web/clientoptions.js#L48-L54

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.

tjhorner avatar Feb 29 '24 22:02 tjhorner

@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:

https://github.com/grpc/grpc-web/blob/0350052fab4d6a01945526c91ab1c4a4ad89999f/javascript/net/grpc/web/clientoptions.js#L57-L62

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

https://github.com/grpc/grpc-web/blob/0350052fab4d6a01945526c91ab1c4a4ad89999f/javascript/net/grpc/web/grpcwebclientbase.js#L187

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! 😃

sampajano avatar Mar 01 '24 01:03 sampajano

@sampajano Thank you for the very quick response!

My solution was remarkably similar, minus the streamBinaryChunks option:

https://github.com/grpc/grpc-web/blob/6c41064e828238ed55e7340b8279a2694f94223f/javascript/net/grpc/web/grpcwebclientbase.js#L184-L191

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!

tjhorner avatar Mar 01 '24 02:03 tjhorner

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! 😃

sampajano avatar Mar 05 '24 01:03 sampajano