Implement CallInfo usage in context and add integration tests

[smaye81]

This adds the usage of CallInfo in context for issuing requests with the new simple API. This builds on top of the #851 which implements the simple flag for unary and server-streaming

In addition, it adds integration tests for the simple and generics API.

[jhump]

You might use an unexported newOutgoingContext that returns the concrete type *callInfo, instead of CallInfo. That way you can easily poke around at unexported fields. Ideally here, we wouldn't call call.ResponseHeader() since that will allocate another map. Instead, you could look directly at the unexported field and if it's nil, just set it to resp.Header(). In most cases, that skip an allocation and copy since most callers will never look at or care about these headers.

(Same for trailers.)

[jhump]

This is copying in the wrong direction. The caller may have setup headers already in the CallInfo, and you need to copy them into conn.RequestHeader(). In fact, you probably want to copy the info headers first. That way, if the caller setup headers in a context and also set headers up in a *Request (using "non-simple" code gen), the *Request passed to the RPC call would take precedence.

Although... looking at mergeHeaders it never overwrites entries, just appends. I guess that's the safest behavior (though could also be surprising if anyone expects things to override). Although there's still a question of what would you expect first in the list of headers; and I think the more intuitive answer is "headers from context first; then headers from explicit request".

[jhump]

You need two different keys, one for outgoing and one for incoming. It is not safe for them to share the same key.

For example, an incoming call info (for a handler) could contain authentication credentials in request headers. Using the same context to make another RPC, if using the same context key, would see the call info in context and then propagate all of those headers along. That could be a security issue since it could be inappropriately sending the client's credentials to another server.

So "incoming" is for handlers because the call is incoming to the handler from a client. And "outgoing" is for clients, because they are making outgoing calls.

[jhump]

Need to distinguish between CallInfoFromOutgoingContext and CallInfoFromIncomingContext.

[jhump]

Like above, this is going the wrong direction. On the server, handler code can set the call info response headers and trailers, and we need to copy them into the conn so they correctly get sent to the client. Also, for both above and these calls, it's likely best to have a concrete value so you can examine the unexported fields and skip this step if the maps are nil/empty, instead of indirectly instantiating an unused map (since that happens inside call.ResponseHeader()).

[jhump]

You'll want something more like so:

call := &callInfo{
    peer:          request.Peer(),
    spec:          request.Spec(),
    method:        request.HTTPMethod(),
    requestHeader: request.Header(),
}
ctx = newIncomingContext(ctx, call)

[jhump]

You shouldn't need to do this here. Everything related to call info is already being done above in the implementation function inside NewUnaryHandler.

[jhump]

This is where you'd use an alternate implementation of CallInfo that just delegates to conn, since it should implement all of these methods (except HTTPMethod() which can be hard-coded to "POST" for streaming calls).

Something like so:

call := &streamCall{streamingConn: conn}
ctx = newIncomingContext(ctx, call)

Then you don't need to copy anything because the CallInfo methods directly delegate to the stream and thus directly return its maps for headers and trailers.

The streamCall could look something like:

type streamingConn interface {
    // subset of CallInfo that all streaming conns provide
    Peer() Peer
    Spec() Spec
    RequestHeader() http.Header
    ResponseHeader() http.Header
    ResponseTrailer() http.Trailer
}

type streamCall struct {
    streamingConn
}

func (c *streamCall) HTTPMethod() string {
    return http.MethodPost // all stream calls are POSTs.
}

[jhump]

We don't want to add an extra exported type to the API for this. And nothing even references this interface. So let's remove it and change these back to the way they were.

[jhump]

We don't want to allocate maps here if the headers and trailers were unset. So let's refer to the unexported fields of info instead. If they are nil/empty, this should do nothing.

In fact, we should probably do the same below for response, to similarly avoid unnecessary allocations.

[smaye81]

I added the nil check to the info headers/trailers, but the response header/trailer functions lazily create the map, so I'm just checking for len == 0 on those.

[jhump]

These comments could use a little clarification.

Suggested change

	// Add response headers/trailers into the context callinfo
	mergeNonProtocolHeaders(conn.ResponseHeader(), info.ResponseHeader())
	mergeNonProtocolHeaders(conn.ResponseTrailer(), info.ResponseTrailer())
	// Add response headers/trailers into the conn
	// Add response headers/trailers from the context callinfo into the conn.
	mergeNonProtocolHeaders(conn.ResponseHeader(), info.ResponseHeader())
	mergeNonProtocolHeaders(conn.ResponseTrailer(), info.ResponseTrailer())
	// Add response headers/trailers from the response into the conn.

[jhump]

On the client conn.ResponseHeader() will block for actual headers to come back from the server. So this seems like it would deadlock -- we haven't half-closed the stream yet (that happens in the conn.CloseRequest() call below), and the server isn't expected to send response headers until that happens.

But just moving the call to after conn.CloseRequest() is called, below, still changes the semantics slightly. Previously, this method would send the request but then not block for the response headers until the first call to conn.ResponseHeader() or to conn.Receive(). But if we call conn.ResponseHeader() below, then we're moving that blocking into this function.

Maybe we need another implementation of callInfo -- like a clientCallInfo that doesn't have its own source of response headers and trailers. Instead it could embed the source and do something like this:

type responseSource interface {
    ResponseHeader() http.Header
    ResponseTrailer() http.Header
}

type clientCallInfo struct {
    // .... other fields to implement other methods ...
    responseSource
}

func (c *clientCallInfo) ResponseHeader() http.Header {
    if c.responseSource == nil {
        return nil
    }
    return c.responseSource.ResponseHeader()
}

func (c *clientCallInfo) ResponseTrailer() http.Header {
    if c.responseSource == nil {
        return nil
    }
    return c.responseSource.ResponseTrailer()
}

Then, above, when setting the other fields of callInfo, you can also set callInfo.responseSource = conn. For unary, you can set it to the *Response.

I think we'd need to add some docs to the CallInfo interface, too. Something like so to describe ResponseHeaders and ResponseTrailers:

On the client side, these methods return nil before the call is actually made. After the call is made, for streaming operations, this method will block for the server to actually return response headers.

You might also add something to the docs for CallInfo to clarify that CallInfo values are not thread-safe. (So if someone creates an outgoing info, they should expect data races if they pass that info to another goroutine and try to call methods on it concurrent with the RPC.)

[jhump]

It would be ideal if interceptors could examine the context for these. But, as this is, we won't set anything on the call info until after interceptors return. The callUnary function above runs all interceptors and then the handler.

We could remedy this by moving this logic into the initial definition of callUnary, around line 78 inside NewClient, right after the call to receiveUnaryResponse. You'll have to dig the call info out of the context again, but I think that should work. You probably also want to leave a comment here explaining why we're not doing that here.

[smaye81]

Moved this and then added tests to interceptor_ext_test.go to verify peer, spec, and method. We probably need more interceptor tests, but it seems like that might be a follow-up since the interceptor tests require a bit of setup? I'm thinking maybe we need:

• verify call info for streaming
• verify headers in context info for streaming and unary
• verify a unary handler can separately set response headers and trailers

[smaye81]

Think I got the placement right on these.

[smaye81]

@jhump incorporated all your feedback in fa4e176. Lmk what you think.