Update documentation of stream interfaces using generics and remove code generation of stream methods in service interfaces

[stevenroose]

For a bi-directional stream, I get the following generated code:

type Btcd_SubscribeTransactionsServer interface {
	Send(*MempoolTransaction) error
	Recv() (*SubscribeTransactionsRequest, error)
	grpc.ServerStream
}

There is absolutely no documentation about the behavior of these methods. They should either be documented in the code, or have a link to a page with documentation about them.

The "Go Basics" documentation merely gives the following example code:

func (s *routeGuideServer) RouteChat(stream pb.RouteGuide_RouteChatServer) error {
	for {
		in, err := stream.Recv()
		if err == io.EOF {
			return nil
		}
		if err != nil {
			return err
		}
		key := serialize(in.Location)
                ... // look for notes to be sent to client
		for _, note := range s.routeNotes[key] {
			if err := stream.Send(note); err != nil {
				return err
			}
		}
	}
}

It seems that it's impossible to get a channel for incoming messages. But it's also not clear if Recv is a blocking call. Does io.EOF mean that the client closed that channel or that there is currently nothing to be read?

UPDATE:

Implementations for streams are modified to use generics #7057, which will cause the gRPC codegen to use prebuilt generic types to implement client and server stream objects, rather than generating new types and implementations for every RPC method. We need to make sure the stream interfaces using generics are well documented. Here are the high level tasks for completion

• Add documenation for stream interfaces - grpc: add docs for generic stream interfaces  #7470
• Delete the code to generate stream interfaces for client and server.
• update the grpc.io docs to use generics instead of generated interfaces.

[dfawley]

Recv is blocking. io.EOF from Recv means the client called CloseSend (or SendAndClose).

There is a manual for our generated code on grpc.io, but I agree we should have better godocs or pointers to the manual from the generated code.

[stevenroose]

So it's not really possible to do send and receive both from the same goroutine? I recently did an implementation of a notification system using bidirectional streams (where the client could change the topics it is interested in via messages and the server sends notifications over the stream).

I ended up having a small goroutine putting incoming messages on a channel manually:
btcsuite/btcd@d638d33#diff-c830ab1edc3089a502e68c02d7f576bfR1272

So that I had a single goroutine that can do both sending and receiving with a select (sending on incoming notifications via a channel).

It seems that I should exclude io.EOF in that small goroutine from closing the channel. So that the client can still receive notifications when it no longer wants to change it's subscription. Or explicitly document that the server closes the stream when the client does. (Seems a lot easier for the client as well, since otherwise canceling a stream needs to be done by cancelling a context..)

[dfawley]

So it's not really possible to do send and receive both from the same goroutine?

Right, this is not recommended (but it is technically possible if you control both server and client and can ensure they work together properly). Send() is also blocking, so if the other side is not receiving, it will eventually run out of flow control and block until the other side does call Recv(). This could lead to a deadlock if you aren't careful.

It seems that I should exclude io.EOF in that small goroutine from closing the channel. So that the client can still receive notifications when it no longer wants to change it's subscription. Or explicitly document that the server closes the stream when the client does.

Either way seems reasonable for this use case. I don't think there's much value in calling CloseStream from the client except when it's ready to end the stream, and then you can get the final status in case it's potentially interesting (which you can't get if you have to cancel the context to end the stream).

[srini100]

Using this bug to consolidate issues regarding documentation around streaming API.
#1880
#1876

[dfawley]

@srini100, note that we've recently added some stream documentation here:

https://github.com/grpc/grpc-go/blob/master/Documentation/concurrency.md

Is that sufficient for all of these issues?

[stevenroose]

@dfawley I'd primarily like code documentation on the generated methods. So that I can read it (1) from godocs and (2) from going into the code with my IDE. Mostly the 2nd is where I was missing it. When you're implementing your server, you are using these interfaces. So it's normal that you want to go look at the documented behavior of them, right? Right now you get nothing.

[stale]

This issue is labeled as requiring an update from the reporter, and no update has been received after 7 days. If no update is provided in the next 7 days, this issue will be automatically closed.

[floatdrop]

There are still some things that could be clarified more about Send() method on stream: we receiving io.EOF errors after calling this method and only one possible answer can be found in mailing list by @dfawley – https://groups.google.com/d/msg/grpc-io/XcN4hA9HonI/F_UDiejTAwAJ

Client-side, io.EOF is returned directly from grpc-go steram.Send() calls when the server closes the stream. At that point, Recv() should be called until it returns a non-nil error to receive the server's messages and eventually the status. If io.EOF is returned by Recv(), that indicates a success; otherwise that is the final status of the RPC. All errors are permanent from the standpoint of that stream -- subsequent streams are possible using the same client, even if it is a connection error, as the client automatically attempts to reconnect. Make sure you always call Recv() on the stream until you get a non-nil error or cancel the context used when creating the stream, or else a goroutine and other resources will be leaked.

Can we clarify what errors and how they should be handled may Send() method return?

[janardhanvissa]

@purnesh42H Please assign me this issue.

[purnesh42H]

@janardhanvissa recently the implementations for streams are modified to use generics #7057, which will cause the gRPC codegen to use prebuilt generic types to implement client and server stream objects, rather than generating new types and implementations for every RPC method

So, most of the above discussion is not relevant anymore. For now, you can add inline comments to stream interfaces's methods following the documentation here https://grpc.io/docs/languages/go/generated-code/

@dfawley to confirm if that is enough for this task

[dfawley]

+1. The medium-term plan is now to completely delete these generated interfaces in favor of the generics (they are still optionally generated for now). We should make sure the generics are well documented and make sure the generated code has enough information to help users understand them. We should also update the grpc.io doc @purnesh42H pointed to as needed. Another thing I noticed yesterday is that it doesn't mention the embedding of the Unimplemented<Service>Server struct inside implementations of the service. That should ideally be added there as well.

[janardhanvissa]

@purnesh42H, I have added the behavior of the function "RouteChat" and generated the protobuf files. I have raised a PR for the changes that made. Please review the PR and let us know if any changes are required for the ticket. Please find the PR below.

#7441

[purnesh42H]

@janardhanvissa the PR is not addressing the requirements of this task. As mentioned above, the implementations for streams are modified to use generics #7057, so we need to document the stream interfaces in generics. Here are the high level tasks that needs to be done.

1. Add the inline comments for interfaces in

   grpc-go/stream_interfaces.go

   Line 24 in 566aad1

   type ServerStreamingClient[Res any] interface {

   
   Use the explainations from here https://grpc.io/docs/languages/go/generated-code/

2. delete these generated interfaces (if there are any)

3. update the grpc.io docs to use generics instead of generated interfaces.

Let's modify the PR you sent for 1) and then we will move to 2) and 3)

[dfawley]

2. delete these generated interfaces (if there are any)

FWIW this is not correct. Generated code should never be directly modified. It should be regenerated when the generator or source files change, which our vet-proto checker will fail if we don't do that correctly.

[janardhanvissa]

@purnesh42H Added Inline comments for all the 6 interfaces in stream_interfaces.go. Please find the PR below. Also please provide the brief information regarding 2) and 3) as mentioned above.

#7470

[janardhanvissa]

@purnesh42H I removed the parent interface inline comments as mentioned. Could you please provide the brief information regarding 2) and 3) as mentioned.

1. Add the inline comments for interfaces in stream_interfaces.go - Done
2. delete these generated interfaces (if there are any)
3. update the grpc.io docs to use generics instead of generated interfaces.

[dfawley]

@purnesh42H - you reopened this -- what still needs to be done here?

[purnesh42H]

@purnesh42H - you reopened this -- what still needs to be done here?

yeah, don't we still have to do these 2?

• Delete the code to generate stream interfaces for client and server like here https://github.com/grpc/grpc-go/blob/master/cmd/protoc-gen-go-grpc/grpc.go#L337-L346 ?
• update the grpc.io docs to use generics instead of generated interfaces. https://grpc.io/docs/languages/go/generated-code/ is not applicable anymore

[dfawley]

Sure. Maybe we can update the title of the issue to include these things, too? We added documentation to the generic interfaces, so from the title it seems like we're done here.

[purnesh42H]

Sure. Maybe we can update the title of the issue to include these things, too? We added documentation to the generic interfaces, so from the title it seems like we're done here.

Updated

[eshitachandwani]

We will need to update the documentation to remove the part that says that we can go back to old behavior by changing the flag, when we remove the flag and make the use of stream generics default