server: add embedding requirement to ServerTransportStream

[dfawley]

grpc.ServerTransportStream is currently not extensible, because it does not enforce that a delegate implementation is embedded. This change creates that requirement. I noticed this when I was investigating the possibility of using this kind of intercepting (i.e. extending it to support messages) to suggest a way to accomplish #7794. I'm not sure if that's appropriate, but something like this generally seems like a good idea to do regardless.

cc @jhump - this is potentially a breaking change to an (experimental) API you created. Does this seem reasonable to you, or actually break anything of yours?

RELEASE NOTES:

• server: add embedding requirement to ServerTransportStream (experimental)

[codecov]

Codecov Report

Attention: Patch coverage is 28.57143% with 5 lines in your changes missing coverage. Please review.

Project coverage is 81.90%. Comparing base (d66fc3a) to head (0e3c4ce).
Report is 14 commits behind head on master.

Files with missing lines	Patch %	Lines
server.go	33.33%	2 Missing and 2 partials ⚠️
internal/transport/transport.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7798      +/-   ##
==========================================
+ Coverage   81.71%   81.90%   +0.18%     
==========================================
  Files         374      371       -3     
  Lines       38166    37705     -461     
==========================================
- Hits        31188    30882     -306     
+ Misses       5699     5544     -155     
  Partials     1279     1279              

Files with missing lines	Coverage Δ
internal/transport/transport.go	86.84% <0.00%> (-6.12%)	⬇️
server.go	82.42% <33.33%> (+0.34%)	⬆️

... and 23 files with indirect coverage changes

[jhump]

@dfawley, I'm having trouble understanding this. This experimental API was used to provide a separate implementation of a transport, of grpc.ClientConnInterface. To that effect, it will not have any *grpc.Stream instance that it can use as a delegate. I suspect this totally breaks that use case (but I can checkout those transport impls and try out this branch). Could custom transports embed a nil *Stream or would that guarantee a nil-derference panic? Is the idea here to make newer methods available on ServerTransportStream via embedding the Stream?

[jhump]

Yeah, this seems to totally defeat the point of the experimental API and definitely breaks those transports. I see that it's not even possible to embed a nil Stream since the type that implements the necessary interface is not grpc.Stream but rather a type inside of an internal package 😦. So it's now impossible to provide an external implementation of ServerTransportStream (backed by a stream representation for an alternate transport), which was the original point.

[dfawley]

@jhump thanks for looking. For this kind of testing, I believe you could just embed the interface itself to satisfy the requirement:

type myTestTransportStream struct {
	grpc.ServerTransportStream
	// etc
}

I thought the primary purpose of this interface was to intercept the SetHeader/SendHeader/etc operations in unary server interceptors, which otherwise can't be intercepted, because our API requires method handlers to call grpc.SetHeader(ctx)/etc.

[jhump]

@dfawley, thanks for the reply. I did think of that and got things working:
fullstorydev/grpchan#71

I'm still not clear on what the purpose of embedding is. This seems like a nasty nil-dereference risk in this library now, if there's anything else inside of grpc-go that might get a hold of our implementation and try to type-assert it.

[dfawley]

Per the description, the intent was to enable us to extend this interface in a backward compatible way. Otherwise, adding new operations will break anyone just trying to intercept the existing set of operations. So the expected use case for that "normal"(?) usage would be:

func myInterceptor(ctx, ...) ... {
	curSTS := grpc.ServerTransportStreamFromContext(ctx)
	mySTS := &mySTSImpl{ServerTransportStream: curSTS, ...} // Overrides the functions it wants to intercept.
	ctx = grpc.NewContextWithServerTransportStream(ctx, mySTS)
	methodHandler(ctx, ...)
}

But since you have things that are implementing it from scratch, you're right that if we were to do that and you were to embed this interface and leave it at nil, then you would be broken at runtime instead of at compile time, which would be worse.

We could instead create an optional interface that can be implemented if you want to intercept new things. But I think that's also wrong. E.g. SetSendCompressor and ClientSupportedCompressors are both actually broken right now in your current use case -- they fail at runtime, because the ServerTransportStream you put in the context is not a *transport.Stream. That's not great, either.

Maybe we should do this instead:

type UnstableServerTransportStream interface {
	// All interceptable methods go here.
	// We reserve the right to add more and break you at compile time forever.
}

type ServerTrasnportStream interface {
	UnstableServerTransportStream
	mustEmbedDelegate()
}

func NewContextWithUnstableServerTransportStream(context.Context, UnstableServerTransportStream) ctx {}

You'd use the "unstable" symbols since you're implementing from scratch, and you'll learn when you upgrade that you are missing some methods. That's probably better than your users finding out at runtime?

Thoughts?

[dfawley]

@jhump - See the latest commit for a PoC of the above idea - it's basically exactly the same as what I wrote, just actually implemented with tests passing (I think?).

[jhump]

I suppose I could make this work, but it still feels a little icky being broken each time a method is added.

What about this:

1. Go back to just a single ServerTransportStream, add the new methods there as well as the requirement to embed, so that new methods can safely be added to the interface in the future.
2. Provide a NopServerTransportStream concrete type that implements ServerTransportStream, where all methods have a dummy/no-op body and return zero values. External implementations can embed this type to meet the new embed requirement, and will risk neither nil-dereference panic nor compile error when a new method is added in the future.

[dfawley]

Yes, that sounds good with one extension: the way I noticed this and got here was that I was looking for some way to intercept data operations at the transport level, too, as an attempt to make #7794 more feasible, and possibly good enough to consider #1805 done. I really don't want to add a third (fourth?) interceptor API, and instead I think we could extend/replace this one to accomplish what we need. But I still need to look a bit more closely at things in order to see how possible that would be.

[jhump]

@dfawley, question about the minimum Go version being 1.22. I made it in a comment on linked PR, but I suspect you might not get a notification from a comment in that other repo. Here's the link to the thread: fullstorydev/grpchan#71 (comment)

[dfawley]

I'm going to close this for now..it will take some extra consideration. But I think the current state is problematic since there are some things that just don't work correctly when this is used (i.e. SetSendCompressor and ClientSupportedCompressors).

[jhump]

@dfawley, please tag me in any future PRs when you get to an alternate solution for this, so I can prep necessary changes in downstream projects. And thank you so much for your consideration of those projects!

[dfawley]

I most certainly will. Thanks for your input @jhump!