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

L37: Go Custom Transports #103

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

dfawley
Copy link
Member

@dfawley dfawley commented Sep 20, 2018

No description provided.

@dfawley
Copy link
Member Author

dfawley commented Sep 20, 2018

cc @ejona86 @markdroth

Copy link
Member

@jhump jhump left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exciting!


This would have a simpler API, however, it would require each `ClientConn` to
use only a single transport. The proposed design enables a `ClientConn` with
different types of connections.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value of this is unclear. Under what cases would a single connection want different types of connection?

The only thing I can imagine might be to provide both in-process and remote connections for a service that is exposed both locally and by the server's peers. But that would only be useful if there were a rich API for affinity -- such that some requests (based on affinity keys) might be routed locally over an in-process transport and others could be routed to a remote server. But that could also easily be implemented as a "composite channel" abstraction -- where a channel delegates to a ClientConn with in-process connections for some requests but to another ClientConn for others.

Perhaps a simpler API would be more valuable?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it would be normal for any one ClientConn to support multiple types of transports (though it would be possible). I do think it's normal to do something like grpc.Dial(someStringIGotFromTheUser, ...), and want to have multiple transports ready to handle the resolved addresses whether they are tcp or UDS or in-process or other address types. This proposal provides this flexibility, whereas directly injecting a single transport into a client would require the caller to know ahead of time which transport needs to be used.

hide HTTP/2 semantics and grpc wire protocol semantics).
1. Transports should be message-based, not byte-based, to facilitate an
in-process transport that uses `proto.Clone()` for efficiency.
- Note that retry support requires byte-based support from all transports,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious about this constraint. Could you point to what in the streaming API imposes this constraint?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Retry for streaming RPCs necessitates the ability to replay multiple sent messages.

Spec: https://github.com/grpc/proposal/blob/master/A6-client-retries.md#memory-management-buffering
grpc-go's buffer: https://github.com/grpc/grpc-go/blob/master/stream.go#L361

In the Go streaming API, grpc-go does not acquire ownership of the messages when users pass them to SendMsg. The only way we can safely reference the messages after returning from SendMsg is if we serialize them first.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I'm still a little confused. If the transport copies the message (like as done in an in-process transport), then it owns its copy and does not need to resort to serialization to support retries. What am I missing?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I think I get it. The issue is that the stream abstraction for the call is at a different layer than the stream abstraction for the transport. So the message will come into the call layer, and then need to be serialized in order to be cached for retries, before it gets relayed to the transport-layer (which could happen multiple times, like when the call is actually retried).

It think there must be an alternate solution to this, especially since we're talking about a brand new API (i.e. if designed right, seems like it should be able to facilitate the desired behavior: caching messages in a safe way without requiring the call/channel layer to serialize to bytes first).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is that the stream abstraction for the call is at a different layer than the stream abstraction for the transport

Right. The transport stream only handles a single attempt of the RPC. Even if it were to make a copy of the message, we will kill the stream it when it fails, so the copy would be lost. It's possible/desired to use a different transport (even if it's the same type of transport) when we retry.

It think there must be an alternate solution to this, especially since we're talking about a brand new API

The new API is only between grpc and the transport. The problem comes about because of the existing API between the user and grpc.

Another option could be to check for and use a Clone(interface{}) interface{} method on the marshaller when caching messages for retry. However, we can't require this, to preserve backward compatibility. So I think the transport would still either need to support byte-based transmission (which seems pretty trivial to implement) or just fail the stream when a serialized message is encountered.

This has made me notice that I am missing a way to access the raw message passed to Send/Recv from inside the transport. I believe a method on IncomingMessage and OutgoingMessage to access it (e.g. RawMessage() interface{}) would be the obvious way to do that.

dfawley added a commit to dfawley/grpc that referenced this pull request Oct 25, 2018
When security is disabled, not waiting for the HTTP/2 handshake can lead to
DoS-style behavior.  For details, see:
grpc/grpc-go#954.  This requirement will incur an
extra half-RTT latency before the first RPC can be sent under plaintext, but
this is negligible and unencrypted connections are rarer than secure ones.

Under TLS, the server will effectively send its part of the HTTP/2 handshake
along with its final TLS "server finished" message, which the client must wait
for before transmitting any data securely.  This means virtually no extra
latency is incurred by this requirement.

Go had attempted to separate "connection ready" with "connection successful"
(Issue: grpc/grpc-go#1444 PR:
grpc/grpc-go#1648).  However, this is confusing to
users and introduces an arbitrary distinction between these two events.  It has
led to several bugs in our reconnection logic (e.g.s
grpc/grpc-go#2380,
grpc/grpc-go#2391,
grpc/grpc-go#2392), due to the complexity, and it makes
custom transports (grpc/proposal#103) more difficult
for users to implement.

We are aware of some use cases (in particular,
https://github.com/soheilhy/cmux) expecting the behavior of transmitting an RPC
before the HTTP/2 handshake is completed.  Before making behavior changes to
implement this, we will reach out to our users to the best of our abilities.
@asim
Copy link

asim commented Feb 9, 2019

If you're looking for inspiration around pluggable transports, clients, servers, etc you should take a look at go-micro https://github.com/micro/go-micro. We support the ability to do grpc as codec, client/server and transport.

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

Successfully merging this pull request may close these issues.

4 participants