Add simple flag

[bufdev]

EDIT: Most of the remaining things mentioned in the text below was done in the last couple of commits, as a collective effort by @smaye81, @srikrsna-buf, and @jchadwick-buf. That arc of work implements CallInfo and associated functions as a way to interact with the context in order to set and retrieve the metadata that was otherwise only available in the wrapper types.

These commits also add numerous new tests of the new functionality and generated code. They also include updates to the client- and bidi-stream generated code. The bidi-stream code did not previously use the wrapper types at all, but it did require the caller to explicitly call stream.Send to actually start the RPC, in case they wanted to first add request headers (the same is true for client-stream calls). If the caller wanted to start the RPC before actually sending any messages, they had to (counter-intuitively) pass a nil message to stream.Send. But with this new code-gen mode, calling code can put request headers into the context before calling the method, so client-stream and bidi-stream RPCs can be started immediately.

A future commit will update the README (we wanted to wait until after a release was created, so users looking at the README wouldn't be confused by it being out-of-sync with the latest release). We'll also follow-up soon with updates to the connectrpc.com docs.

---

This is re: #848 #421. This is the simplest possible version, and is just done as a prototype for discussion. Very little here should make it to merge: the doubling-up of the methods to add Simple is pretty lazy, the naming isn't overly consistent, etc. The code as-is is just a demonstration.

The general concept is to effectively just model what people know already. The interface is very similar to grpc-go in this case. It's a misuse of context.Context in the general sense, but basically every RPC library (except connect-go, but including grpc-go, which is what people are typically migrating from) does this. This results in unary-only Handler interfaces matching grpc-go and Twirp, and Client interfaces almost matching grpc-go (and matching Twirp). This is (in my opinion) generally a feature, but there are caveats: people using the grpc-go metadata package will be tripped up when migrating, when they realize their headers/trailers aren't being propagated. Given how few people actually interact with headers and trailers, however, this feels like a minor downside that can be documented as part of migration.

A final version of this will likely not be flag-based, we'll want to carefully choose how to expose this:

• A new plugin, such as protoc-gen-connect-gosimple.
• Generate to a separate package, such as .*connectsimple.
• Generate with a new interface name in the same package, such as .*SimpleClient.

Whatever we do, we do not want to v2 and will never break existing users.

EDIT: This is closer to good to go now, and I'm actually more in favor of the flag option than I was. However, we're still not there. If we end up doing this, better documentation and testing is needed. We'll also need to update documentation on connectrpc.com, and on the Buf side we'll want to release a new plugin connect/gosimple that uses this flag.

[mfridman]

Strong +1 from me.

After a good few years and many servers/clients written for work and personal projects, my take is:

1. Unfamiliar and verbose migrating from twirp-go/grpc-go
2. Wrapping with New{Request,Response} is tedious, and so daily ergonomics suffer
3. Optimizes for the rare case, most code doesn't touch headers. So every call site pays the complexity cost
4. Context propagation is fine. For the rare times you need headers, passing via context is acceptable!

TL;DR - developer ergonomics matter more (esp. in this case) than avoiding the "context misuse" when that misuse is already the established patterns developers expect.

[Alfus]

Strong +1 from me as well. Wrapping causes a lot of extra, unneeded boilerplate code.

[mattrobenolt]

Generate to a separate package, such as .*connectsimple.

Does it make sense to also use this as an opportunity, likely configurable since I understand why we do it this way currently, given this "simple" interface to generate into the same parent package by default to mirror gRPC behavior?

[bufdev]

Speaking for myself, I still feel relatively strongly about generating to a separate package by default, but maybe could be convinced otherwise. I think this was a big miss on grpc-go's part, it sets up a whole host of problems w.r.t. plugin interactions. I get where your mind is though - grpc-go migration, import simplicity.

Note that with the implementation as-is in this PR now, it is just flag-based and generates to the same package as before. And this is configurable via package_suffix, including setting to empty to copy grpc-go semantics of generating to same package. I'd argue to keep it this way - two independent flags - as opposed to introducing special semantics w.r.t package if you set the simple flag.

[bufdev]

A separate overall note: the implementation as-is fully allows and supports multiple protoc-gen-connect-go calls to independent packages, so for users who want both interfaces available, this is trivial. In fact, this is what this repository is now doing as of this PR: look at buf.gen.yaml, which generates to internal/gen/generics and internal/gen/simple.

[mattrobenolt]

Oh yeah, I agree with the choices entirely to keep it a separate package, but more thinking in lines of expectations that users have.

This API that is being addressed here and the separate packages are the biggest real complains I've observed from discussions.

Even though we agree a separate package is better, I'm more looking at it from a lense of default expectations.

If I'm a user and I want this "simple" API, I likely am just wanting a grpc-go experience to align and be as much drop-in as possible.

I definitely don't hold a strong opinion here tho. I'm just pushing for a flip in the default behavior when opting into this simple API. But I think it's fine either way.

[jhump]

I'm a little worried about the fact that it generates incompatible code and requires a plugin option. This is mainly an issue for BSR generated SDKs -- we can't change the options used for the "connect-go" generated packages because of back-compat issues. Do you instead anticipate adding a "connect-go-simple" set of SDKs?

What if we generated both always, e.g. PingServiceSimpleClient, PingServiceSimpleHandler?

[jhump]

In grpc-go, this accumulates a slice of all pointers for all calls. That way, if it goes through any middleware or other library code, where this is called multiple times, everyone gets back the same headers. With this implementation, the last one will get back the headers and all prior callers will get back nothing.

[jhump]

In addition to the headers and trailers, you also need functions for accessing Spec and Peer.

Instead of mirror the grpc-go API so closely and having all of these loose functions for dealing this stuff, how about we try something a little different and consolidate so that we have fewer free functions. Something like so:

// RequestInfo represents metadata about the current RPC
// operation.
//
// On client-side, RequestHeader returns mutable map for setting
// headers before the RPC is invoked. Other values can be inspected
// later after RPC completes.
//
// On server-side, ResponseHeader and ResponseTrailer return
// mutable maps for setting response metadata before handler
// returns. Other values can be inspected immediately.
type RequestInfo interface {
	Spec() Spec
	Peer() Peer
	RequestHeader() http.Header
	ResponseHeader() http.Header
	ResponseTrailer() http.Header

	// unexported method gives us the flexibility to add to this
	// interface, just like we have in AnyRequest and AnyResponse
	internalOnly()
}

// Create a new request context for use from a client. When the returned
// context is passed to RPCs, the returned request info can be used to set
// request metadata before the RPC is invoked and to inspect response
// metadata after the RPC completes.
//
// The returned context may be re-used across RPCs as long as they are
// not concurrent. Results of all RequestInfo methods other than
// RequestHeader are undefined if the context is used with concurrent RPCs.
//
// If the given context is already associated with outgoing RequestInfo, then
// ctx and the existing RequestInfo are returned.
func NewOutgoingContext(context.Context) (context.Context, RequestInfo)

// RequestInfoFromOutgoingContext returns the RequestInfo for the given
// context, if there is one.
//
// Here for completeness, but not really necessary since NewOutgoingContext
// can return existing RequestInfo and since interceptors get wrappers for
// direct access to request info properties.
func RequestInfoFromOutgoingContext(context.Context) (RequestInfo, bool)

// RequestInfoFromIncomingContext returns the RequestInfo for the given
// context, if there is one. Always returns a value and true for contexts passed
// to server interceptors and handlers.
func RequestInfoFromIncomingContext(context.Context) (RequestInfo, bool)

This would also resolve the comment about, about needing to accumulate response header/trailer addresses, since NewOutgoingContext can be called more than once before the RPC and all callers get access to the same info.

[bufdev]

I like this a lot - much cleaner. I'll work towards this.

[jhump]

I think we will actually want "simple" versions of the client and bidi-stream client interfaces, too. Currently, they allow the caller to set request headers first, and the RPC isn't actually invoked until the first call to stream.Send. But a simple version could omit the RequestHeader() method and immediately invoke the RPC, because the header data can be provided in the context.

I think this will be more intuitive for users because it has been a question in the past (particularly for bidi) for how to send request headers and start the RPC but not send an initial message. Having to call stream.Send(nil) was the work-around, but with "simple" clients, we could resolve that quirk.

[bufdev]

Given this isn't a Buf product, I'm not as concerned with that. But with respect to the BSR, Buf would just host it as a new plugin "connect/gosimple" that called this option.

I don't want to add a suffix to the interfaces - that's making the simple interfaces more complicated. Presenting a single package with two different types of clients and handlers makes the problem worse IMO.

[jhump]

Coincidentally, this would also solve #850 since it provides a way for unary handlers to explicitly set header or trailer metadata, independent of the metadata set on a returned error.

[jhump]

I was thinking a bit more about this: a flag named "simple" may not be the most intuitive way to turn this on. It's not that it's "simpler" than the other way; it's just an alternate approach. While it may feel simpler to not have the wrappers, it's actually less simple/less obvious when you do need to interact with metadata.

So maybe it's more like "metadata_mode=context" and the existing/default behavior is "metadata_mode=wrapper". (I don't particularly like "metadata_mode", but I'm having a hard time deciding what to call it. I similarly don't actually like the name RequestInfo in my other suggestion/proposal, but am not sure what the best name would be...)

[bufdev]

I'd have to hard disagree on this one.

The whole thing here is that almost no one interacts with the metadata. It's significantly more complicated for the vast majority of users to have a Request/Response wrapper - if it were simpler to use the existing system, no one would be clamoring for this feature.

The option should be clear, concise, and the closest thing to get users to use it as a suggested default (short of v2'ing connect-go and making it the actual default, which we do not want to do). A boolean option simple accomplishes that.

When I read context vs wrapper as two option values, I do not know what these mean or why I should choose one or the other without reading the docs - and it's a fair assumption that most users won't read the docs. The vast majority of developers will be served best by using the simple API - it gets them what they need, in the way they expect it. Calling this option simple will pull gravity in this direction.

[pkwarren]

This looks great - removing the request/response wrappers would remove a lot of boilerplate. My main feedback was related to supporting Peer/Spec and avoiding storing multiple values on the context, but @jhump covered that above: #851 (comment).

[bufdev]

As a reader, I don't know what simple mode is (I'm not sure what it is in this context myself). This, and the other methods for simple mode, need Godoc explaining what the differences are, likely 3-6 sentences minimum, especially as most share the same function signature.

[smaye81]

Added changes for this, client_stream.go and context.go in #865

[bufdev]

Same comment about Godoc.

[bufdev]

Why would there be a data race if I just call a read-only method? I think what you mean is that "if you ie modify an http.Header concurrently, there will be a data race", which would make sense, however this isn't how this reads - it reads that just calling any of the methods will cause a data race, which I would not expect to be true. This needs clarification, if it even needs to be called out - calling ie Spec across two goroutines should not result in a race, and if it does, the backing struct for callInfo needs a RWLock (maybe it does anyways).