server: fix ChainUnaryInterceptor and ChainStreamInterceptor to allow retrying handlers

[yiminc]

Make it possible to retry UnaryServerInterceptor in chained UnaryInterceptors.

RELEASE NOTES:

• server: fix ChainUnaryInterceptor and ChainStreamInterceptor to allow retrying handlers

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: yiminc / name: Yimin Chen (1d05928)

[easwars]

Could you please tell us what these changes are meant for? It is not clear from reading the changes in the PR.

[github-actions]

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

[yiminc]

If you have one interceptor in a chained interceptors that has retry logic, it will skip all subsequent interceptors. See the added unit test, if the interceptor i1 has retry logic when it sees retry-able error, the retry will skip interceptor i2 without this fix.

[easwars]

If you have one interceptor in a chained interceptors that has retry logic

What do you mean by retry logic? How do we know if an interceptor has retry logic?

I'm not being rude here. But I don't think this change makes any sense. If you are interesting in adding functionality like this, please open an issue and have a discussion with us before sending us a PR.

[yiminc]

You could have a generic interceptor that does automatic retry on some errors like:

func RetryIntercept(
	ctx context.Context,
	req interface{},
	info *grpc.UnaryServerInfo,
	handler grpc.UnaryHandler,
) (interface{}, error) {
	var response interface{}
	op := func(ctx context.Context) error {
		var err error
		response, err = handler(ctx, req) // When this is called by retry, it skips subsequent interceptors.
		return err
	}

	err := backoff.RetryWithContext(ctx, op, retryPolicy)
	return response, err
}

Then you chain this interceptor with your other interceptors:
chainedInterceptor := grpc.ChainUnaryInterceptor(RetryIntercept, fooIntercept, barIntercept)
And you use chainedInterceptor as the final interceptor, when handler(ctx, req) is called for retry by RetryIntercept, the fooIntercept will be skipped. This is a problem if fooIntercept setup something in the context that barIntercept expect, barIntercept will fail to find that value from context.

@easwars

[dfawley]

Can you explain why you're retrying on a server? I can understand retrying on the client, but how and why would you retry on a server?

[github-actions]

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

[yiminc]

This could be used as a generic way to retry all APIs on specific error types. So instead of having retry logic all over the places, we could have one interceptor that do the generic retry.
The current behavior is surprising because it skips some interceptors and caused unexpected results.

[easwars]

I looked at how interceptors are chained on the client. See: https://github.com/grpc/grpc-go/blob/fdcc01b8c12bf76885cfe93e2c3dd6080a655d81/clientconn.go#L335:6. This sets the invoker to getChainUnaryInvoker which uses a recursive logic to run the next interceptor in the chain.

The chaining logic on the server does not use a recursive mechanism, but keeps some local state which is modified as part of the execution of every interceptor in the chain. And this is the reason why retrying on the server side does not work as expected (although we still don't understand when and where retrying on the server would be helpful).

We discussed this, and we think that changing the server implementation to mimic the client implementation would be better fix. Would you be interested in making that change?

[yiminc]

@easwars thanks for spending time on this. Yes, the bug was because the server side logic uses that local variable. The fix is to properly decrement after each invocation. This mimic the behavior of a recursive behavior.
I have updated the PR to use a regular recursive instead.

[easwars]

@dfawley : For second set of eyes.