Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

NameResolver can be NotThreadSafe #2649

Closed
zhangkun83 opened this issue Jan 24, 2017 · 1 comment · Fixed by #5364
Closed

NameResolver can be NotThreadSafe #2649

zhangkun83 opened this issue Jan 24, 2017 · 1 comment · Fixed by #5364
Assignees
@zhangkun83
Milestone
1.20

Comments

@zhangkun83
Copy link
Contributor

With ManagedChannelImpl2, NameResolver is always called from Channel Executor, except for getAuthority(). After ManagedChannelImpl2 is promoted, NameResolver can get rid of @ThreadSafe.
@zhangkun83 zhangkun83 added this to the 1.2 milestone Jan 24, 2017
@zhangkun83 zhangkun83 self-assigned this Mar 7, 2017
@carl-mastrangelo carl-mastrangelo modified the milestones: Next, 1.2 Mar 15, 2017
zhangkun83 added a commit to zhangkun83/grpc-java that referenced this issue Apr 15, 2017
@zhangkun83
core: document that NameResolver is not thread-safe.
cbb2d4c 
Resolves grpc#2649.  This makes resolvers easier to implement.
@zhangkun83 zhangkun83 mentioned this issue Apr 15, 2017
Closed
@zhangkun83
Copy link
Contributor Author

As pointed out in #2918, we would need to expose Channel Executor to the NameResolver, maybe similar to LoadBalancer.Helper.runSerialized(). Otherwise if NameResolver run a task asynchronously, it can't serialize that task with Channel Executor, thus would require other means of synchronization, which beats the purpose of making NameResolver NotThreadSafe.

@zhangkun83 zhangkun83 mentioned this issue Feb 11, 2019
Merged
zhangkun83 added a commit that referenced this issue Feb 12, 2019
@zhangkun83
core: introduce NameResolver.Helper and deprecate the params on newNa…
e875a8c 
…meResolver() (#5345)

Context: [#4159 (comment)](#4159 (comment))

`Attributes` is appropriate for plumbing optional objects, especially useful for a long plumbing path where components in the middle may not care or see all objects in the container. It's not the case for the `params` on `newNewResolver()`. Both the default port and the proxy detector are guaranteed to be there and the plumbing path is very short. In this case, a first-class object is more appropriate and easier to use.

The `Helper` will also have `getSynchronizationContext()` (#2649) and a method to parse and validate service config. We we also considering merging `Listener` into the `Helper`, to make `NameResolver` match the `LoadBalancer` API.
@zhangkun83 zhangkun83 mentioned this issue Feb 13, 2019
Merged
zhangkun83 added a commit that referenced this issue Feb 20, 2019
@zhangkun83
core: make NameResolver not thread-safe (#5364)
b867f8e 
Resolves #2649

As a prerequisite, added `getSynchronizationContext()` to `NameResolver.Helper`.

`DnsNameResolver` has gone through a small refactor around the `Resolve` runnable, which makes it a little simpler.
@ejona86 ejona86 modified the milestones: Next, 1.20 Feb 25, 2019
@lock lock bot locked as resolved and limited conversation to collaborators May 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@zhangkun83 zhangkun83

Labels
None yet
Projects
None yet
Milestone
1.20
3 participants
@ejona86 @carl-mastrangelo @zhangkun83