Adds a JSON/SSE transport for streaming endpoints to the http channel.#84
Adds a JSON/SSE transport for streaming endpoints to the http channel.#84kellegous wants to merge 3 commits intofullstorydev:masterfrom
Conversation
This is primarily intended to enable a streaming response from the server to a browser-based client. However, this also adds the functionality to the client to make it possible to properly test the server.
httpgrpc/client.go
Outdated
| BaseURL *url.URL | ||
| Transport http.RoundTripper | ||
| BaseURL *url.URL | ||
| ContentType ContentType |
There was a problem hiding this comment.
BTW, I didn't really want to make this part of the public interface but the best way to test the server was to have the client speak the same language. I then tried to find a way to enable the JSON support in tests, but I couldn't come up with a clever way to keep it all private but also enable the testing.
There was a problem hiding this comment.
seems reasonable to just expose it? not sure how else you would select an internal protocol... I guess you were thinking maybe not support this protocol for Go clients, only Go servers? IDK seems fine to me.
There was a problem hiding this comment.
I'm not a fan of this mechanism because it feels counter-intuitive that setting this to JSON would in turn trigger a "text/event-stream" content type in the response. Also, this doesn't provide us much flexibility for changes/improvements in the future. Also, I think use of JSON is different from SSE support -- that feels like two different settings. In fact, I thought at FS there was already usage of this HTTP-over-gRPC package with JSON, for web clients. Is it really safe to assume all clients that use JSON will understand SSE streaming responses?
FWIW, I started a branch a couple of years ago (but never pushed or opened a pull request) that added support for the ConnectRPC and gRPC-Web protocols to this package. The intent was to allow users of the grpc-go library an easy way to interop with browsers that were using connect-web. The approach I took in that branch:
- Add a
NewChannelfactory function that accepts variadic options. (Legacy code that creates the struct directly would see backwards-compatible behavior since the defaults would be what the package does today, without any options.) - Add unexported fields, which are set from those options, that control behavior of this sort. This allows for options with more semantic names, without exposing a slew of exported fields on the struct. The factory function can also return an error, which means we can do proper validation of the options (which we can't do if we expose this stuff just via exported struct fields).
My branch added options like WithConnect() and WithGRPCWeb(), that would configure the channel to use a different protocol. (The server impl handled all of them without options.) For this, we could add two options: one for the configuring the codec (i.e. "json", instead of "proto" default) and another for advertising to the server that the client supports SSE responses, which could be conveyed via an Accept request header that includes both "application/json" and "text/event-stream" (that feels more standards-compliant 🤷).
There was a problem hiding this comment.
Also, the above approach still makes it possible to add support for the Connect protocol later. This ContentType approach, sadly, does not really allow use of Connect, at least not easily, because the Connect protocol can also use "application/json" as the content-type for unary RPCs (and it uses "application/connect" for streaming RPCs -- a duality not easily supported, or at least not intuitively/readably, by a field like this).
(FWIW, on the server, we can distinguish between the Connect protocol and other "application/json" requests via the presence of a header: "Connect-Protocol-Version: 1".)
Also, variadic options could allow future enhancements for configuring other alternate codecs and also for configuring compression support.
There was a problem hiding this comment.
I do like adding the NewChannel w/ variadic options. I had looked for an existing way to apply variadic options but nothing currently exists at the channel level.
I'm trying to make sure I understand your suggestions so I'm going to make this concrete. Here's what I think it would look like to create a new channel that uses this protocol...
cc := httpgrpc.NewChannel(
http.DefaultTransport,
baseUrl,
httpgrpc.WithCodec(jsonCodec),
httpgrpc.AcceptSSEResponse(),
)The only issue with httpgrpc.WithCodec is that switching to JSON means more than just a change in the codec, it also means the framing changes. with "proto", we're using messages that are framed with a 4-byte header containing the size. With JSON, we're just relying on the json.Decoder to decode values in sequence. If you are suggseting WithCodec, it would mean that we would change the framing based on the name of the codec, right?
Or are you suggesting something more like this.
cc := httpgrpc.NewChannel(
http.DefaultTransport,
baseUrl,
httpgrpc.WithJSONEncoding(),
httpgrpc.AcceptSSEResponse(),
)httpgrpc.WithJSONEncoding would imply that the codec would be JSON and the framing would change as well.
My final quesiton is what should happen if we pass the option to enable json but we DO NOT pass the option to accept an SSE response?
cc := httpgrpc.NewChannel(
http.DefaultTransport,
baseUrl,
httpgrpc.WithJSONEncoding(),
)If I call an endpoint with a streaming response, would I get an error because the "Accepts" header contains neither "application/x-httpgrpc-proto+v1" nor "text/event-stream"? Or would I get a binary proto stream because the client always sends "applicaiton/x-httpgrpc-proto+v1" as an acceptable option?
There was a problem hiding this comment.
Good questions! I wasn't correctly remembering the state of this repo and what all it supports and what it's protocol handling looks like. (I've been doing much more in connect-go than in this stuff for the past ~4 years.)
Yeah, a pluggable codec doesn't really make sense here, so your suggestion of a WithJSONEncoding option makes the most sense. As far as use of the "accept" header, we don't usually look at it or respect it. I was only suggesting looking at it so that we don't need to invent some other way to advertise what the client understands. I think you would normally omit that header entirely (which is how it works today), and we only ever look at it for the JSON encoding case, to decide if the client supports the "text/event-stream" encoding for a server stream.
In case anyone were curious for what I had started on:
master...jhump:jh/grpcweb
I had started off with a completely different channel impl just so I could write a quick unit test that verified interop between the two (to make sure my changes didn't screw up compatibility with older clients). I would not have opened a PR that way or tried to merge it that way.
I should probably revisit that branch -- Claude Code could likely help me finish it in a snap. (Though one of the things I had hoped to set up was use of the connect conformance suite, to verify that this implementation was correct for the ConnectRPC and gRPC-Web protocols, which may be a bit of a lift...)
There was a problem hiding this comment.
@jhump ... I updated the PR taking into account your feedback, my interpretation of it, anyway. Looking at your grpcweb branch, I grokked a lot of what you were after and I do think the whole streamReader & streamWriter mechanism I created should be encapsulated into something like a protocol adapter. I didn't take on that refactoring, but I tried to reorganize the code to make such a refactor possible in the near future.
Enabling the json transport is now done with the WithJSONEncoding variadic option. I think this should allow us to add peer options in the future, like WithConnectRPC or WithGRPCWeb ... those would just set a different protocol adapter.
The one thing I'm not sure about it whether to do anything with the Accept header. Currently, in this PR, the client now advertises the expected response type unconditionally ... but nothing checks that type. Let me know if you had something different in mind.
httpgrpc/client.go
Outdated
|
|
||
| // ContentTypeJSON uses JSON-encoded messages over the channel. For unary calls, | ||
| // the request and response are JSON values. For streaming calls, the request is | ||
| // a series of JSON values and the response is SSE events containing JSON values. |
There was a problem hiding this comment.
If the request side is declared as a stream, the client should push in a series of concatenated json objects? Would bidi streaming still work in a Go client <-> server situation? I know actual bidi won't work from a browser-- you have to push all the request values up front and then stream back the responses after.
There was a problem hiding this comment.
If the request side is declared as a stream, the client should push in a series of concatenated json objects?
Yes. Since there is no clear standard for streaming from client to server, I just use the fact that JSON is, kind of, self framing. I also didn't want to have to introduce an odd-ball Content-Type for sending a stream of JSON objects to the server when the primary case for this is to send a non-stream request and get back a stream.
Would bidi streaming still work in a Go client <-> server situation?
No. True bidi streaming won't work with any transport on the http chan. You can technically stream either way, client -> server or server -> client, but you cannot stream both ways at the same time due to the transactional semantics of HTTP. And even if you were to totally hijack TCP connection, any reverse proxy in your infra is going to wreck your dreams. In practice, server -> clent, seems to be the only direction with any real utility and I'm not even sure I would rely on client -> server.
If you look at the httpgrpc_test.go, you'll notice that all the tests pass supportsFullDuplex = false.
(Updated to be clear what I was saying "No" to)
dragonsinth
left a comment
There was a problem hiding this comment.
@jhump would you have a moment to peruse also?
Added WithJSONEncoding ChannelOption. Pass Accepts header to server to advertise expected Content-Type. Cleaned up the code that determines readers, writers, codecs and media types.
Summary
Adds a JSON/SSE transport as an alternative wire format for streaming RPCs in the HTTP channel, alongside the existing binary proto format.
Approach
The core idea is to decouple message framing from RPC logic by introducing a
streamReader/streamWriterabstraction layer in a newstreamer.gofile. Both the client and server are refactored to use these function types instead of directly calling size-prefixed proto I/O functions (readSizePreface,readProtoMessage,writeProtoMessage). This makes it straightforward to swap in different wire formats.Two wire formats are now supported:
data:event and the final trailer is an event withevent: trailer. This format is primarily intended for use in the server to support browser-based clients but the client implements this format for testing the SSE streaming server implementation.The format is selected via a new
ContentTypefield on theChannelstruct (ContentTypeProtoorContentTypeJSON). On the server side, the format is negotiated from the request'sContent-Typeheader — the server selects appropriate reader/writer pairs and may respond with a different content type than the request (e.g. requestapplication/json→ responsetext/event-stream).Changes
internal/sse/— New package with a minimal SSE encoder and decoder (onlydataandeventfields, which is all that's needed).httpgrpc/io.go— DefinesstreamReader/streamWriterfunction types and factory functions that create format-specific readers and writers for both client and server roles.httpgrpc/server.go— RefactoredhandleStreamandserverStreamto usestreamReader/streamWriterinstead of direct proto I/O. Error detail headers now always use proto encoding regardless of the request content type.httpgrpc/client.go— RefactoredclientStreamto usestreamWriterfor sending andstreamReader(created from the response'sContent-Type) for receiving. AddedContentTypeenum and content-type accessor methods toChannel.httpgrpc/protocol_versions.go— AddedEventStreamContentTypeconstant, removed unusedgetStreamingCodec.httpgrpc/httpgrpc_test.go— AddedTestJSONSSEServerthat runs the full channel test suite over JSON/SSE.