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

Support grpc-web in grpc-dart #109

Merged
merged 35 commits into from Mar 15, 2019

Conversation

Projects
None yet
@fuzzybinary
Copy link
Contributor

fuzzybinary commented Aug 5, 2018

This is a work in progress, and I haven't gone down the path of implementing grpc-web yet, but I wanted to get some feedback on this refactor while I work on a GrpcWebTransport.

Depending on feedback, I can abandon this PR and create another once that transport is done.

One thing that needs to be done is there are no unit tests for the Http2Transport, only generalized transport.

Refactors out http2 dependencies into a standalone Transport
This refactor should allow us to create a gRpc-web transport in the client as well.
@thelinuxfoundation

This comment has been minimized.

Copy link
Collaborator

thelinuxfoundation commented Aug 5, 2018

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

Regards,
CLA GitHub bot

@googlebot googlebot added the cla: yes label Aug 5, 2018

@listepo-alterpost

This comment has been minimized.

Copy link

listepo-alterpost commented Aug 9, 2018

Hi, any news?

@fuzzybinary

This comment has been minimized.

Copy link
Contributor Author

fuzzybinary commented Aug 9, 2018

@listepo-alterpost I've signed the CLA (not sure how often that updates). I've started work on gRPC-Web in a personal branch but no updates yet. Was hoping to get some feedback from someone on whether this is a good approach.

@kevmoo do you know someone who might be good to reach out to?

[WIP] Add initial gRPC-Web support
This is not fully tested and needs work but is an initial step in moving forward with gRPC-Web support in dart.
@fuzzybinary

This comment has been minimized.

Copy link
Contributor Author

fuzzybinary commented Aug 15, 2018

To update this (and update #43) I now have very basic gRPC-Web support working, but I have not tried anything but the simplest example in this branch
image

I'd like to get feature parity with the echo example before this can be merged in, but I'm leaving the PR open for feadback.

@fuzzybinary fuzzybinary changed the title Refactor Http2 dependencies into a "Transport" interface to eventually support grpc-web [WIP] Support grpc-web in grpc-dart Aug 15, 2018

@listepo-alterpost

This comment has been minimized.

Copy link

listepo-alterpost commented Aug 15, 2018

@fuzzybinary good job 👍

@gedw99

This comment has been minimized.

Copy link

gedw99 commented Aug 15, 2018

Using this necessitates TLS 1.3 ?

@fuzzybinary

This comment has been minimized.

Copy link
Contributor Author

fuzzybinary commented Aug 15, 2018

@gedw99 The HTTP2 stuff might but I don't really know. I haven't tested the gRPC-Web transport against https yet, so it could just fail outright. I'll handle that once I write / fix the unit tests.

@kevmoo

This comment has been minimized.

Copy link
Contributor

kevmoo commented Aug 15, 2018

fuzzybinary added some commits Aug 16, 2018

Additional functional fixes
Fix tests by addiing conditional import
Close the stream properly by checking the state of the HttpRequest
Add Streaming serve rresponse sample (not 100% working)
@nichite
Copy link
Contributor

nichite left a comment

Overall, I think the refactoring into a Transport interface is a fine approach. There's an open question as to how we want to deal with methods that you don't really need in the XHR context since you're not managing a connection in the same way.

@@ -0,0 +1,62 @@
# Description
The grpc-web example shows how to use the Dart gRPC library with a gRPC-Web capible server.

This comment has been minimized.

@nichite

nichite Aug 20, 2018

Contributor

nit: typo ("capable").

ActiveStateHandler onActiveStateChanged;
SocketClosedHandler onSocketClosed;

Future<Null> connect();

This comment has been minimized.

@nichite

nichite Aug 20, 2018

Contributor

nit: prefer Future<void> to Future<Null>


@override
void terminate() {
// TODO: implement terminate

This comment has been minimized.

@nichite

nichite Aug 20, 2018

Contributor

Other than closing the incoming / outgoing streams, is there anything else to do here? My impression was that since we're just delegating to XHR we don't need to do the same cleanup. Maybe call abort() if the request is in flight?

@override
int get id => null;

// TODO: implement incomingMessages

This comment has been minimized.

@nichite

nichite Aug 20, 2018

Contributor

Are these TODOs still needed?

.map(GrpcHttpEncoder.frame)
.listen((data) {
//final encoded = base64Encode(data);
_request.send(data);

This comment has been minimized.

@nichite

nichite Aug 20, 2018

Contributor

One thing that's not clear from wrapping the xhr HttpRequest like this: what's the behavior of client / server / bi-directional streaming cases through this transport type? It looks like in client / directional cases, just send the first request?

This comment has been minimized.

@fuzzybinary

fuzzybinary Aug 21, 2018

Author Contributor

This gets at some of the other comments about the missing implementations of the other calls.

I might need to find a way to determine whether we're making a single call or a request for bi-directional streaming which would require WebSocket support, not an XHR call. At the moment, the XHR should support the single RPC call and a server stream (although it's buggy at the moment) but not client stream or bi-directional stream.

Apparently client / bi-directional streams are in some gRPC-Web implementaitons, so I just have to figure out how to port them.

@fuzzybinary

This comment has been minimized.

Copy link
Contributor Author

fuzzybinary commented Aug 21, 2018

Thanks for the review - I'll fix some of the nits and the dartfmt issues sometime this week and continue to look into unit tests for the transports, fixing up some of the bugs and looking into websocket support.

@mit-mit mit-mit requested a review from sigurdm Aug 23, 2018

@fuzzybinary

This comment has been minimized.

Copy link
Contributor Author

fuzzybinary commented Aug 23, 2018

Looks like I might have to switch away from HttpRequest to the package:html Request / StreamedResponse APIs in order to support server side streaming. This also may be easier to transition to WebSockets later on down the road.

@gedw99

This comment has been minimized.

Copy link

gedw99 commented Aug 24, 2018

@fuzzybinary.

This might help as it's a 100% golang grpc / grpc-web server than also produces typescript clients.

https://github.com/improbable-eng/grpc-web

I use this currently and it's been stable for over a year.

It might help answer some of the questions about approaches to bi-directional streaming.

@gedw99

This comment has been minimized.

Copy link

gedw99 commented Aug 24, 2018

I avoided this one due to many dependencies of nginx etc.

https://github.com/grpc/grpc-web

Fix server stream by supporting progress event.
Use ‘overrideMimeType’ to support getting data on progress events so we can supply messages before the stream gets closed.
Update all examples to dart 2 libraries so dartanalyzer doesn’t complain
Fix nits from code review.
@fuzzybinary

This comment has been minimized.

Copy link
Contributor Author

fuzzybinary commented Aug 24, 2018

@gedw99 Awesome thanks! I'd actually looked at that repo before but for some reason it didn't register that their TS is much cleaner. Reading it unblocked me on getting the XHR support working for server side streaming, so now the echo example works.

Thanks!

@gedw99

This comment has been minimized.

Copy link

gedw99 commented Aug 24, 2018

@fuzzybinary great ! Yeah i can see what you mean that seeing it as Typescript is much easier to understand whats going on.

I have been using that lib with GRPC dart and had no issues btw.
It woudl be great if GRPC-web Dart and GRPC-web golang are interchangeable.
For example i tend to want to run golang on the server and dart on the clients.
Some others might want to run Dart on the server and client.

t would be super productive is this is possible.

@fuzzybinary

This comment has been minimized.

Copy link
Contributor Author

fuzzybinary commented Aug 24, 2018

That should be 100% possible provided everything conforms to standard. The grpc-dart server should work with any grpc-web client provided there's a grpc-web proxy in front. Similarly, the grpc-dart web client should work with any grpc server provided there's a grpc-web proxy in front.

That said, only the improbable-eng go proxy has experimental support for WebSockets at the moment, and I don't know if that's going to enter the standard or not.

@gedw99

This comment has been minimized.

Copy link

gedw99 commented Aug 24, 2018

@fuzzybinary websockets can be turned off with this feature flag.

improbable-eng/grpc-web@2cf3019

fuzzybinary and others added some commits Aug 30, 2018

Add simple http2 transport tests.
Refactored a bit to prepare to do a WebSocket transport.
[broken] Adding initial tests for http2 transport and xhr.
Currently broken - committing to diagnose a build_runner issue.
@stt106

This comment has been minimized.

Copy link

stt106 commented Dec 3, 2018

This is great for dart-web!

@sigurdm

This comment has been minimized.

Copy link
Contributor

sigurdm commented Dec 18, 2018

This has not been completely forgotten - just many other things are going on.
I plan to put some effort into getting this landed early q1.

@fuzzybinary

This comment has been minimized.

Copy link
Contributor Author

fuzzybinary commented Dec 18, 2018

Let me know if there's anything I can do to help!

@bitsdominicada

This comment has been minimized.

Copy link

bitsdominicada commented Jan 22, 2019

Let me know if there's anything I can do to help!

Hi!! how can implement a Dart Backend Server behind Envoy proxy?

@sigurdm

This comment has been minimized.

Copy link
Contributor

sigurdm commented Jan 22, 2019

I started working on updating the generated code to not depend on dart:io.
That should enable us to do a cleaner split.

sigurdm and others added some commits Feb 7, 2019

WIP
@fuzzybinary

This comment has been minimized.

Copy link
Contributor Author

fuzzybinary commented Feb 28, 2019

@sigurdm Sorry for the delay - I've approved and merge your changes into my fork so this should be ready to go.

@supermuka

This comment has been minimized.

Copy link

supermuka commented Mar 8, 2019

I'm happy and I think many other Dart web enthusiasts are also seeing that soon we will have a published version of grpc-dart for web. As a suggestion if possible, in addition to the example\grpc-web example containing a node implementation we also have an example with Envoy + grpc implementation in Dart Server Side (vm). I'm not sure how to configure this yet!

@jonasfj
Copy link

jonasfj left a comment

This package exposes a lot of classes that most users don't really need, and this PR gets around changing some of them by making them abstract and concrete depending on how they are imported.
It might be better to simply make a major version bump and rename the concrete implementations instead of trying too hard to be backwards compatible.

I would suggest exposing the following libraries:

  • grpc_web.dart (expose GrpcWebClientChannel and option types needed to create it, if any)
  • grpc_io.dart (expose Http2ClientChannel and option types needed to create it, such as Http2ChannelOptions and Http2ChannelCredentials)
  • grpc.dart (export grpc_io.dart or grpc_web.dart using conditional imports)
  • service_api.dart (expose only abstract types needed in generated code)

It seems unwise to me that we expose concrete implementations of Client, ClientCall and Connection which are only used in generated code (thus, only needs to be visible from service_api.dart, and only needs to be exposed as pure interfaces). This PR seems to break compatibility because it changes these concrete implementations. Maybe it would be better to make them pure abstract interfaces and limit visibility to service_api.dart, and then do a major version bump.

Most of my suggestions here are perhaps unrelated to grpc-web support, and more to do with exposing too many concrete types, which makes adding grpc-web awkward to do without breaking compatibility. Granted we could argue that instantiating types like Connection was always unintended behavior, as this wasn't what the type was exposed for. Thus, breaking the constructor does not break backwards compatibility. Then most of the cleanup I've suggested could be done later (though it is peculiar to split the ClientChannel type in two types without breaking backwards compatibility).

import 'channel.dart';

@visibleForTesting
Future<Transport> connectXhrTransport(

This comment has been minimized.

@jonasfj

jonasfj Mar 11, 2019

You need to bump the SDK range from sdk: '>=2.0.0 <3.0.0' to sdk: '>=2.1.0 <3.0.0', or do an import 'dart:async' show Future; for this to work.

This not applies a few other places too..

@@ -21,23 +21,28 @@ import 'call.dart';
import 'connection.dart';
import 'method.dart';
import 'options.dart';
import 'transport/transport.dart';

typedef ConnectTransport = Future<Transport> Function(

This comment has been minimized.

@jonasfj

jonasfj Mar 11, 2019

Please document what this type is :)

export 'src/client/method.dart';
export 'src/client/options.dart';
export 'src/client/transport/transport.dart';
export 'src/client/web_channel.dart' show GrpcWebClientChannel;

This comment has been minimized.

@jonasfj

jonasfj Mar 11, 2019

So this is a general note on GrpcWebClientChannel, channel.ClientChannel and http2_channel.ClientChannel.

You've essentially split ClientChannel into two parts an abstract interface (channel.ClientChannel) and a concrete implementation (http2_channel.ClientChannel) for use with dart:io.

It seems to me that http2_channel.ClientChannel should be named Http2ClientChannel. But that we are unable to do that without breaking backwards compatibility.

We could add both http2_channel.ClientChannel and http2_channel.Http2ClientChannel, and then annotate http2_channel.ClientChannel as deprecated, and make it clear in documentation comments that this type is only present for compatibility...

But won't the generated code need to be updated? I feels sketchy to split a type in two... isn't it safer to bump the major version?


/// Enable TLS and optionally specify the [certificates] to trust. If
/// [certificates] is not provided, the default trust store is used.
const ChannelCredentials.secure(

This comment has been minimized.

@jonasfj

jonasfj Mar 11, 2019

Won't this break backwards compatibility in minor release? Perhaps it's better to break the public API, and then do it right...

If I'm reading it right this PR splits ChannelCredentials into Http2ChannelCredentials and ChannelCredentials, which is a breaking change.

I don't even understand why Http2ChannelCredentials needs to inherit from ChannelCredentials. I can't pass ChannelOptions to GrpcWebClientChannel anyways.

It seems to me that ChannelOptions and ChannelCredentials could both be renamed with an Http2 prefix, and they are only usable with the http2_channel.ClientChannel, is that not correct? (as previously mentioned, we could rename http2_channel.ClientChannel to http2_channel.Http2ClientChannel if we broken backwards compatibility).

With this it seems even less feasible to me to avoid breaking backwards compatibility.

}

void _onRequestError() {
// TODO: Implement errors on requests

This comment has been minimized.

@jonasfj

jonasfj Mar 11, 2019

Is this not critical?

@@ -68,22 +37,12 @@ class GrpcHttpEncoder extends Converter<GrpcMessage, StreamMessage> {
}
throw new GrpcError.internal('Unexpected message type');
}

static List<int> frame(List<int> payload) {

This comment has been minimized.

@jonasfj

jonasfj Mar 11, 2019

This breaks backwards compatibility of the public API... (Maybe we should just bump major version).

Note. I'm not even sure why GrpcHttpEncoder is a publicly exposed class.

dart:
- stable
- dev

# Define test tasks to run.
dart_task:
- test: --platform vm
- test: --platform chrome

This comment has been minimized.

@jonasfj

jonasfj Mar 11, 2019

I'm not sure if it's easy to do, but it might be worth testing with Firefox too..

I would love to add safari and edge, but I understand if those are hard to automate :)

}

if (_request.readyState == HttpRequest.HEADERS_RECEIVED) {
if (contentType.startsWith('application/grpc')) {

This comment has been minimized.

@jonasfj

jonasfj Mar 11, 2019

Are we completely unconcerned with the response status?

}

if (_request.readyState == HttpRequest.DONE) {
_incomingProcessor.close();

This comment has been minimized.

@jonasfj

jonasfj Mar 11, 2019

What if _request.onError happened? Perhaps the connection was broken... If this was a streaming response would we know the difference between a broken connection and a connection that closed because no more messages was supposed to arrive?


/// Used for idle and reconnect timeout, depending on [_state].
Timer _timer;
Duration _currentReconnectDelay;

ClientConnection(this.host, this.port, this.options);
ClientConnection(this.host, this.port, this.options, this.connectTransport);

This comment has been minimized.

@jonasfj

jonasfj Mar 11, 2019

Won't this break backwards compatibility?

It probably wasn't a good idea for ClientConnection to be public concrete type in the first place :)
Since we probably don't want to people to instantiate their own ClientConnection objects and return them from a custom ClientChannel implementation.

@jonasfj

This comment has been minimized.

Copy link

jonasfj commented Mar 11, 2019

@sigurdm, feel free to moderate/overrule my suggestions, and make an executive decision to land this..
I had one concern regarding xhr2 error handling, but otherwise it's mostly about the public API, and whether we can call this backwards compatible :)

Cleaning up the public API could be done separately. Even if you decide that this needs a major version bump.

@sigurdm

This comment has been minimized.

Copy link
Contributor

sigurdm commented Mar 15, 2019

Thanks a bunch for the review @jonasfj , and for all the patience @fuzzybinary. I will merge this now (but not publish yet), and do a follow up PR addressing the concerns.

@sigurdm sigurdm merged commit d586595 into grpc:master Mar 15, 2019

3 checks passed

cla/google All necessary CLAs are signed
cla/linuxfoundation fuzzybinary authorized
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@vjpai

This comment has been minimized.

Copy link
Member

vjpai commented Mar 16, 2019

This pull request is breaking interop tests at grpc/grpc . Errors seem to be of the form:

2019-03-16 16:01:13,286 file:///var/local/git/grpc-dart/lib/src/client/http2_channel.dart:24:1: Error: 'Future' expects 0 type arguments.
Future<Transport> connectTransport(
^
file:///var/local/git/grpc-dart/lib/src/client/http2_channel.dart:26:10: Error: A value of type '#lib1::Http2Transport' can't be assigned to a variable of type 'dart.async::FutureOr<invalid-type>'.
Try changing the type of the left hand side, or casting the right hand side to 'dart.async::FutureOr<invalid-type>'.
  return Http2Transport(host, port, options)..connect();
         ^
file:///var/local/git/grpc-dart/lib/src/client/http2_channel.dart:24:19: Error: Functions marked 'async' must have a return type assignable to 'Future'.
Future<Transport> connectTransport(
                  ^^^^^^^^^^^^^^^^
file:///var/local/git/grpc-dart/lib/src/client/http2_channel.dart:32:21: Error: The argument type '(dart.core::String, dart.core::int, #lib1::ChannelOptions) → invalid-type' can't be assigned to the parameter type '(dart.core::String, dart.core::int, #lib1::ChannelOptions) → dart.async::Future<#lib2::Transport>'.
Try changing the type of the parameter, or casting the argument to '(dart.core::String, dart.core::int, #lib1::ChannelOptions) → dart.async::Future<#lib2::Transport>'.
      : super(host, connectTransport, port: port, options: options);
                    ^

Any ideas of what we can do here? @jtattermusch @nicolasnoble

@sigurdm

This comment has been minimized.

Copy link
Contributor

sigurdm commented Mar 18, 2019

Yes - this is landed incomplete because it was becoming too difficult to manage. But the error you see is because your dart SDK is before v. 2.1

@sigurdm

This comment has been minimized.

Copy link
Contributor

sigurdm commented Mar 18, 2019

I will move this to a branch, and finish it there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.