client: export types implementing CallOptions for access by interceptors #1902

jhump · 2018-03-07T16:07:03Z

This fixes #1495.

If I understood correctly, @dfawley, this is the approach you were suggesting. Can you take a look and let me know?

thelinuxfoundation · 2018-03-07T16:07:04Z

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

User @jhump isn't covered by a CLA. They will need to complete the form at https://identity.linuxfoundation.org/projects/cncf

Regards,
CLA GitHub bot

jhump · 2018-03-07T16:08:37Z

credentials/alts/utils.go

@@ -44,7 +44,7 @@ const (
 type platformError string

 func (k platformError) Error() string {
-	return fmt.Sprintf("%v is not supported", k)
+	return fmt.Sprintf("%s is not supported", string(k))


I'm guessing this test doesn't run in CI? This resulted in a stack overflow because this fmt.Sprintf call effectively recursed back into this Error() method. (I discovered this when I tried to run all tests locally, and this was the only package that failed.)

+@cesarghali

This should be a separate PR.

#1906 is merged. Please rebase this PR.

jhump · 2018-03-07T16:56:03Z

Also, any idea how I kick the cla/linuxfoundation CLA check? I agreed to CLA but don't know how to re-trigger the check so that one goes green.

I am pretty confident my change could not have induced the test failures. And I cannot reproduce any of them locally. Perhaps the tests are a little flaky and I got a bad run? Nothing in this branch would have introduced anything racy: it's very straight-forward replacing of a concrete type with an interface and one spot where I moved some code from one method to another.

dfawley · 2018-03-07T18:42:01Z

I am pretty confident my change could not have induced the test failures.

The Go1.9 run says rpc_util.go needs to be run through gofmt (this is the only build that does the lint/etc checks). The other failure is a known flake.

jhump · 2018-03-07T19:17:19Z

The Go1.9 run says rpc_util.go needs to be run through gofmt

Doh! My bad. I must admit: I didn't look at the details because I saw two jobs that both used 1.9.4, and it passed in one and failed in the other. (So I assumed it must have been some sort of flake.)

@dfawley, I addressed your comments, so I think it's ready for a review now.

…an recover the configuration

jhump · 2018-03-09T13:36:58Z

Ping?

dfawley · 2018-03-09T21:17:58Z

Sorry for the delay, and thanks for the implementation. This is, indeed, what I meant. But seeing it in practice, I'm not sure how I feel about this from a usability perspective. It may be better if the <Foo>CallOption types themselves were exported; then the user could do:

for _, co := range callOpts {
  switch o := co.(type) {
  case *grpc.HeaderCallOption: *o.HeaderAddr() = ... // or *o.HeaderAddr = ?
  case *grpc.TrailerCallOption: ...
  }
}

With this version, I think users would need to do:

for _, co := range callOpts {
  if o, ok := co.(interface{HeaderAddr() *metadata.MD}); ok {
    *o.HeaderAddr() = ... // or *o.HeaderAddr = ? or o.SetHeader()?
  } else if o, ok := co.(interface{TrailerAddr() *metadata.MD}); ok {
     ...
  }
}

With the types being exported, it would be more documented as well (i.e. the exact method signatures would show up in godoc).

What do you think?

I'd also like to mark anything we do here as experimental, and give it some time before we decide to definitely go with that approach.

jhump · 2018-03-09T21:25:18Z

What do you think?

I dig it. I didn't do it that way to start because I was worried about adding too much API surface area to support. But, admittedly, if it's documented that the thing implements a particular method, it's not too much more to just give users exported types to use, too.

As far as marking it experimental, would it suffice to just document all of the new exported types like so?

// This is an EXPERIMENTAL API.

Or should I also make note of it in the call option factory function (since its return type is part of the experimental API)?

dfawley · 2018-03-09T21:30:22Z

As far as marking it experimental, would it suffice to just document all of the new exported types like so?

Sounds good.

Or should I also make note of it in the call option factory function (since its return type is part of the experimental API)?

No; those should continue to return CallOptions as their type. If you want to mention in the docstring that it returns a particular type, then you could write something like:

// Returns a CallOption implemented by HeaderCallOption (experimental).

But that would be optional IMO.

dfawley

Thanks, this looks better! Please add some test cases in test/end2end_test.go. (Let me know if you need any help with that.)

dfawley · 2018-03-09T22:42:16Z

rpc_util.go

@@ -250,10 +343,22 @@ func PerRPCCredentials(creds credentials.PerRPCCredentials) CallOption {
 //


If WithCompressor is also set, UseCompressor has higher priority.

This raises an interesting point... we have some DialOptions that also can alter per-RPC behavior. Especially WithDefaultCallOptions.

To do this right, we should make sure there are CallOption equivalents for every appropriate DialOption, express them in terms of default CallOptions instead, and then pass those defaults along with the per-call CallOptions to the interceptors.

The other dial options (WithMaxMsgSize and WithCodec) use WithDefaultCallOptions under the hood. So exposing these all to the interceptor turned out to be pretty simple.

dfawley · 2018-03-09T22:50:38Z

rpc_util.go

+// FailFastCallOption is a CallOption for indicating whether an RPC should fail
+// fast or not.
+// This is an EXPERIMENTAL API.
+type FailFastCallOption bool


We have some CallOptions that are structs and some that are not. We should unify these since they are now exported. The struct gives us a little better extensibility, so unless there's a compelling reason to use non-structs, I would prefer structs.

dfawley · 2018-03-09T22:55:02Z

rpc_util.go

+	}
+}
+
+// HeaderAddr returns the address where header metadata will be stored after the


I'd prefer to export the field vs. add an accessor. It would be simpler to use, and these CallOption types are passed around by value anyway, so we don't have to worry about the interceptor modifying something inside the application (except the header/trailer/etc data directly). Would you have any concerns with that?

That totally makes sense. The methods only made sense when relying on caller to do an interface-type-assertion to call it. With exported types, this is much better.

jhump · 2018-03-10T23:55:32Z

Please add some test cases in test/end2end_test.go.

Since I didn't actually change behavior, it's unclear what tests you wanted me to add. I just added one that ensures that all call options (including those setup as default call options during dial) are correctly observed (and thus inspectable via exported types) by interceptors.

…dd test case

jhump · 2018-03-13T01:40:14Z

@dfawley, PTAL

jhump · 2018-03-15T21:16:24Z

ping

dfawley

Looks great, thanks! Just one comment about comments...

dfawley · 2018-03-16T19:49:19Z

rpc_util.go

+// HeaderCallOption is a CallOption for collecting response header metadata.
+// This is an EXPERIMENTAL API.
+type HeaderCallOption struct {
+	HeaderAddr *metadata.MD


Perhaps a comment to indicate this should only be consumed after the RPC has completed would be good. (And on Trailer and Peer)

I updated the comments. Hopefully the wording is to your liking. If not, I'm open to suggestions :)

…RPC ends

dfawley

Thanks @jhump!

euroelessar · 2018-03-28T08:29:17Z

call.go

@@ -27,6 +27,10 @@ import (
 //
 // All errors returned by Invoke are compatible with the status package.
 func (cc *ClientConn) Invoke(ctx context.Context, method string, args, reply interface{}, opts ...CallOption) error {
+	// allow interceptor to see all applicable call options, which means those
+	// configured as defaults from dial option as well as per-call options
+	opts = append(cc.dopts.callOptions, opts...)


@jhump @dfawley this looks like data race in case of concurrent access with non-empty opts ...CallOptions as they override the same elements in cc.dopts.callOptions (given sufficient capacity)

Good catch! See #1948

…ors (grpc#1902)

jhump commented Mar 7, 2018

View reviewed changes

jhump mentioned this pull request Mar 7, 2018

allow storing alternate transport.ServerStream implementations in context #1904

Merged

jhump force-pushed the jh/call-option-methods branch from 4fbd53b to e92eae8 Compare March 7, 2018 22:15

call options provide methods so that interceptors in other packages c…

6492fec

…an recover the configuration

jhump force-pushed the jh/call-option-methods branch from e92eae8 to 6492fec Compare March 7, 2018 22:27

This was referenced Mar 7, 2018

fix panics fullstorydev/grpchan#4

Merged

Custom transports cannot use call options or exchange response headers and trailers for unary methods fullstorydev/grpchan#6

Closed

dfawley self-assigned this Mar 8, 2018

export call option types; indicate they are experimental in docs

d42cc48

jhump force-pushed the jh/call-option-methods branch from a473fbe to d42cc48 Compare March 9, 2018 22:37

dfawley reviewed Mar 9, 2018

View reviewed changes

use structs with exported member for all call option types

927ff41

interceptor can also see default call options setup by dial option; a…

be55cbd

…dd test case

jhump force-pushed the jh/call-option-methods branch from d7d0145 to be55cbd Compare March 11, 2018 03:00

dfawley reviewed Mar 16, 2018

View reviewed changes

update comment for call option structs that only get populated after …

21fc83a

…RPC ends

dfawley approved these changes Mar 16, 2018

View reviewed changes

dfawley added the Type: Feature New features or improvements in behavior label Mar 16, 2018

dfawley merged commit fa28bef into grpc:master Mar 16, 2018

dfawley changed the title ~~Call options provide methods so interceptors in other packages can recover settings~~ client: export types implementing CallOptions for access by interceptors Mar 16, 2018

jhump mentioned this pull request Mar 21, 2018

Unary interceptors provide no mechanism to intercept response metadata #1494

Closed

menghanl added this to the 1.11 Release milestone Mar 27, 2018

euroelessar reviewed Mar 28, 2018

View reviewed changes

jhump mentioned this pull request Mar 28, 2018

client: Fix race when using both client-side default CallOptions and per-call CallOptions #1948

Merged

lyuxuan pushed a commit to lyuxuan/grpc-go that referenced this pull request Apr 4, 2018

client: export types implementing CallOptions for access by intercept…

b3f4983

…ors (grpc#1902)

lock bot locked as resolved and limited conversation to collaborators Sep 26, 2018

		@@ -250,10 +343,22 @@ func PerRPCCredentials(creds credentials.PerRPCCredentials) CallOption {
		//

client: export types implementing CallOptions for access by interceptors #1902

client: export types implementing CallOptions for access by interceptors #1902

Uh oh!

Conversation

jhump commented Mar 7, 2018

Uh oh!

thelinuxfoundation commented Mar 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhump commented Mar 7, 2018

Uh oh!

dfawley commented Mar 7, 2018

Uh oh!

jhump commented Mar 7, 2018

Uh oh!

jhump commented Mar 9, 2018

Uh oh!

dfawley commented Mar 9, 2018

Uh oh!

jhump commented Mar 9, 2018

Uh oh!

dfawley commented Mar 9, 2018

Uh oh!

dfawley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhump Mar 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhump commented Mar 10, 2018

Uh oh!

jhump commented Mar 13, 2018

Uh oh!

jhump commented Mar 15, 2018

Uh oh!

dfawley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dfawley left a comment

Choose a reason for hiding this comment

Uh oh!

euroelessar Mar 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jhump Mar 10, 2018 •

edited

Loading

euroelessar Mar 28, 2018 •

edited

Loading