Skip to content

Conversation

@glbrntt
Copy link
Collaborator

@glbrntt glbrntt commented Jun 10, 2020

Motivation:

At the moment the four client calls {Client,Server,Bidirectional}Streaming
and Unary all rely on a base call class. It provides things like the
stream channel setup and a place to hold on to the response part
futures.

It also relies on having two different handler in the stream
channel; one for writing requests and another for responses. There are
two variants for of each of these handlers; unary and streaming.
This meant that there was state more or less all over the place. As an
example, timeouts must be adhered to even if the channel never comes up
yet the scheduled timeout task lived in the response handler(s) since it
needs to be able to fail response part promises.

The base call also makes it difficult to use something other than a
Channel for transport, which might be useful for in-process testing, or
generating client calls where the requests may be recorded and the
responses injected by the caller.

Creating a "gap" between the call and the channel may also lend itself
to interceptors and a lower-level API in the future.

Modifications:

BaseClientCall no longer exists, and each of the four call objects holds
on to a ChannelTransport object. This sits between the call and a
channel. It will buffer writes until it has been 'activated' (by the
underlying channel). It also holds on to the response part promises as
well as the timeout for the RPC. The four channel handlers have been
replaced by a single inbound handler which just forwards events to the
transport.

Some tests were rewritten to not use the deleted code.

Result:

  • Code is more maintainable
  • More visibility into the call state when something goes wrong (i.e.
    did we have a channel when we timed out?)
  • Provides an opportunity to add interceptors/lower level API

Motivation:

At the moment the four client calls {Client,Server,Bidirectional}Streaming
and Unary all rely on a base call class. It provides things like the
stream channel setup and a place to hold on to the response part
futures.

It also relies on having two different handler in the stream
channel; one for writing requests and another for responses. There are
two variants for of each of these handlers; unary and streaming.
This meant that there was state more or less all over the place. As an
example, timeouts must be adhered to even if the channel never comes up
yet the scheduled timeout task lived in the response handler(s) since it
needs to be able to fail response part promises.

The base call also makes it difficult to use something other than a
Channel for transport, which might be useful for in-process testing, or
generating client calls where the requests may be recorded and the
responses injected by the caller.

Creating a "gap" between the call and the channel may also lend itself
to interceptors and a lower-level API in the future.

Modifications:

BaseClientCall no longer exists, and each of the four call objects holds
on to a `ChannelTransport` object. This sits between the call and a
channel. It will buffer writes until it has been 'activated' (by the
underlying channel). It also holds on to the response part promises as
well as the timeout for the RPC. The four channel handlers have been
replaced by a single inbound handler which just forwards events to the
transport.

Some tests were rewritten to not use the deleted code.

Result:

- Code is more maintainable
- More visibility into the call state when something goes wrong (i.e.
  did we have a channel when we timed out?)
- Provides an opportunity to add interceptors/lower level API
@glbrntt glbrntt added ⚠️ semver/major Breaks existing public API. nio labels Jun 11, 2020
@glbrntt glbrntt requested a review from Lukasa June 11, 2020 09:44
/// The options used to make the RPC.
public let options: CallOptions

/// The `Channel` user to transport messages for this RPC.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// The `Channel` user to transport messages for this RPC.
/// The `Channel` used to transport messages for this RPC.

self.transport.sendRequest(.end, promise: promise)
}

public func newMessageQueue() -> EventLoopFuture<Void> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, what's this for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's a requirement on the ClientCall protocol; I think it was added right at the start as a way to chain sending messages: rpc.newMessageQueue().flatMap { rpc.sendMessage(...) } etc., I don't think we've reconsidered its existence though since it clearly doesn't make a lot of sense!

let trailingMetadataPromise: EventLoopPromise<HPACKHeaders>

/// A promise for the call status.
let statusPromise: EventLoopPromise<GRPCStatus>
Copy link
Collaborator

Choose a reason for hiding this comment

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

How useful are these promises? Is it more useful for this to be some kind of builder? I'm a little worried about the allocation pattern here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Status is probably the most useful, but you're right, the metadata promises are unlikely to be used nearly as much. I'll update this.

/// ```
/// var queue = call.newMessageQueue()
/// for message in messagesToSend {
/// queue = queue.then { call.sendMessage(message) }
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

then -- must be old!

@glbrntt glbrntt merged commit 937d85a into grpc:master Jun 16, 2020
@glbrntt glbrntt deleted the gb-call-backend branch June 16, 2020 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

⚠️ semver/major Breaks existing public API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants