v2: Client unary interceptor timeout on v2 branch #330

instabledesign · 2020-09-01T08:20:32Z

👋 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?

interceptors/timeout/examples_test.go

instabledesign · 2020-09-01T13:02:30Z

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 · 2020-09-01T13:16:14Z

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 · 2020-09-01T13:42:11Z

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 · 2020-09-01T13:44:47Z

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 · 2020-09-01T14:52:51Z

interceptors/timeout/timeout.go

+func TimeoutUnaryClientInterceptor(timeout 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, timeout)
+		defer cancel()


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

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)

Cool: grpc/grpc-go#2130

bwplotka

LGTM!

Some tests are not working though

bwplotka · 2020-09-02T10:50:14Z

interceptors/timeout/examples_test.go

+		log.Fatal(err)
+	}
+
+	// Initialize your grpc service with connection


missing trailing period on all comments here (:

instabledesign · 2020-09-03T09:59:45Z

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 · 2020-09-03T10:01:57Z

interceptors/timeout/timeout.go

+		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.

bwplotka · 2020-09-03T11:58:00Z

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 · 2020-09-04T07:07:48Z

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 · 2020-09-04T08:46:00Z

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 · 2020-09-08T09:36:16Z

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 · 2020-09-08T10:36:12Z

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 · 2021-01-17T10:55:46Z

Tests has to pass as well (:

Co-authored-by: Johan Brandhorst-Satzkorn <johan.brandhorst@gmail.com>

google-cla · 2021-03-26T15:02:21Z

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 · 2021-03-26T15:25:17Z

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 · 2021-03-26T15:32:08Z

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

instabledesign · 2021-03-26T15:32:28Z

@googlebot I consent.

google-cla · 2021-03-26T15:32:36Z

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

eko approved these changes Sep 1, 2020

View reviewed changes

johanbrandhorst reviewed Sep 1, 2020

View reviewed changes

interceptors/timeout/examples_test.go Outdated Show resolved Hide resolved

gusmin approved these changes Sep 1, 2020

View reviewed changes

johanbrandhorst requested changes Sep 1, 2020

View reviewed changes

bwplotka reviewed Sep 1, 2020

View reviewed changes

bwplotka approved these changes Sep 2, 2020

View reviewed changes

bwplotka reviewed Sep 2, 2020

View reviewed changes

johanbrandhorst reviewed Sep 3, 2020

View reviewed changes

bwplotka approved these changes Jan 17, 2021

View reviewed changes

bwplotka changed the title ~~Client unary interceptor timeout on v2 branch~~ v2: Client unary interceptor timeout on v2 branch Jan 17, 2021

instabledesign and others added 9 commits March 26, 2021 16:01

Client unary interceptor timeout on v2 branch

770ee92

change fmt.println with log.Println in example (linter fix)

5f87a55

fix test

306b506

Update interceptors/timeout/examples_test.go

fdda9bc

Co-authored-by: Johan Brandhorst-Satzkorn <johan.brandhorst@gmail.com>

fixup example linter

0fc1497

fix when context already has timeout

f14115f

fixup trailing period

bfa7786

try increase timing

68a56d2

delete deadline check

2f77e2d

instabledesign added 2 commits March 26, 2021 16:01

update comment

48deb42

add readme timeout note

bef9f01

instabledesign force-pushed the timeout-unary-interceptor-v2 branch from 66b17d6 to bef9f01 Compare March 26, 2021 15:02

fixup test

e1e3186

bwplotka approved these changes Mar 26, 2021

View reviewed changes

bwplotka merged commit 524ce8f into grpc-ecosystem:v2 Mar 26, 2021

instabledesign deleted the timeout-unary-interceptor-v2 branch March 27, 2021 17:18

aimuz mentioned this pull request Mar 8, 2023

docs: add logging.InjectFields usage description #540

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v2: Client unary interceptor timeout on v2 branch #330

v2: Client unary interceptor timeout on v2 branch #330

instabledesign commented Sep 1, 2020

johanbrandhorst left a comment

instabledesign commented Sep 1, 2020

johanbrandhorst commented Sep 1, 2020

johanbrandhorst left a comment

instabledesign commented Sep 1, 2020

johanbrandhorst commented Sep 1, 2020

bwplotka left a comment

bwplotka Sep 1, 2020

instabledesign Sep 1, 2020

bwplotka Sep 2, 2020

bwplotka left a comment

bwplotka Sep 2, 2020

instabledesign commented Sep 3, 2020

johanbrandhorst Sep 3, 2020

bwplotka commented Sep 3, 2020 via email

instabledesign commented Sep 4, 2020

johanbrandhorst commented Sep 4, 2020

instabledesign commented Sep 8, 2020

johanbrandhorst commented Sep 8, 2020

bwplotka left a comment

bwplotka commented Jan 17, 2021

google-cla bot commented Mar 26, 2021

google-cla bot commented Mar 26, 2021

instabledesign commented Mar 26, 2021

instabledesign commented Mar 26, 2021

google-cla bot commented Mar 26, 2021

bwplotka left a comment

v2: Client unary interceptor timeout on v2 branch #330

v2: Client unary interceptor timeout on v2 branch #330

Conversation

instabledesign commented Sep 1, 2020

johanbrandhorst left a comment

Choose a reason for hiding this comment

instabledesign commented Sep 1, 2020

johanbrandhorst commented Sep 1, 2020

johanbrandhorst left a comment

Choose a reason for hiding this comment

instabledesign commented Sep 1, 2020

johanbrandhorst commented Sep 1, 2020

bwplotka left a comment

Choose a reason for hiding this comment

bwplotka Sep 1, 2020

Choose a reason for hiding this comment

instabledesign Sep 1, 2020

Choose a reason for hiding this comment

bwplotka Sep 2, 2020

Choose a reason for hiding this comment

bwplotka left a comment

Choose a reason for hiding this comment

bwplotka Sep 2, 2020

Choose a reason for hiding this comment

instabledesign commented Sep 3, 2020

johanbrandhorst Sep 3, 2020

Choose a reason for hiding this comment

bwplotka commented Sep 3, 2020 via email

instabledesign commented Sep 4, 2020

johanbrandhorst commented Sep 4, 2020

instabledesign commented Sep 8, 2020

johanbrandhorst commented Sep 8, 2020

bwplotka left a comment

Choose a reason for hiding this comment

bwplotka commented Jan 17, 2021

google-cla bot commented Mar 26, 2021

google-cla bot commented Mar 26, 2021

instabledesign commented Mar 26, 2021

instabledesign commented Mar 26, 2021

google-cla bot commented Mar 26, 2021

bwplotka left a comment

Choose a reason for hiding this comment