v2: Client unary interceptor timeout on v2 branch

[instabledesign]

👋 Hi,
this PR is related to the first one on the v1 #329
It was reworked for the v2 branch

[johanbrandhorst]

Thanks for this PR! Could you just update the example?

[instabledesign]

what do you think if we delegate the choice/computation of the timeout through a function in order to have method specific timeout

func TimeoutUnaryClientInterceptor(timeoutProvider func(string) time.Duration) grpc.UnaryClientInterceptor {
	return func(ctx context.Context, method string, req, reply interface{}, cc *grpc.ClientConn, invoker grpc.UnaryInvoker, opts ...grpc.CallOption) error {
		timedCtx, cancel := context.WithTimeout(ctx, timeoutProvider(method))
		defer cancel()
		return invoker(timedCtx, method, req, reply, cc, opts...)
	}
}

[johanbrandhorst]

what do you think if we delegate the choice/computation of the timeout through a function in order to have method specific timeout

func TimeoutUnaryClientInterceptor(timeoutProvider func(string) time.Duration) grpc.UnaryClientInterceptor {
	return func(ctx context.Context, method string, req, reply interface{}, cc *grpc.ClientConn, invoker grpc.UnaryInvoker, opts ...grpc.CallOption) error {
		timedCtx, cancel := context.WithTimeout(ctx, timeoutProvider(method))
		defer cancel()
		return invoker(timedCtx, method, req, reply, cc, opts...)
	}
}

If the user wants method specific timeouts they can just do that when calling an RPC. I think it makes sense to have a per-grpc-client timeout configuration, so lets keep it as is.

[johanbrandhorst]

Another thought; could you implement this in terms of the ClientReportable interface, like the other interceptors do? CC @bwplotka.

[instabledesign]

I don't find this way clearer than previous
and this one is "not really" a "Reporter"

Something similar than

type TimeoutUnaryClientReportable struct {
	timeout time.Duration
}

func (t *TimeoutUnaryClientReportable) ClientReporter(ctx context.Context, reqProtoOrNil interface{}, typ interceptors.GRPCType, service string, method string) (interceptors.Reporter, context.Context) {
	timedCtx, cancel := context.WithTimeout(ctx, t.timeout)
	return &TimeoutReporter{cancel: cancel}, timedCtx
}

type TimeoutReporter struct {
	cancel context.CancelFunc
}

func (t *TimeoutReporter) PostMsgReceive(replyProto interface{}, err error, recvDuration time.Duration) {
	t.cancel()
}

func (t *TimeoutReporter) PostCall(err error, rpcDuration time.Duration) {

}

func (t *TimeoutReporter) PostMsgSend(reqProto interface{}, err error, sendDuration time.Duration) {

}

[johanbrandhorst]

I agree this is less clear. What do you think @bwplotka?

[bwplotka]

Not sure if this is really needed, let's double check. If it's then I am fine non reporter version of it - it's quite simple.

[bwplotka]

Just curious, how this is different to setting timeout on gRPC client side, or passing context with timeout?

[instabledesign]

this middleware just ensure that all grpc call has a "default timeout".
in other hand i have commit c747443 a fix where the incoming context already has timeout (it will keep it)

[bwplotka]

Cool: grpc/grpc-go#2130

[bwplotka]

LGTM!

Some tests are not working though

[bwplotka]

missing trailing period on all comments here (:

[instabledesign]

I am not really fan when test depend on timing
I just increase the timing to work on OSX too
it seems the github osx was slowest that my personal computer

[johanbrandhorst]

I don't think this is the right design, adding a new context timeout to a context with an existing timeout isn't going to overwrite it, they will just both be active. I think we can remove this.

[bwplotka]

Yea, ideally we want to remove all time-based tests, it's kind of legacy. Help wanted (:

…

On Thu, 3 Sep 2020 at 11:02, Johan Brandhorst-Satzkorn < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In interceptors/timeout/timeout.go <#330 (comment)> : > + if _, ok := ctx.Deadline(); ok { + return invoker(ctx, method, req, reply, cc, opts...) + } I don't think this is the right design, adding a new context timeout to a context with an existing timeout isn't going to overwrite it, they will just both be active. I think we can remove this. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#330 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABVA3O4PMNRAMJ6SEECLQQ3SD5SSLANCNFSM4QRNVWLA> .

[instabledesign]

But the main purpose of this middleware is to deal with time.
Have you any suggestion about how it can be done?
I think used channel, but it doesn't fix the middleware timer

[johanbrandhorst]

Not sure there's much choice here since we're using context.WithTimeout directly, and we can't inject a custom clock or anything. If we wanted to make it testable you'd make the context creator configurable, so that in a test you could replace it with something deterministic, while in production use it'd just use context.WithTimeout. Personally I'm not sure that level of indirection is necessary. Can we just use single millisecond sleeps and timeouts?

[instabledesign]

i agree with you, i am not really fan of doing such things only for testability purpose.

But if it stabilize tests suite, i can do a pattern option to customize the context.WithTimeout creation.

[johanbrandhorst]

Nah that is only going to risk users abusing it. Unless @bwplotka has a concrete suggestion I think we need to do a timing test here.

[bwplotka]

Make sense!

Can you add entry to README.md as well? 🤗

Thanks 💪🏽 Sorry for lag in review! Let's get v2 released!

[bwplotka]

Tests has to pass as well (:

[google-cla]

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

[google-cla]

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

[instabledesign]

Hi!!
I rebase from v2, and update tests accordling to v2 changes
Hope its enough

[instabledesign]

@googlebot I consent.

[google-cla]

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

[bwplotka]

Ultra simple, but great! (: Thanks. LGTM