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.

Sign up for GitHub

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

Jump to bottom

Support streaming? #1

Closed
akshayjshah opened this issue Aug 3, 2021 · 2 comments
Closed

Support streaming? #1

akshayjshah opened this issue Aug 3, 2021 · 2 comments
Labels
question Further information is requested

Comments

@akshayjshah
Copy link
Member

akshayjshah commented Aug 3, 2021 •
edited
Loading

Right now, reRPC only exposes support for unary RPCs. However, it wouldn't be terribly difficult to support client, server, and bidirectional streaming - under the hood, NewReflectionHandler already uses bidi streaming.

Advantages of streaming:

  • Support a wider variety of use cases.
  • Users wouldn't need to migrate to grpc-go just to call or serve one streaming method, so building with reRPC becomes a safer bet.

Disadvantages of streaming:

  • We'd be using net/http in a somewhat unusual way. I'd expect more difficult-to-diagnose bugs.
  • reRPC's surface area would grow quite a bit. We'd need streaming equivalents for Func and Interceptor, and we might even need separate interfaces for each streaming type (like grpc-go).
  • Twirp doesn't support streaming: Proposal: Support streaming in the Twirp spec twitchtv/twirp#3. Most of the discussion on that issue focuses on the JSON streaming format, but we'd also probably want a framing mechanism for binary protobuf and a way to communicate errors mid-stream (after the HTTP status code has already been sent). For all its complexity, the gRPC specification solves these problems well.
  • Client and server streaming will work over HTTP/1.1, but bidi requires HTTP/2. With the lack of Twirp support above, this makes reRPC's compatibility guarantees harder to understand.

I lean toward keeping reRPC unary-only, but I'm leaving this issue open for discussion. I'm particularly interested in talking to anyone willing to use reRPC streaming in production.
@akshayjshah akshayjshah added the question Further information is requested label Aug 3, 2021
akshayjshah added a commit that referenced this issue Aug 10, 2021
@akshayjshah
Rework handler and client internals into streams
d6d22c7 
This is a very large commit that reworks the internals of handlers and
clients to operate on streams. Apart from a few changes to NewHandler
and NewClient, the tests continue to pass as-is. Adding support for
streaming RPCs should now be easy: we're just generating type-safe
wrappers around the generic stream type.

The surface area of the change is relatively small: we add a Stream
interface, Func equivalents for client- and server-side streams, and two
helper functions to pull Interceptors out of Options.

There are some distinct rough edges left:

* I haven't yet added interceptor support for streams.
* The client type should probably be reduced to a single-shot call.
  Though we'll still expose clients from generated code, they're doing
  very little beyond holding options.
* The client-side stream implementation is overly complex.
* Hand-writing unary handlers is now a fair bit of work, so we should
  move the health-checking support into a subpackage so we can use
  generated code without import cycles.
* Once we can generate bidi streaming handlers, we should consider
  moving reflection support into a separate package too.

This begins to address #1.
@akshayjshah
Copy link
Member Author

Pushed a bit more progress tonight: we're now generating the server-side code to support all three streaming RPC types. Hopefully I'll get the client-side code and tests done in the next few days.

akshayjshah added a commit that referenced this issue Aug 13, 2021
@akshayjshah
Generate client-side streaming code
9550b1a 
Generate the client-side streaming methods and types. Now that we've got
both sides set up, also add some reRPC-on-reRPC tests. (We'll add
cross-tests in a future PR.)

This is a big step closer to solving #1.
@akshayjshah
Copy link
Member Author

Done! reRPC now supports client, server, and bidirectional streaming, and cross-tests show that we're compatible with grpc-go. (We should probably exercise more corner cases in tests, though.)

Adding streaming didn't expand the API as much as I feared. We needed:

  • A Stream interface, which also serves as a pretty good transport abstraction for the generated code.
  • A StreamFunc interface, analogous to Func.
  • An additional method in the Interceptor interface.
  • A set of StreamType constants.

I'm still pretty close to the code, but the APIs feel pretty narrow to me.

akshayjshah added a commit that referenced this issue Feb 28, 2022
@akshayjshah
Rework handler and client internals into streams
27d4be4 
This is a very large commit that reworks the internals of handlers and
clients to operate on streams. Apart from a few changes to NewHandler
and NewClient, the tests continue to pass as-is. Adding support for
streaming RPCs should now be easy: we're just generating type-safe
wrappers around the generic stream type.

The surface area of the change is relatively small: we add a Stream
interface, Func equivalents for client- and server-side streams, and two
helper functions to pull Interceptors out of Options.

There are some distinct rough edges left:

* I haven't yet added interceptor support for streams.
* The client type should probably be reduced to a single-shot call.
  Though we'll still expose clients from generated code, they're doing
  very little beyond holding options.
* The client-side stream implementation is overly complex.
* Hand-writing unary handlers is now a fair bit of work, so we should
  move the health-checking support into a subpackage so we can use
  generated code without import cycles.
* Once we can generate bidi streaming handlers, we should consider
  moving reflection support into a separate package too.

This begins to address #1.
akshayjshah added a commit that referenced this issue Feb 28, 2022
@akshayjshah
Generate client-side streaming code
dc72cf6 
Generate the client-side streaming methods and types. Now that we've got
both sides set up, also add some reRPC-on-reRPC tests. (We'll add
cross-tests in a future PR.)

This is a big step closer to solving #1.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
question Further information is requested
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@akshayjshah