remove resolver.UnregisterForTesting usage #417
Conversation
|
|
protobuf/source_reflection.go
Outdated
| @@ -32,8 +32,7 @@ type ReflectionArgs struct { | |||
| // NewDescriptorProviderReflection returns a DescriptorProvider that reaches | |||
| // out to a reflection server to access file descriptors. | |||
| func NewDescriptorProviderReflection(args ReflectionArgs) (DescriptorProvider, error) { | |||
| r, deregisterScheme := GenerateAndRegisterManualResolver() | |||
| defer deregisterScheme() | |||
| r := GenerateAndRegisterManualResolver() | |||
There was a problem hiding this comment.
We're creating potential memory leak here.
Let's discuss potential solution with a custom resolver, i.e. to register only one resolver, and then reuse it for different services.
There was a problem hiding this comment.
I've looked into two options to address it:
Keep the manual resolver.
We can create the scheme deterministically (I'm thinking scheme = "dest-" + args.Service).
This allows us to use resolver.Get(scheme) to avoid creating unnecessary resolvers, and we also can keep an updated the list of peers in the resolver.
Use passthrough
This results in simpler code, but only allows to specify one peer.
If the peer-list file used contains outdated addresses this could make the reflection fail where otherwise it would have worked.
|
|
||
| rb := resolver.Get(scheme) | ||
| if rb != nil { | ||
| if r, ok := rb.(*manual.Resolver); ok { |
There was a problem hiding this comment.
Manual resolvers implement both, Resolver and Builder.
r.InitialState(newState) will set the new list of peers the next time that Build is called
| @@ -196,6 +196,44 @@ func TestReflectionRoutingHeaders(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestResolverAlreadyExists(t *testing.T) { | |||
There was a problem hiding this comment.
Test creating a new resolver for the same service with a different list of peers
| rb := resolver.Get(scheme) | ||
| if rb != nil { | ||
| if r, ok := rb.(*manual.Resolver); ok { | ||
| r.InitialState(newState) |
There was a problem hiding this comment.
InitialState doesn't use mutex inside, so it's a potential data race.
resolver.UnregisterForTestinghas been removed from grpc-go grpc/grpc-go#6471, this change is needed in order to update to grpc 1.59.0