core: document that NameResolver is not thread-safe.

core/src/main/java/io/grpc/NameResolver.java

@@ -44,9 +44,12 @@
  *
  * <p>The addresses and attributes of a target may be changed over time, thus the caller registers a
  * {@link Listener} to receive continuous updates.
+ *
+ * <p>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 <strong>must</strong> 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

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<ScheduledExecutorService> timerServiceResource;
   private final Resource<ExecutorService> 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,20 +163,16 @@ public void run() {
           }
           savedListener.onAddresses(servers, Attributes.EMPTY);
         } finally {
-          synchronized (DnsNameResolver.this) {
-            resolving = false;
-          }
+          resolving = false;
         }
       }
     };
 
   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;
     }