Idempotent functions cannot return both data & errors

[akutz]

Long Story Short

We cannot return both a value and an error for idempotent calls. I propose we use the gRPC error details as was discussed previously. I still want to know if an idempotent result was idempotent. Short of that, we have to drop the errors completely.

This is a P0 in my opinion. We need to nail down the expected behavior prior to v0.1.0.

Long Story Long

As @cpuguy83 has pointed out, returning a non-nil result with a non-nil error is not idiomatic Go. Turns out, Go gRPC doesn't even give you a choice. While the gRPC spec appears to support the idea of returning both data and errors, the Go implementation of gRPC does not. Using CreateVolume as an example, this problem occurs in two places:

The Generated Client Code (link)

func (c *controllerClient) CreateVolume(ctx context.Context, in *CreateVolumeRequest, opts ...grpc.CallOption) (*CreateVolumeResponse, error) {
	out := new(CreateVolumeResponse)
	err := grpc.Invoke(ctx, "/csi.Controller/CreateVolume", in, out, c.cc, opts...)
	if err != nil {
		return nil, err
	}
	return out, nil
}

Note that if the error is not nil, a nil response is always returned. The thing is, I thought I could side-step this by creating my own client:

type controllerClient struct {
	csi.ControllerClient
	cc *grpc.ClientConn
}

// NewControllerClient returns a new client for the CSI controller service.
func NewControllerClient(cc *grpc.ClientConn) csi.ControllerClient {
	return &controllerClient{
		ControllerClient: csi.NewControllerClient(cc),
		cc:               cc,
	}
}

func (c *controllerClient) CreateVolume(
	ctx context.Context,
	in *csi.CreateVolumeRequest,
	opts ...grpc.CallOption) (*csi.CreateVolumeResponse, error) {

	out := new(csi.CreateVolumeResponse)
	return out, grpc.Invoke(
		ctx, "/csi.Controller/CreateVolume", in, out, c.cc, opts...)
}

This is the weird part -- it still doesn't work. So I did some more digging.

The gRPC Package Code (link)

		err = sendRequest(ctx, cc.dopts, cc.dopts.cp, c, callHdr, stream, t, args, topts)
		if err != nil {
			if done != nil {
				done(balancer.DoneInfo{
					Err:           err,
					BytesSent:     true,
					BytesReceived: stream.BytesReceived(),
				})
			}
			// Retry a non-failfast RPC when
			// i) the server started to drain before this RPC was initiated.
			// ii) the server refused the stream.
			if !c.failFast && stream.Unprocessed() {
				// In this case, the server did not receive the data, but we still
				// created wire traffic, so we should not retry indefinitely.
				if firstAyttempt {
					// TODO: Add a field to header for grpc-transparent-retry-attempts
					firstAttempt = false
					continue
				}
				// Otherwise, give up and return an error anyway.
			}
			return toRPCErr(err)
		}
		err = recvResponse(ctx, cc.dopts, t, c, stream, reply)

If the invoker receives an error then the response isn't even received and marshalled.

cc @saad-ali @jieyu @jdef @julian-hj

[jdef]

Thanks for confirming by trial/error. This is the problem that I linked to a few weeks back - that the gRPC lib encodes dropping responses when the error reported by the server app is not nil.

…

On Tue, Nov 28, 2017 at 11:49 PM, Schley Andrew Kutz < ***@***.***> wrote: Long Story Short We cannot return both a value and an error for idempotent calls. I propose we use the gRPC error details as was discussed previously. I still want to know if an idempotent result was idempotent. Short of that, we have to drop the errors completely. This is a P0 in my opinion. We need to nail down the expected behavior prior to v0.1.0. Long Story Long As @cpuguy83 <https://github.com/cpuguy83> has pointed out, returning a non-nil result with a non-nil error is not idiomatic Go. Turns out, Go gRPC doesn't even give you a choice. While the gRPC spec appears to support the idea of returning both data *and* errors, the Go implementation of gRPC does not. Using CreateVolume as an example, this problem occurs in two places: The Generated Client Code (link <https://github.com/container-storage-interface/spec/blob/master/lib/go/csi/csi.pb.go#L1898-L1905> ) func (c *controllerClient) CreateVolume(ctx context.Context, in *CreateVolumeRequest, opts ...grpc.CallOption) (*CreateVolumeResponse, error) { out := new(CreateVolumeResponse) err := grpc.Invoke(ctx, "/csi.Controller/CreateVolume", in, out, c.cc, opts...) if err != nil { return nil, err } return out, nil } Note that if the error is not nil, a nil response is always returned. The thing is, I thought I could side-step this by creating my own client: type controllerClient struct { csi.ControllerClient cc *grpc.ClientConn } // NewControllerClient returns a new client for the CSI controller service.func NewControllerClient(cc *grpc.ClientConn) csi.ControllerClient { return &controllerClient{ ControllerClient: csi.NewControllerClient(cc), cc: cc, } } func (c *controllerClient) CreateVolume( ctx context.Context, in *csi.CreateVolumeRequest, opts ...grpc.CallOption) (*csi.CreateVolumeResponse, error) { out := new(csi.CreateVolumeResponse) return out, grpc.Invoke( ctx, "/csi.Controller/CreateVolume", in, out, c.cc, opts...) } This is the weird part -- it still doesn't work. So I did some more digging. The gRPC Package Code (link <https://github.com/grpc/grpc-go/blob/master/call.go#L277-L301>) err = sendRequest(ctx, cc.dopts, cc.dopts.cp, c, callHdr, stream, t, args, topts) if err != nil { if done != nil { done(balancer.DoneInfo{ Err: err, BytesSent: true, BytesReceived: stream.BytesReceived(), }) } // Retry a non-failfast RPC when // i) the server started to drain before this RPC was initiated. // ii) the server refused the stream. if !c.failFast && stream.Unprocessed() { // In this case, the server did not receive the data, but we still // created wire traffic, so we should not retry indefinitely. if firstAyttempt { // TODO: Add a field to header for grpc-transparent-retry-attempts firstAttempt = false continue } // Otherwise, give up and return an error anyway. } return toRPCErr(err) } err = recvResponse(ctx, cc.dopts, t, c, stream, reply) If the invoker receives an error then the response isn't even received and marshalled. cc @saad-ali <https://github.com/saad-ali> @jieyu <https://github.com/jieyu> @jdef <https://github.com/jdef> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#157>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACPVLJXw9lfH8iGKkjYE3b7DvTkSwuw-ks5s7OJRgaJpZM4QuaxT> .

[jdef]

xref #115 (comment)

[akutz]

Thanks @jdef! I don’t know if it’s the same for all gRPC libs as the gRPC wire protocol supports both. My logger shows the server does return both values — it’s just Go’s gRPC client package that drops the response when there is an error (as you said).

[akutz]

Hi @saad-ali, @jdef, @julian-hj, @jieyu,

I know at least @saad-ali agrees with me that the CO having the knowledge a call was idempotent is useful for debugging. Moving forward with the assumption we are going to maintain this philosophy (and I believe we should), here is my proposal. For idempotent calls we've discussed:

1. Returning both the result and the error. This is prohibited by at least the Go gRPC client implementation.
2. Returning the result inside the error details. I favor this approach as it is idiomatic gRPC. Others did not care for it.
3. @julian-hj suggested adding a boolean field Idempotent to CreateVolumeResponse, DeleteVolumeResponse, et. al. where a response could be idempotent.

I prefer solution no. 2, but others did not like this. It's idiomatic, which is in part why I dislike no. 3 -- the knowledge that a call is idempotent is, well, metadata at best.

That's when it hit me.

For calls that can return idempotent results -- instead of returning the result and the error, which we cannot do, and in lieu of using idiomatic gRPC and wrapping the result in the error details, let's use the gRPC metadata!

I'm already using the gRPC metadata in a number of places. It would be simple to facilitate and not affect the spec at all. The following example shows how to send gRPC metadata in a server response:

header := metadata.Pairs("idempotent", "true")
grpc.SendHeader(ctx, header)

The client can then receive the metadata:

var header metadata.MD
r, err := controller.CreateVolume(
	ctx,
	&csi.CreateVolumeRequest{Name: "MyNewVolume"},
	grpc.Header(&header))

var idempotent bool
if v, ok := header["idempotent"]; ok && len(v) > 0 {
	idempotent, _ = strconv.ParseBool(v[0])
}

Basically the answer to whether a call is idempotent or not will be encoded in a gRPC response's metadata, there for the taking if someone wants to look for it.

[jieyu]

I am fine with option 2.

[cpuguy83]

Instead of setting this with metadata, it should be encoded in the status.
https://godoc.org/google.golang.org/grpc/status#Status.WithDetails

[akutz]

Hi @cpuguy83,

Instead of setting this with metadata, it should be encoded in the status.

That was option no. 2 and my preferred choice. I noted it was discussed two weeks ago and I seemed to be the only one in favor of it.