From cbb2d4c7b0e4598856f20b5ba4cdd2ed2af95e67 Mon Sep 17 00:00:00 2001 From: Kun Zhang Date: Fri, 14 Apr 2017 17:13:51 -0700 Subject: [PATCH] core: document that NameResolver is not thread-safe. Resolves #2649. This makes resolvers easier to implement. --- core/src/main/java/io/grpc/NameResolver.java | 6 +- .../io/grpc/internal/DnsNameResolver.java | 60 +++++++------------ 2 files changed, 27 insertions(+), 39 deletions(-) diff --git a/core/src/main/java/io/grpc/NameResolver.java b/core/src/main/java/io/grpc/NameResolver.java index 0aa73ca82e8..a0cf2c113c7 100644 --- a/core/src/main/java/io/grpc/NameResolver.java +++ b/core/src/main/java/io/grpc/NameResolver.java @@ -44,9 +44,12 @@ * *

The addresses and attributes of a target may be changed over time, thus the caller registers a * {@link Listener} to receive continuous updates. + * + *

Thread-safety: {@link #getServiceAuthority} may be called concurrently. Other methods on + * {@link NameResolver} don't need to be thread-safe. {@link Factory} and {@link Listener} are + * thread-safe. */ @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1770") -@ThreadSafe public abstract class NameResolver { /** * Returns the authority used to authenticate connections to servers. It must be @@ -83,6 +86,7 @@ public abstract class NameResolver { */ public void refresh() {} + @ThreadSafe public abstract static class Factory { /** * The port number used in case the target or the underlying naming system doesn't provide a diff --git a/core/src/main/java/io/grpc/internal/DnsNameResolver.java b/core/src/main/java/io/grpc/internal/DnsNameResolver.java index 23cea0333bc..ccbdee7e2d0 100644 --- a/core/src/main/java/io/grpc/internal/DnsNameResolver.java +++ b/core/src/main/java/io/grpc/internal/DnsNameResolver.java @@ -49,7 +49,6 @@ import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; import javax.annotation.Nullable; -import javax.annotation.concurrent.GuardedBy; /** * A DNS-based {@link NameResolver}. @@ -65,17 +64,11 @@ class DnsNameResolver extends NameResolver { private final int port; private final Resource timerServiceResource; private final Resource executorResource; - @GuardedBy("this") private boolean shutdown; - @GuardedBy("this") private ScheduledExecutorService timerService; - @GuardedBy("this") private ExecutorService executor; - @GuardedBy("this") private ScheduledFuture resolutionTask; - @GuardedBy("this") private boolean resolving; - @GuardedBy("this") private Listener listener; DnsNameResolver(@Nullable String nsAuthority, String name, Attributes params, @@ -111,7 +104,7 @@ public final String getServiceAuthority() { } @Override - public final synchronized void start(Listener listener) { + public final void start(Listener listener) { Preconditions.checkState(this.listener == null, "already started"); timerService = SharedResourceHolder.get(timerServiceResource); executor = SharedResourceHolder.get(executorResource); @@ -120,7 +113,7 @@ public final synchronized void start(Listener listener) { } @Override - public final synchronized void refresh() { + public final void refresh() { Preconditions.checkState(listener != null, "not started"); resolve(); } @@ -130,18 +123,16 @@ public final synchronized void refresh() { public void run() { InetAddress[] inetAddrs; Listener savedListener; - synchronized (DnsNameResolver.this) { - // If this task is started by refresh(), there might already be a scheduled task. - if (resolutionTask != null) { - resolutionTask.cancel(false); - resolutionTask = null; - } - if (shutdown) { - return; - } - savedListener = listener; - resolving = true; + // If this task is started by refresh(), there might already be a scheduled task. + if (resolutionTask != null) { + resolutionTask.cancel(false); + resolutionTask = null; } + if (shutdown) { + return; + } + savedListener = listener; + resolving = true; try { if (System.getenv("GRPC_PROXY_EXP") != null) { EquivalentAddressGroup server = @@ -153,16 +144,14 @@ public void run() { try { inetAddrs = getAllByName(host); } catch (UnknownHostException e) { - synchronized (DnsNameResolver.this) { - if (shutdown) { - return; - } - // Because timerService is the single-threaded GrpcUtil.TIMER_SERVICE in production, - // we need to delegate the blocking work to the executor - resolutionTask = - timerService.schedule(new LogExceptionRunnable(resolutionRunnableOnExecutor), - 1, TimeUnit.MINUTES); + if (shutdown) { + return; } + // Because timerService is the single-threaded GrpcUtil.TIMER_SERVICE in production, + // we need to delegate the blocking work to the executor + resolutionTask = + timerService.schedule(new LogExceptionRunnable(resolutionRunnableOnExecutor), + 1, TimeUnit.MINUTES); savedListener.onError(Status.UNAVAILABLE.withCause(e)); return; } @@ -174,9 +163,7 @@ public void run() { } savedListener.onAddresses(servers, Attributes.EMPTY); } finally { - synchronized (DnsNameResolver.this) { - resolving = false; - } + resolving = false; } } }; @@ -184,10 +171,8 @@ public void run() { private final Runnable resolutionRunnableOnExecutor = new Runnable() { @Override public void run() { - synchronized (DnsNameResolver.this) { - if (!shutdown) { - executor.execute(resolutionRunnable); - } + if (!shutdown) { + executor.execute(resolutionRunnable); } } }; @@ -198,7 +183,6 @@ InetAddress[] getAllByName(String host) throws UnknownHostException { return InetAddress.getAllByName(host); } - @GuardedBy("this") private void resolve() { if (resolving || shutdown) { return; @@ -207,7 +191,7 @@ private void resolve() { } @Override - public final synchronized void shutdown() { + public final void shutdown() { if (shutdown) { return; }