core: make NameResolver not thread-safe #5364

zhangkun83 · 2019-02-13T23:18:00Z

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 · 2019-02-15T17:59:45Z

core/src/main/java/io/grpc/internal/DnsNameResolver.java

+  private final Stopwatch stopwatch;
+  private final SynchronizationContext syncContext;
+
+  // Following fields must be accessed from synContext


s/synContext/syncContext/

ejona86 · 2019-02-15T18:00:04Z

core/src/main/java/io/grpc/internal/DnsNameResolver.java

  private boolean resolving;
-  @GuardedBy("this")
  private Listener listener;


It looks like this can be accessed outside of the syncContext; it's only changed in start().

The field will only be accessed from syncContext, while it can be called from any thread. I have made it more clear in the comments.

I'm saying that listener can actually be accessed from any thread. It doesn't change.

ejona86 · 2019-02-15T18:04:56Z

core/src/main/java/io/grpc/internal/DnsNameResolver.java

@@ -138,17 +138,17 @@
  private final String host;
  private final int port;
  private final Resource<Executor> executorResource;
-  @GuardedBy("this")
+  private final long cacheTtlNanos;
+  private final Stopwatch stopwatch;


This should only be accessed from syncContext.

carl-mastrangelo · 2019-02-13T23:20:24Z

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

@@ -214,13 +218,27 @@ void onAddresses(
    /**
     * The port number used in case the target or the underlying naming system doesn't provide a
     * port number.
+     *
+     * @since 1.19.0


Independently of this PR, you should probably backport this to v1.19.x

Sure: #5367

carl-mastrangelo · 2019-02-13T23:20:36Z

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

@@ -37,10 +37,14 @@
 * {@link Listener} is responsible for eventually (after an appropriate backoff period) invoking
 * {@link #refresh()}.
 *
+ * <p>Implementations <string>don't need to be thread-safe</strong>.  All methods are guaranteed to


s/string/strong/

Fixed. Blame muscle memory :P

carl-mastrangelo · 2019-02-15T22:03:54Z

core/src/main/java/io/grpc/internal/DnsNameResolver.java

+  private final SynchronizationContext syncContext;
+
+  // Following fields must be accessed from synContext
+  private ResolutionResults cachedResolutionResults = null;


remove the null. Its the default and surprising to see it.

carl-mastrangelo · 2019-02-15T22:05:38Z

core/src/main/java/io/grpc/internal/DnsNameResolver.java

-    this.resolveRunnable = new Resolve(this, stopwatch, getNetworkAddressCacheTtlNanos(isAndroid));
+    this.cacheTtlNanos = getNetworkAddressCacheTtlNanos(isAndroid);
+    this.stopwatch = Preconditions.checkNotNull(stopwatch, "stopwatch");
+    this.syncContext =


This is a little uncomfortable to me. It would be better if the ctor was dumber, and the DNRP constructed the dependencies for this class, rather than DNR constructing its own. Same with proxyDetector.

I am not sure what you are suggesting. Are you suggesting instead of passing Helper to the ctor, passing the defaultPort, proxyDetector and syncContext? I don't see much benefit of it rather than a longer argument list. There will be more stuff on Helper that DNR needs to look at. Flatting them out doesn't seem favorable.

This new form was much clearer for me, because they are the same for every resolver. Having them in the resolver is harder to reason about because I then have to check if they change. I'd actually prefer if Listener was removed from Resolver as well.

Okay then, I think helper needs a checkNotNull too, then.

carl-mastrangelo · 2019-02-15T22:06:21Z

core/src/main/java/io/grpc/internal/DnsNameResolver.java

    Preconditions.checkState(this.listener == null, "already started");
    executor = SharedResourceHolder.get(executorResource);
    this.listener = Preconditions.checkNotNull(listener, "listener");
    resolve();
  }

  @Override
-  public final synchronized void refresh() {
+  public final void refresh() {


This does not need to be final, since the class is final.

Removed final on all methods like this.

carl-mastrangelo · 2019-02-15T22:09:28Z

core/src/main/java/io/grpc/internal/DnsNameResolver.java

+            public void run() {
+              cachedResolutionResults = results;
+            }
+          });
        if (cacheTtlNanos > 0) {
          stopwatch.reset().start();


Can stopwatch be accessed by multiple threads?

Good catch. Moved into syncContext.

zhangkun83

Thanks for the review. PTAL

zhangkun83 · 2019-02-16T00:18:25Z

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

@@ -37,10 +37,14 @@
 * {@link Listener} is responsible for eventually (after an appropriate backoff period) invoking
 * {@link #refresh()}.
 *
+ * <p>Implementations <string>don't need to be thread-safe</strong>.  All methods are guaranteed to


Fixed. Blame muscle memory :P

zhangkun83 · 2019-02-16T00:19:44Z

core/src/main/java/io/grpc/internal/DnsNameResolver.java

@@ -138,17 +138,17 @@
  private final String host;
  private final int port;
  private final Resource<Executor> executorResource;
-  @GuardedBy("this")
+  private final long cacheTtlNanos;
+  private final Stopwatch stopwatch;


zhangkun83 · 2019-02-16T00:21:10Z

core/src/main/java/io/grpc/internal/DnsNameResolver.java

+  private final Stopwatch stopwatch;
+  private final SynchronizationContext syncContext;
+
+  // Following fields must be accessed from synContext


zhangkun83 · 2019-02-16T00:21:14Z

core/src/main/java/io/grpc/internal/DnsNameResolver.java

+  private final SynchronizationContext syncContext;
+
+  // Following fields must be accessed from synContext
+  private ResolutionResults cachedResolutionResults = null;


zhangkun83 · 2019-02-16T00:22:32Z

core/src/main/java/io/grpc/internal/DnsNameResolver.java

  private boolean resolving;
-  @GuardedBy("this")
  private Listener listener;


The field will only be accessed from syncContext, while it can be called from any thread. I have made it more clear in the comments.

zhangkun83 · 2019-02-16T00:25:57Z

core/src/main/java/io/grpc/internal/DnsNameResolver.java

-    this.resolveRunnable = new Resolve(this, stopwatch, getNetworkAddressCacheTtlNanos(isAndroid));
+    this.cacheTtlNanos = getNetworkAddressCacheTtlNanos(isAndroid);
+    this.stopwatch = Preconditions.checkNotNull(stopwatch, "stopwatch");
+    this.syncContext =


I am not sure what you are suggesting. Are you suggesting instead of passing Helper to the ctor, passing the defaultPort, proxyDetector and syncContext? I don't see much benefit of it rather than a longer argument list. There will be more stuff on Helper that DNR needs to look at. Flatting them out doesn't seem favorable.

zhangkun83 · 2019-02-16T00:28:32Z

core/src/main/java/io/grpc/internal/DnsNameResolver.java

    Preconditions.checkState(this.listener == null, "already started");
    executor = SharedResourceHolder.get(executorResource);
    this.listener = Preconditions.checkNotNull(listener, "listener");
    resolve();
  }

  @Override
-  public final synchronized void refresh() {
+  public final void refresh() {


Removed final on all methods like this.

zhangkun83 · 2019-02-16T00:28:47Z

core/src/main/java/io/grpc/internal/DnsNameResolver.java

+            public void run() {
+              cachedResolutionResults = results;
+            }
+          });
        if (cacheTtlNanos > 0) {
          stopwatch.reset().start();


Good catch. Moved into syncContext.

zhangkun83 · 2019-02-16T01:18:35Z

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

@@ -214,13 +218,27 @@ void onAddresses(
    /**
     * The port number used in case the target or the underlying naming system doesn't provide a
     * port number.
+     *
+     * @since 1.19.0


Sure: #5367

ejona86 · 2019-02-16T01:28:11Z

core/src/main/java/io/grpc/internal/DnsNameResolver.java

+  private final SynchronizationContext syncContext;
+
+  // Must only be called from syncContext
+  private final Stopwatch stopwatch;


It seems better to include this in the "must be accessed from the syncContext" section.

zhangkun83

@carl-mastrangelo, PTAL
I'd like to include this change in my import this week.

carl-mastrangelo

One comment but LGTM

carl-mastrangelo · 2019-02-20T18:12:04Z

core/src/main/java/io/grpc/internal/DnsNameResolver.java

-    this.resolveRunnable = new Resolve(this, stopwatch, getNetworkAddressCacheTtlNanos(isAndroid));
+    this.cacheTtlNanos = getNetworkAddressCacheTtlNanos(isAndroid);
+    this.stopwatch = Preconditions.checkNotNull(stopwatch, "stopwatch");
+    this.syncContext =


Okay then, I think helper needs a checkNotNull too, then.

…nto nr_not_threadsafe

zhangkun83 added 2 commits February 13, 2019 15:07

core: make NameResolver not thread-safe

ff04c5b

Add comment

f034ab3

zhangkun83 requested review from ejona86 and carl-mastrangelo February 13, 2019 23:18

zhangkun83 assigned ejona86 Feb 15, 2019

ejona86 reviewed Feb 15, 2019

View reviewed changes

carl-mastrangelo reviewed Feb 15, 2019

View reviewed changes

Address feedback

4e15f97

zhangkun83 mentioned this pull request Feb 16, 2019

doc: document since 1.19.0 on NameResolver.Helper #5367

Merged

zhangkun83 commented Feb 16, 2019

View reviewed changes

ejona86 reviewed Feb 16, 2019

View reviewed changes

Relocate stopwatch

b40c7aa

zhangkun83 commented Feb 20, 2019

View reviewed changes

carl-mastrangelo approved these changes Feb 20, 2019

View reviewed changes

zhangkun83 added 2 commits February 20, 2019 11:12

checkNotNull for helper

9d7c6fd

Merge branch 'nr_not_threadsafe' of github.com:zhangkun83/grpc-java i…

10e1a2f

…nto nr_not_threadsafe

zhangkun83 merged commit b867f8e into grpc:master Feb 20, 2019

zhangkun83 deleted the nr_not_threadsafe branch February 20, 2019 19:45

zhangkun83 mentioned this pull request Feb 21, 2019

core: document that NameResolver is not thread-safe. #2918

Closed

lock bot locked as resolved and limited conversation to collaborators May 21, 2019

core: make NameResolver not thread-safe #5364

core: make NameResolver not thread-safe #5364

Conversation

zhangkun83 commented Feb 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangkun83 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangkun83 left a comment

Choose a reason for hiding this comment

carl-mastrangelo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment