Evaluate Style Guide

[amckinney]

Package Structure

• The codec and compress packages are split out so that we can more easily add a separate package for the Protocol abstraction (e.g. gRPC, gRPC-Web, Connect) without introducing an import cycle.
• The clientstream and handlerstream packages are split out so that we can tidy up names for these types. Otherwise, you'd have names like NewClientClientStream (the client's view of a client streaming endpoint).
• We might want it to be compress.Gzip instead of compress.GzipCompressor.
  • Edit: We'll move this to compress/gzip.Compressor.

Method Naming

• The stream has a ReceivedHeader method to distinguish itself from the request headers. Should we instead just name this ResponseHeader for clarity?
• Similarly, let's make it explicit to be RequestHeader and ResponseHeader.
• As discussed, we need to decide what we're doing with Simple/Full.
  • Client-side: WrappedPingClient and UnwrappedPingClient interfaces. PingClient is reserved for the type that users interact with.
  • Server-side: PingService (acts upon generic types), and NewPingService (acts upon any). Comments are left in-line to describe how to implement the simple, non-generic method signatures.

Future Proofing

• Top-level abstractions (e.g. Codec and Compressor) are propagated through the clientCfg and into the protocol abstraction via a catch-all protocolClientParams. If we eventually plan to export the protocol abstraction and include it as a ClientOption, the relationship here is fuzzy.
  • What happens if we ever have a protocol that doesn't interact with all of the protocolClientParams types - is it a no-op, an error, or a silent failure?
  • We could tie these options to each protocol individually to clear these relationships up, but we end up with some more repetition (i.e. we need to repeat the same options for similar protocols like gRPC and gRPC-Web). For example, each of the gRPC and gRPC-Web protocols would have a Codec option.
  • In the connect-api branch, we were able to get around this because the protocols were separate client implementations, and they could each individually own what options they exposed (e.g. here).
• We still need to figure out error details. I know the gRPC protocol requires the proto.Any, and Akshay had some ideas around this - we could rename the methods to include Any so it leaves room for us to add other types later (e.g. AddAnyDetail). The abstraction violation between the pluggable Codec and the proto.Any sucks, but I know we're at mercy to the gRPC protocol here.

1.0 Features

• The gRPC health and reflection packages are valuable, but they aren't really necessary to begin with. We should consider whether or not these packages should exist in a separate repository (similar to gRPC middleware repositories).
  • I know we need to be mindful of this w.r.t. including the RegistrationName in the generated code. If we were to drop this support to begin with, we'd need to reintroduce this as a HandlerOption later, and that's tied to the connect library itself. It's not immediately obvious how this would work.
  • Decision: health and reflection are staying where they are. We need these features for easy gRPC user adoption. To be clear, health is non-optional. reflection is a huge quality of life improvement and its (nearly) part of the gRPC specification at this point.
• connect.MaxHeaderBytes is kinda nice, but doesn't feel necessary and is prone to change across different protocols.
• Should connect.ReceiveResponse be in an internal package? It's only used twice and otherwise distracts from the API that users ought to interact with. This might already be your plan based on the conversations we had earlier about the user-facing API and connect internals.
  • It looks like ReceiveRequest needs to be exported for the generated code, so I can see an argument to export it for symmetry.
• Drop IsValidHeaderKey and IsValidHeaderValue.

Implementation Details

• In the connect-api branch, I left a note for myself about whether or not the discard helper function can hang forever (e.g. discard(cs.response.Body)). This might have happened when I introduced the gRPC testing suite, but I can't recall. We need to make sure this can't happen.
  • Nothing to do here - this is a consequence of needing to read the http.Response.Body to completion to reuse the connection. This is also just an implementation detail, so it's not blocking regardless.

[akshayjshah]

In the connect-api branch, I left a note for myself about whether or not the discard helper function can hang forever (e.g. discard(cs.response.Body)). This might have happened when I introduced the gRPC testing suite, but I can't recall. We need to make sure this can't happen.

It's Go - every read from/write to the network can hang until the request times out. 🤷🏽‍♂️ I discussed this on #26 in the context of streams, but it's a general problem. When working on $PROXY at $PREVIOUS_TAXI_COMPANY, we used the net.Conn APIs to set a deadline before every read/write, but it was horrifically gross and probably inappropriate for a package that operates at this level of abstraction.

discard is a marginal improvement over the usual defer io.Copy(io.Discard, r.Body) - we do our best to reuse the connection, but we won't schlep around gigabytes of garbage. I'm looking at the implementation again, and I don't see any obvious bugs - do you remember anything about what made you worried about it?

[akshayjshah]

Should connect.ReceiveResponse be in an internal package?

Can't move it to a different package from the Response struct without exporting all the fields on Response. We could do that, but it doesn't seem worth the loss of future flexibility.

We could move a bunch of stuff into an exported subpackage (e.g., github.com/bufconnect/connect/guts) and selectively expose types in connect with aliases, in which case ReceiveRequest and ReceiveResponse wouldn't get aliased. I'll open a PR to explore that - I think it'll end up being pretty awkward, but maybe I'll be surprised!

[akshayjshah]

Future-proofing concerns are 100% valid. Let's discuss those in person and document results here.

[bufdev]

We might want it to be compress.Gzip instead of compress.GzipCompressor.

I'd disagree with this one, I don't think it's in line with our codebase

[akshayjshah]

We might want it to be compress.Gzip instead of compress.GzipCompressor.

I'd disagree with this one, I don't think it's in line with our codebase

Pending your API review down the road, we decided to copy the standard lib and go with compress/gzip.Compressor.

[akshayjshah]

@amckinney and I discussed the future compatibility issues above.

• We're punting a fancier design of the unexported protocol abstraction to a future date. What we have now is private, simple, and sufficient for our current needs: most importantly, it lets us configure clients to use different protocols with options rather than generating a whole new constructor. If we ever decide we'd like to export protocols, we can figure out whether we need a redesign.
• We think that there's a nice interface-based way to avoid depending on the concrete *anypb.Any type. Users can still pass a proto.Any, but we'll have the freedom to introduce something better down the road.