Use streaming functionality to implement unary calls

[dfawley]

I don't believe there's any reason why call.go could not be implemented as:

NewClientStream(); SendMsg(); CloseSend(); RecvMsg()

@menghanl noted that a unary RPC is essentially a streaming RPC with neither client- nor server-side streaming.

This would allow us to remove a lot of code and increase our test coverage of stream.go.

E.g. consider these two places in our code (for unary and client-only streaming, respectively) to receive the response and status:

grpc-go/call.go

Lines 70 to 75 in 1cd2346

	if err = recv(p, dopts.codec, stream, dc, reply, *c.maxReceiveMessageSize, inPayload, comp); err != nil {
	if err == io.EOF {
	break
	}
	return
	}

grpc-go/stream.go

Lines 458 to 470 in 1cd2346

	if !cs.desc.ClientStreams || cs.desc.ServerStreams {
	return
	}
	// Special handling for client streaming rpc.
	// This recv expects EOF or errors, so we don't collect inPayload.
	if cs.c.maxReceiveMessageSize == nil {
	return status.Errorf(codes.Internal, "callInfo maxReceiveMessageSize field uninitialized(nil)")
	}
	err = recv(cs.p, cs.codec, cs.s, cs.dc, m, *cs.c.maxReceiveMessageSize, nil, cs.decomp)
	cs.closeTransportStream(err)
	if err == nil {
	return toRPCErr(errors.New("grpc: client streaming protocol violation: get <nil>, want <EOF>"))
	}

Note that they will have subtly different behavior even though they should be the same. If the server returns two payloads, for a unary RPC we will ignore the first payload, but for a client-only stream, we will return a protocol violation error.

[jhump]

Yes, please! This also allows users of the libraries to implement interceptors just once instead of 2x (since the current formulation requires two separate interceptors for any cross-cutting functionality: one for unary RPCs and one for streaming RPCs).

Admittedly, if users want to supply unary-only interceptors in the current simplified form, it would be nice if grpc-go still supported that mechanism. That would also provide backwards compatibility with the current APIs. There should also be a way to register stream interceptors that (unlike with current APIs), are triggered for unary RPCs, not just streaming RPCs. That would also address #1494.

A similar bifurcation exists in the server-side, which requires server interceptors to be written twice, too. If the server side behaved similarly to the proposed client-side change, then server dispatch could register a grpc.ServerStream in the context instead of a *transport.Stream, which would also be a step towards addressing #1802.

[dfawley]

@jhump to maintain backward compatibility, we will have to continue supporting interceptors exactly the way they currently work. It might be possible to introduce a new type of interceptor that works for both unary and streaming RPCs, but that would be a different feature request.

Independently, we're talking about some CallOption changes as a follow-on to #1794 that would fix #1494.

[jhump]

introduce a new type of interceptor

I think that new type of interceptor would basically look exactly like the stream interceptor.

For backwards compatibility, instead of a new type, I think you'd just need a new dial option (new exported method). The new method would configure the client to use a stream interceptor for unary RPCs. Or perhaps a new method that registers a stream interceptor that is used for both streaming and unary RPCs.

I'll file a new issue to describe this feature request.

[jhump]

The name of this issue does not indicate that it is client-only. Is that the intent? If so, I can add a new issue for the server-side analog. I think it's quite straight-forward in the server, too, with a stream handler that does so:

// error handling elided for clarity
m := new(RequestType)
req, err := stream.RecvMsg(m)
resp, err := svr.CallMethod(stream.Context(), req)
stream.SendMsg(resp)

[dfawley]

I think that new type of interceptor would basically look exactly like the stream interceptor.

Most likely yes. I meant "type" as in "variety", not as in "Go type system". A new DialOption to register an interceptor for unary and streaming, that has (maybe?) the same signature as streaming interceptors, seems like the most reasonable option to me.

I'll file a new issue to describe this feature request.

Sounds good, thanks.

The name of this issue does not indicate that it is client-only. Is that the intent?

This issue is intended to be for an internal-only cleanup to delete the bulk of call.go. No APIs or behavior should be affected (unless there are bug fixes / minor tweaks to error cases like the example in the first comment).