core: ManagedChannelImpl2. #2530

zhangkun83 · 2016-12-17T00:10:05Z

Adapt to LoadBalancer2 interface. Channel holds on to a single
DelayedClientTransport2.
Lock-free: every channel state mutation, including Subchannel
mutations, calling into LoadBalancer, idleness and shutdown, is made
from channelExecutor.
Idleness grace period is no longer needed.

As usual, the first commit is a pure fork of ManagedChannelImpl and its tests, although the change may be so drastic so that reviewing the whole PR may be easier.

/cc @lukaszx0 @kkaneda

1. Adapt to LoadBalancer2 interface. Channel holds on to a single DelayedClientTransport2. 2. Lock-free: every channel state mutation, including Subchannel mutations, calling into LoadBalancer, idleness and shutdown, is made from channelExecutor. 3. Idleness grace period is no longer needed.

kkaneda · 2016-12-17T00:39:54Z

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

+  //   2a terminating <- true
+  //   2b loadBalancer <- null
+  //   2c nameResolver <- null
+  //   2d loadBalancer.shutdown()


nitpick: Step 2d should happen before 2b?

Yes :). Fixed.

kkaneda · 2016-12-17T00:45:54Z

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

+
+  // Must be called from channelExecutor
+  private void maybeShutdownNowSubchannels() {
+    Status nowStatus = Status.UNAVAILABLE.withDescription("Channel shutdownNow invoked");


kkaneda · 2016-12-17T00:48:52Z

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

+
+  private final AtomicBoolean shutdown = new AtomicBoolean(false);
+  // Must be mutated from channelExecutor
+  private boolean shutdownNowed;


question: is this correct that shutdownNowed doesn't need to be volatile since both read and write happen from channelExecutor?

Yes. Updated the comment to make it clear.

kkaneda · 2016-12-17T00:54:47Z

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

+
+    @Override
+    public void run() {
+      if (cancelled) {


i'm not sure, but should we also return immediately when shutdown.get() returns true?

Not necessarily.
If shutdown()'s task is scheduled before this task, the former will set cancelled to true, thus the code below will not be run.
Otherwise, this task will still run, which is totally fine.

kkaneda · 2016-12-17T02:14:11Z

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

+
+        @Override
+        public void transportTerminated() {
+          subchannelImpl.shutdown();


(still learning the code..) so we want to shut down the sub-channel when an oob-channel is terminated, not when the transport is shut down?

We shut down the sub-channel when the delayed transport (which is essentially the buffer) is terminated (which means the buffer has completed drained). If the buffer is not empty, sub-channel needs to remain open to serve the pending RPCs in the buffer.

kkaneda · 2016-12-17T02:28:40Z

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

+
+    @GuardedBy("shutdownLock")
+    boolean shutdownRequested;
+    @GuardedBy("shutdownLock")


we update delayedShutdownTask at line 839 without acquiring shutdownLock. should we remove this @GuardedBy?

It should actually be protected, because ManagedChannel.shutdown() is thread-safe.
I have updated the code below accordingly.

kkaneda · 2016-12-17T02:32:55Z

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

+      }
+      log.log(Level.FINE, "[{0}] resolved address: {1}, config={2}",
+          new Object[] {getLogId(), servers, config});
+      channelExecutor.executeLater(new Runnable() {


not sure that's better, but we can use runSerialized here..

No we can't, because the helper is not accessible here.
We can, however, use runSerialized in place of channelExecutor.executeLater() in LbHelperImpl.

Why isn't the helper accessible here? It looks like it is to me. Maybe the code changed and now it is.

Yeah, with a small API tweak now it's accessible.

kkaneda · 2016-12-17T02:46:42Z

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

+  }
+
+  @Override
+  public ManagedChannel shutdownNow() {


should we set shutdown to true here?

Good catch! Fixed.

dapengzhang0 · 2016-12-19T19:31:46Z

there was an unsed field in DelayedClientTransport2Test.java:
headers3

kkaneda

Thanks, @zhangkun83 ! Looks good to me!

kkaneda · 2016-12-20T19:02:10Z

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

+
+  private final SharedResourceHolder.Resource<ScheduledExecutorService> timerService;
+  private final Supplier<Stopwatch> stopwatchSupplier;
+  /** The timout before entering idle mode. */


nitpick: s/timout/timeout/

kkaneda · 2016-12-20T20:06:56Z

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

+    }
+    // Cancel the timer now, so that a racing due timer will not put Channel on idleness
+    // when the caller of exitIdleMode() is about to use the returned loadBalancer.
+    cancelIdleTimer();


nitpick: not sure that's better, but we can also do

if (inUseStateAggregator.isInUse()) { cancelIdleTimer(); } else { rescheduleIdleTimer(); }

since rescheduleIdleTimer calls cancelIdleTimer.

Applied suggested change.

kkaneda · 2016-12-20T21:01:14Z

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

+        // Should not be possible.
+        throw new IllegalArgumentException(e);
+      }
+      if (targetUri != null) {


nitpick: can remove this line?

Yes and removed.

kkaneda · 2016-12-20T21:14:13Z

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

+          "invalid idleTimeoutMillis %s", idleTimeoutMillis);
+      this.idleTimeoutMillis = idleTimeoutMillis;
+    }
+    this.decompressorRegistry = decompressorRegistry;


should we add checkNotNull to decompressorRegistry and compressorRegistry?

Yes. I have also added checkNotNull for several other fields.

kkaneda · 2016-12-20T21:27:53Z

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

+            nameResolver = null;
+          }
+
+          // Until LoadBalancer is shutdown, it may still create new subchannels.  We catch them


Question: Do we need to set shutdownNowed to true here?

I don't think so, since we only want to set shutdownNowed if the client requested an immediate shutdown. If shutdownNow wasn't called, we want to wait until the RPCs naturally complete.

ejona86

Not through it yet, but needing to make an appointment. The maybeShutdownNowSubchannels() comment is the main thing that needs to be changed.

ejona86 · 2016-12-22T18:10:24Z

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

+        }
+
+        @Override
+        public void transportInUse(boolean inUse) {


Ignoring this method points out that the OobChannel won't have some possibly-useful features. And the addition of the Executor shows that the configuration is rather limited. As time goes on, we may need to tweak this part. (But it's fine right now.)

ejona86 · 2016-12-22T20:01:55Z

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

+    channelExecutor.executeLater(new Runnable() {
+        @Override
+        public void run() {
+          maybeTerminateChannel();


Won't this be handled by delayedTransport's termination listener? If it is needed here, why isn't it needed in shutdownNow()?

You are right. It's not needed here. Deleted.

ejona86 · 2016-12-22T20:03:59Z

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

+      for (InternalSubchannel subchannel : subchannels) {
+        subchannel.shutdownNow(SHUTDOWN_NOW_STATUS);
+      }
+      subchannels.clear();


Should these and the oobChannels really be removed? Don't we need to wait until the transports process the shutdownNow before claiming to be terminated?

Nice catch! Removed the two clear() lines and added tests to cover this case.

ejona86 · 2016-12-22T20:08:18Z

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

+            nameResolver = null;
+          }
+
+          // Until LoadBalancer is shutdown, it may still create new subchannels.  We catch them


I don't think so, since we only want to set shutdownNowed if the client requested an immediate shutdown. If shutdownNow wasn't called, we want to wait until the RPCs naturally complete.

ejona86 · 2016-12-22T21:05:51Z

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

+          shutdownRequested = true;
+        }
+        ScheduledExecutorService scheduledExecutorCopy = scheduledExecutor;
+        // Add a delay to shutdown to deal with the race between 1) a transport being picked and


We maybe should start thinking about making newStream (or maybe start) atomic with transport selection. If clientTransportProvider created the stream as well as chose the transport, then it could deal with this race by retrying. Transports would need to help out by returning differently if they are shutdown, but we do things like that frequently already with only volatile reads in the fast path.

I don't think we need to do that now (in this PR), but this race will now be happening frequently enough that it's probably time to address it directly. And we can reuse some of the same tricks we're using for the LB rework.

I have filed #2562 to track this.

ObjectPool is simpler to use and more test-friendly than the raw SharedResourceHolder. Also assign ManagedChannel's executor to directExecutor after returning the real executor to pool.

ejona86 · 2016-12-29T18:58:29Z

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

+      terminatedLatch.countDown();
+      executorPool.returnObject(executor);
+      // Needed for delivering rejections to new calls after OobChannel is terminated.
+      executor = MoreExecutors.directExecutor();


I'm uncomfortable with calling executor after termination, as generally it is a bug. It also seems insufficient, as there is still the race between executor being read during newCall and ClientCallImpl.start(). If this is temporary until we fix the real bug, then I'm fine with it. Ditto in OobChannel.

The comment here also seems strange. Is the reference to OobChannel because it was a copy/paste that wasn't fully updated?

If newCall() is called after the channel is terminated, the call should fail, but how should we deliver the failure? Currently we deliver the failure to the call listener, which requires the app executor. As we shouldn't use the original app executor after it's returned to the pool, the direct executor seems to be the best bet. The other option is to let newCall() throw.

Yes I should've done s/OobChannel/Channel after pasting it.

This is pre-existing. I would feel very comfortable leaving it as equally broken as the current code (ManagedChannelImpl) and fixing it later. There are so many things happening already in this PR. I'm okay with a temporary hack in order to let tests work or similar.

the app's executor shouldn't be called at all after termination. That applies equally to the pool, but the pool is under our control and so we have more freedom, whereas the app's executor interaction is part of externally-visible behavior. As soon as the Channel becomes terminated the application is free to shutdown their executor. Calling the listener in the same thread seems appropriate, but using directExecutor here is missing the larger problem. The appExecutor is saved in ClientCall which is created during newCall(), but between newCall() and ClientCall.start() the Channel could become terminated.

You are right. I never thought of the race between termination and ClientCall.start(). I have reverted to the preexisting state and captured this case in #1981

ejona86

Finally finished the full pass (although I didn't really look at the tests more than a glance).

ejona86 · 2016-12-29T19:57:50Z

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

+          @Override
+          public void run() {
+            if (terminating) {
+              internalSubchannel.shutdown();


This runSerialized() block looks subtle. The runSerialized() does not guarantee that the Runnable has executed before it returns. So we could return a subchannel here that is non-yet-shutdown even when terminating. But that subchannel won't (can't?) be used because the delayed transport is already terminated and so has no RPCs and is not accepting any more RPCs (or passing-through any more RPCs).

It seems a comment would go a long way, if nothing else to prevent future bugs.

I added a comment:

Because runSerialized() doesn't guarantee the runnable has been executed upon when returning, the subchannel may still be returned to the balancer without being shutdown even if "terminating" is already true. The subchannel will not be used in this case, because delayed transport has terminated when "terminating" becomes true, and no more requests will be sent to balancer beyond this point.

ejona86 · 2016-12-29T20:00:11Z

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

+      }
+      log.log(Level.FINE, "[{0}] resolved address: {1}, config={2}",
+          new Object[] {getLogId(), servers, config});
+      channelExecutor.executeLater(new Runnable() {


Why isn't the helper accessible here? It looks like it is to me. Maybe the code changed and now it is.

Because it's handled by delayedTransport termination.

zhangkun83

I have addressed all comments. PTAL.

zhangkun83 · 2016-12-29T22:52:23Z

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

+      terminatedLatch.countDown();
+      executorPool.returnObject(executor);
+      // Needed for delivering rejections to new calls after OobChannel is terminated.
+      executor = MoreExecutors.directExecutor();


You are right. I never thought of the race between termination and ClientCall.start(). I have reverted to the preexisting state and captured this case in #1981

zhangkun83 · 2016-12-29T23:03:00Z

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

+    channelExecutor.executeLater(new Runnable() {
+        @Override
+        public void run() {
+          maybeTerminateChannel();


You are right. It's not needed here. Deleted.

zhangkun83 · 2016-12-29T23:14:56Z

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

+      }
+      log.log(Level.FINE, "[{0}] resolved address: {1}, config={2}",
+          new Object[] {getLogId(), servers, config});
+      channelExecutor.executeLater(new Runnable() {


Yeah, with a small API tweak now it's accessible.

zhangkun83 · 2016-12-29T23:23:02Z

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

+        }
+
+        @Override
+        public void transportInUse(boolean inUse) {


zhangkun83 · 2016-12-30T01:44:55Z

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

+          @Override
+          public void run() {
+            if (terminating) {
+              internalSubchannel.shutdown();


I added a comment:

Because runSerialized() doesn't guarantee the runnable has been executed upon when returning, the subchannel may still be returned to the balancer without being shutdown even if "terminating" is already true. The subchannel will not be used in this case, because delayed transport has terminated when "terminating" becomes true, and no more requests will be sent to balancer beyond this point.

zhangkun83 · 2017-01-03T18:40:02Z

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

+      for (InternalSubchannel subchannel : subchannels) {
+        subchannel.shutdownNow(SHUTDOWN_NOW_STATUS);
+      }
+      subchannels.clear();


Nice catch! Removed the two clear() lines and added tests to cover this case.

zhangkun83 · 2017-01-03T18:43:47Z

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

+    }
+    // Cancel the timer now, so that a racing due timer will not put Channel on idleness
+    // when the caller of exitIdleMode() is about to use the returned loadBalancer.
+    cancelIdleTimer();


Applied suggested change.

zhangkun83 · 2017-01-03T18:47:52Z

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

+          "invalid idleTimeoutMillis %s", idleTimeoutMillis);
+      this.idleTimeoutMillis = idleTimeoutMillis;
+    }
+    this.decompressorRegistry = decompressorRegistry;


Yes. I have also added checkNotNull for several other fields.

zhangkun83 · 2017-01-03T18:48:34Z

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

+        // Should not be possible.
+        throw new IllegalArgumentException(e);
+      }
+      if (targetUri != null) {


Yes and removed.

zhangkun83 · 2017-01-03T19:35:20Z

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

+          shutdownRequested = true;
+        }
+        ScheduledExecutorService scheduledExecutorCopy = scheduledExecutor;
+        // Add a delay to shutdown to deal with the race between 1) a transport being picked and


I have filed #2562 to track this.

zhangkun83 added 2 commits December 16, 2016 15:41

Fork ManagedChannelImpl and its tests.

a79f2c1

zhangkun83 assigned ejona86 Dec 17, 2016

kkaneda reviewed Dec 17, 2016

View reviewed changes

zhangkun83 added 4 commits December 19, 2016 16:48

Cosmetic and documentation fixes.

c0e1b2d

Bug fix: OobChannel.shutdownNow() should set shutdown to true.

5225693

Bug fix: SubchannelImplImpl should schedule the timer within the lock.

0ae084e

Remove unused field from test.

97c463b

kkaneda approved these changes Dec 20, 2016

View reviewed changes

ejona86 reviewed Dec 22, 2016

View reviewed changes

zhangkun83 added 2 commits December 28, 2016 11:42

Switch to ObjectPool.

3bbe8c3

ObjectPool is simpler to use and more test-friendly than the raw SharedResourceHolder. Also assign ManagedChannel's executor to directExecutor after returning the real executor to pool.

Add tests for returning the shared executors.

014ffc2

ejona86 reviewed Dec 29, 2016

View reviewed changes

zhangkun83 added 6 commits December 29, 2016 14:59

Stop clearing app executor when channel is terminated.

108345e

shutdown() doesn't need to call maybeTerminateChannel();

55c05f7

Because it's handled by delayedTransport termination.

Add a comment.

072b6c3

Prefer runSerialized() to channelExecutor.

c52ee3e

Do not clear subchannels and oobchannels at shutdownNow.

60bc5a2

A few no-op changes.

68578f5

zhangkun83 commented Jan 3, 2017

View reviewed changes

ejona86 approved these changes Jan 9, 2017

View reviewed changes

Merge branch 'master' into lbv2_channel_review

72ad669

zhangkun83 merged commit 7cb0497 into grpc:master Jan 9, 2017

zhangkun83 deleted the lbv2_channel_review branch January 9, 2017 22:44

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

core: ManagedChannelImpl2. #2530

core: ManagedChannelImpl2. #2530

Conversation

zhangkun83 commented Dec 17, 2016

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

dapengzhang0 commented Dec 19, 2016

kkaneda 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

ejona86 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

zhangkun83 Dec 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ejona86 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

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

zhangkun83 Dec 29, 2016 •

edited

Loading