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-Web client and server #695

Merged
merged 8 commits into from Jan 9, 2020
Merged

gRPC-Web client and server #695

merged 8 commits into from Jan 9, 2020

Conversation

@JamesNK
Copy link
Member

JamesNK commented Dec 14, 2019

Fixes #99

Implementation of https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-WEB.md for ASP.NET Core server and .NET Core gRPC client.

@JamesNK JamesNK mentioned this pull request Dec 14, 2019
@JamesNK JamesNK requested review from jtattermusch and JunTaoLuo Dec 17, 2019
Copy link
Contributor

JunTaoLuo left a comment

Still taking a look at the base64 pipereader/writer and request/responsestream

@brmdbr

This comment has been minimized.

Copy link

brmdbr commented Dec 30, 2019

I've build it from the sources to use in my Blazor client side app and I get the following error: WASM: Status(StatusCode=Internal, Detail="Error starting gRPC call: Cannot invoke method because it was wiped. See stack trace for details.")

Is this a problem related to this pr or am I implementing something wrong?

@JamesNK

This comment has been minimized.

Copy link
Member Author

JamesNK commented Dec 30, 2019

Instead of passing HttpClientHandler to GrpcWebHandler, create WasmHttpMessageHandler. There seems to be a bug in Blazor that its not public. You can do this:

var wasmHttpMessageHandlerType = Assembly.Load("WebAssembly.Net.Http").GetType("WebAssembly.Net.Http.HttpClient.WasmHttpMessageHandler");
var wasmHttpMessageHandler = (HttpMessageHandler)Activator.CreateInstance(wasmHttpMessageHandlerType);

dotnet/aspnetcore#17115

@SteveSandersonMS

@JamesNK JamesNK force-pushed the JamesNK:jamesnk/grpcweb4 branch from ed061a8 to 9b76a0b Dec 30, 2019
@JamesNK JamesNK force-pushed the JamesNK:jamesnk/grpcweb4 branch from 9b76a0b to 81fc1b1 Dec 30, 2019
@brmdbr

This comment has been minimized.

Copy link

brmdbr commented Dec 30, 2019

Thanks @JamesNK, that seems to work, I no longer get the cannot invoke error. I get another error now: WASM: Status(StatusCode=Internal, Detail="Error starting gRPC call: Buffer is not large enough for header")

Using this code:

var wasmHttpMessageHandlerType = Assembly.Load("WebAssembly.Net.Http").GetType("WebAssembly.Net.Http.HttpClient.WasmHttpMessageHandler");
var wasmHttpMessageHandler = (HttpMessageHandler)Activator.CreateInstance(wasmHttpMessageHandlerType);
var httpClient = new HttpClient(new GrpcWebHandler(GrpcWebMode.GrpcWebText, wasmHttpMessageHandler));
var channel = GrpcChannel.ForAddress("https://localhost:5001", new GrpcChannelOptions
            {
                HttpClient = httpClient
            });
var client = new Protos.LessonService.LessonServiceClient(channel);
var lesson = await client.GetLessonAsync(new Protos.GetLessonRequest { AllowedChars = "asdfghjkl;qwertyuiop", NumberOfWords = 15, Seed = 1337 });

Any idea on this? Again I'm not sure if its related to the pr as I'm implementing in a similar way as the tests in the .web projects.

@JamesNK

This comment has been minimized.

Copy link
Member Author

JamesNK commented Dec 30, 2019

Ohh, I forgot that I modified the client slightly.

You'll need to also compile and use Grpc.Net.Client.

@brmdbr

This comment has been minimized.

Copy link

brmdbr commented Jan 1, 2020

Ohh, I forgot that I modified the client slightly.

You'll need to also compile and use Grpc.Net.Client.

That fixed it! Thanks a lot.

@SteveSandersonMS

This comment has been minimized.

Copy link

SteveSandersonMS commented Jan 2, 2020

Instead of passing HttpClientHandler to GrpcWebHandler, create WasmHttpMessageHandler. There seems to be a bug in Blazor that its not public

Thanks - will be fixed with dotnet/blazor#1960 (as in, HttpClientHandler will just work out of the box and you won't have to do any weird stuff involving WasmHttpMessageHandler).

Copy link
Contributor

JunTaoLuo left a comment

Some comments for the request/responsestreams. I'll finish up with the pipewriter/reader tomorrow.

Copy link
Contributor

JunTaoLuo left a comment

Looks good but I'd like for pipe experts to take a look at the Base64PipeWriter and Base64PipeReader as well. @halter73 @jkotalik

Copy link
Contributor

JunTaoLuo left a comment

@halter73 @jkotalik can you guys take a look at the pipereader/writer logic as well?

JamesNK added 2 commits Jan 7, 2020
Copy link
Contributor

jtattermusch left a comment

Initial set of comments, overall things are looking good but let's discuss the next steps in our sync-up.
Sorry for the delay.

src/Grpc.AspNetCore.Web/README.md Outdated Show resolved Hide resolved
kokoro/build_nuget.sh Outdated Show resolved Hide resolved
src/Grpc.AspNetCore.Web/README.md Show resolved Hide resolved
src/Grpc.AspNetCore.Web/README.md Outdated Show resolved Hide resolved
src/Grpc.AspNetCore.Web/Internal/GrpcWebMode.cs Outdated Show resolved Hide resolved
/// Written to as closely as possible mirror the behaviour of the C++ implementation in grpc/grpc-web:
/// https://github.com/grpc/grpc-web/blob/92aa9f8fc8e7af4aadede52ea075dd5790a63b62/net/grpc/gateway/examples/echo/echo_service_impl.cc
/// </summary>
public class EchoService : Grpc.Gateway.Testing.EchoService.EchoServiceBase

This comment has been minimized.

Copy link
@jtattermusch

jtattermusch Jan 8, 2020

Contributor

looks like you have a copy of the same service at testassets/FunctionalTestsWebsite/Services/EchoService.cs?

This comment has been minimized.

Copy link
@JamesNK

JamesNK Jan 8, 2020

Author Member

Yes. That's so it is tested by CI

Copy link
Contributor

jtattermusch left a comment

This is good enough as a preview implementation of the grpc-web functionality. LGTM, thanks for incorporating all the comments.

This will be in the next release as preview for people to test it out and provide feedback/report issues, but there won't be "stable" nuget packages yet.

The next steps and release criteria for official release will be driven by grpc/proposal#169.

@JamesNK JamesNK merged commit bee59db into grpc:master Jan 9, 2020
5 checks passed
5 checks passed
EasyCLA EasyCLA check passed. You are authorized to contribute.
Details
Interop Tests Kokoro build finished
Details
Nuget Build Kokoro build finished
Details
cla/linuxfoundation JamesNK authorized
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@JamesNK JamesNK deleted the JamesNK:jamesnk/grpcweb4 branch Jan 9, 2020
@JamesNK

This comment has been minimized.

Copy link
Member Author

JamesNK commented Jan 9, 2020

🎈 🎉 🍰

@guardrex guardrex mentioned this pull request Jan 18, 2020
@johanbrandhorst

This comment has been minimized.

Copy link

johanbrandhorst commented Jan 23, 2020

Hi, great work @JamesNK. Would you be interested in adding this implementation to the grpc-web compatibility tests I'm running? See https://github.com/johanbrandhorst/grpc-web-compatibility-test

@muraray

This comment has been minimized.

Copy link

muraray commented Feb 10, 2020

That's one hell of a serious pull request... grpc port to .net core paradigm, very much appreciate your work, thanks a ton... looking forward using it in our next project...

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

Successfully merging this pull request may close these issues.

10 participants
You can’t perform that action at this time.