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

[Feature request] Add wasm support #2294

Closed
MOZGIII opened this issue Sep 6, 2018 · 22 comments
Closed

[Feature request] Add wasm support #2294

MOZGIII opened this issue Sep 6, 2018 · 22 comments
Labels
P3 Status: Blocked Type: Feature New features or improvements in behavior

Comments

@MOZGIII
Copy link

MOZGIII commented Sep 6, 2018

Hey guys!

Since Go has now wasm support as a target, have you considered implementing a version of grpc-go that would be usable from within the browser or nodejs (or any wasm runtime)?

Thanks is advance.

@mastersingh24
Copy link

@MOZGIII - For the browser, you may want to check out gRPC Web.

@nicolasnoble
Copy link
Member

Wasm, by design, doesn't have low level networking support, so this request is inherently impossible.

@MOZGIII
Copy link
Author

MOZGIII commented Sep 6, 2018

@nicolasnoble understandable. On the other hand, wasm on it's own does not have any networking support, but we can declare an API that nodejs can fulfill. Browsers are not a major concern for this, but the browser may be able to fulfill that API is at some point in the future too.
In fact, maybe it should be not in Go, but C.
@mastersingh24 thanks. I'm actually using gRPC-Web already, along with node-grpc.

@dfawley
Copy link
Member

dfawley commented Sep 6, 2018

/cc @johanbrandhorst

@MOZGIII - See #2174. It was not merged because we don't want so much special casing going on inside our transport (which is already in need of some cleanup). Instead we'll be adding support for pluggable transports (#906), and then a separate transport can be implemented for wasm.

@dfawley dfawley added Status: Blocked Type: Feature New features or improvements in behavior P3 labels Sep 6, 2018
@johanbrandhorst
Copy link
Contributor

I do plan on submitting a PR adding transparent WASM client support to this library when the transport interface is available. It'll either be merged and activated under the WASM build tag or I'll publish and maintain it on my public github. Watch this space :).

@johanbrandhorst
Copy link
Contributor

If you want to see #2174 in action, check out the demo on my talk about WASM: https://youtu.be/iTrx0BbUXI4?t=847.

@hsaliak
Copy link

hsaliak commented Sep 6, 2018

@MOZGIII -- trying to understand this request better, and I have some questions: What is the motivation for wanting something like this? Is this something that would be 'cool to have' or will it enable something critical for you?

@hsaliak hsaliak closed this as completed Sep 6, 2018
@hsaliak hsaliak reopened this Sep 6, 2018
@johanbrandhorst
Copy link
Contributor

@hsaliak the motivation for me was to have a gRPC client written in Go transparently work if the build target changes to WASM. It would automatically use a gRPC-Web compatible client, of course, but it would allow for some cool stuff, and maybe even compiling existing go gRPC clients for use in the browser via WASM.

@MOZGIII
Copy link
Author

MOZGIII commented Sep 6, 2018

@hsaliak tl;dr: it's purely a "cool to have" at this point.

The original idea for this PR was to use Go instead of node-grpc in a nodejs-based API gateway. I was elaborating on options to better integrate generated code with with TypeScript - which I resolved even before creating this issue (it doesn't seem to reduce the effort much) - so don't mind that, just mentioning it as a context.

After some time I though a bit and created this issue. It's not on our critical path, and is more like a nice to have. The benefits of it on it's own (without TypeScript definitions to allow type-checked code) would be to share interceptors and metadata handling procedures with the the Go code bases. Potentially sharing load balancing / discovery logic too - but we're more likely to solve it by introducing service mesh (i.e. istio or sth). And even interceptors/metadata stuff is solved differently.

We're already using gRPC-Web in browser, but I mainly envisioned using wasm grpc-go at the server-side in a nodejs server. That means, Go-via-C or other kind of nodejs binding would work too.

Also, I was just curious if somebody is working on this - cause it's cool :)

Now if I think about it, I'd rather have #906 done so that wasm support can be added by users on a need basis, rather than explicitly supporting it. However, it's a good idea to test the baseline - whether the code builds under wasm or not (with some dummy transport) - and keep that part working.

@MOZGIII
Copy link
Author

MOZGIII commented Sep 6, 2018

Transport support for wasm would be effectively solved by #906 (at least I'll be able to plug in my own transport), but ensuring that the code builds with dummy transport should be address explicitly. In that sense, I'd keep this issue blocked until #906 is finished, implemented tests to make sure wasm works (with dummy transport), and only then close this one.

@johanbrandhorst
Copy link
Contributor

@dalu gRPC-Web is production ready. See https://grpc.io/blog/state-of-grpc-web/ for more information. Go WASM is not production ready, however.

@johanbrandhorst
Copy link
Contributor

This isn't really the place for this discussion, but many companies are already using it in production, and it does not require a custom nginx module anymore. I'd be happy to discuss any questions you might have about it (I am the author of the blog post on the official gRPC blog), if you want to learn more. You can find me under @jbrandhorst on Gophers slack or on twitter.

@stale
Copy link

stale bot commented Sep 6, 2019

This issue is labeled as requiring an update from the reporter, and no update has been received after 7 days. If no update is provided in the next 7 days, this issue will be automatically closed.

@stale stale bot added the stale label Sep 6, 2019
@dfawley dfawley removed the stale label Sep 6, 2019
@tarndt
Copy link

tarndt commented Dec 20, 2019

I ran into this problem and created a solution called wasmws! Hopefully this is useful to you guys too. Its pretty young so PRs/testing is very welcome. 😸

My solution was to send the HTTP/2 gRPC traffic over a websocket rather than over TCP.

The result end up looking like fairly normal gRPC usage:
client:

const websocketURL = "ws://yourserver/grpc-proxy"
conn, err := grpc.DialContext(dialCtx, "passthrough:///"+websocketURL, grpc.WithContextDialer(wasmws.GRPCDialer), grpc.WithTransportCredentials(creds))

server:

wsl := wasmws.NewWebSocketListener(appRootCtx)
router.HandleFunc("/grpc-proxy", wsl.HTTPAccept)
err := grpcServer.Serve(wsl)

@johanbrandhorst
Copy link
Contributor

This is a really cool use of the Go grpc interfaces, but I feel obliged to point out that Denys Smirnov did it first: https://github.com/dennwc/dom/tree/master/examples/grpc-over-ws. Well done regardless!

@tarndt
Copy link

tarndt commented Dec 21, 2019

Thanks @johanbrandhorst I didn't realize that existed.

but I feel obliged to point out that Denys Smirnov did it first

Now I feel silly, I'll have to read his implementation and compare them. Nothing like feeling like you did something useful and then realizing you wasted your time 😿 .

@johanbrandhorst
Copy link
Contributor

Now I feel silly, I'll have to read his implementation and compare them. Nothing like feeling like you did something useful and then realizing you wasted your time 😿 .

Don't be discouraged! It is a really clever use of interfaces, it wasn't my intention to put you down. I'm sure you learned a lot while playing around with it, and time learning is never wasted 😄.

@tarndt
Copy link

tarndt commented Dec 23, 2019

time learning is never wasted

True true. And a lesson to do a better job Googling before getting started lol.

Looking closer at the implementations are they are quite different under the hood. I need to run some proper benchmarks but at a glance I would assume:

  • Code that depends on deadlines will not work with Mr. Smirnov's implementation (deadlines are TODO).
  • Mr. Smirnov's implementation will perform slightly better in general cases, less work per read.
  • Mr. Smirnov's implementation will consume significantly more memory in situations when large messages have been received (as used buffer is never released post op).

Talking a closer look, I think that I will update my implementation be a hybrid using his strategy for common cases (better performance) and retaining mine for edge cases (better worst case handling). This will make things more complex, but software engineering... always about the tradeoffs... 😸

P.S. I do need to go real benchmarking to justify the above assumptions first.

@logicethos
Copy link

Just want to add, this is a sorely missing feature in Q3 2020. I wrote my Go WASM app expecting grpc to just work. I wish I had checked first.

@armin-th
Copy link

This is a really cool use of the Go grpc interfaces, but I feel obliged to point out that Denys Smirnov did it first: https://github.com/dennwc/dom/tree/master/examples/grpc-over-ws. Well done regardless!

Question for the issue moderators, Is this not the correct way about it or are you keeping this issue open because the quoted example has limitations I'm not aware of?

@johanbrandhorst
Copy link
Contributor

Using websockets as a net.Conn introduces a lot of overhead, and a proper solution would require a pluggable transport interface that would allow the design of a transport that takes advantage web-specific technologies, for example, the grpcweb protocol. This would also allow go Wasm frontends to be compatible with any grpcweb backend.

@menghanl
Copy link
Contributor

menghanl commented May 3, 2021

Closing this issue. Custom transport support is tracked in #906

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
P3 Status: Blocked Type: Feature New features or improvements in behavior
Projects
None yet
Development

No branches or pull requests

9 participants