From d6bf9e0f05bd47f370e78ee20b65dbc3eb28628a Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Thu, 7 Jan 2021 17:27:56 -0800 Subject: [PATCH 1/3] Make the timing restriction of subchannel creation stricter. Throw at subchannel creation if the channel is being shutting down and the delayed transport is terminated (aka, all retry calls has been finished). --- core/src/main/java/io/grpc/internal/ManagedChannelImpl.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java index 6dccd7aca8b..db1045be7cf 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java @@ -1425,8 +1425,8 @@ public AbstractSubchannel createSubchannel(CreateSubchannelArgs args) { } private SubchannelImpl createSubchannelInternal(CreateSubchannelArgs args) { - // TODO(ejona): can we be even stricter? Like loadBalancer == null? - checkState(!terminated, "Channel is terminated"); + // No new subchannel should be created after delayedTransport has been shutdown. + checkState(!terminating, "Channel is being terminated"); return new SubchannelImpl(args, this); } From 666aebd213d1ba965e7600705f1226ce8e39f439 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Tue, 12 Jan 2021 12:48:16 -0800 Subject: [PATCH 2/3] Minor cleanup. --- core/src/main/java/io/grpc/internal/ManagedChannelImpl.java | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java index 7e46e555f43..a22e8c8cb2a 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java @@ -1391,11 +1391,7 @@ private class LbHelperImpl extends LoadBalancer.Helper { @Override public AbstractSubchannel createSubchannel(CreateSubchannelArgs args) { syncContext.throwIfNotInThisSynchronizationContext(); - return createSubchannelInternal(args); - } - - private SubchannelImpl createSubchannelInternal(CreateSubchannelArgs args) { - // No new subchannel should be created after delayedTransport has been shutdown. + // No new subchannel should be created after load balancer has been shutdown. checkState(!terminating, "Channel is being terminated"); return new SubchannelImpl(args, this); } From b775db0a97fbbb349ebfb7ff0c1b79c1b492ba41 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Tue, 12 Jan 2021 17:21:13 -0800 Subject: [PATCH 3/3] Simplify SubchannelImpl's start by removing handlings that should never happen. --- .../java/io/grpc/internal/ManagedChannelImpl.java | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java index a22e8c8cb2a..fb9e3bb9f01 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java @@ -1819,18 +1819,8 @@ private final class SubchannelImpl extends AbstractSubchannel { private void internalStart(final SubchannelStateListener listener) { checkState(!started, "already started"); checkState(!shutdown, "already shutdown"); + checkState(!terminating, "Channel is being terminated"); started = true; - // TODO(zhangkun): possibly remove the volatile of terminating when this whole method is - // required to be called from syncContext - if (terminating) { - syncContext.execute(new Runnable() { - @Override - public void run() { - listener.onSubchannelState(ConnectivityStateInfo.forNonError(SHUTDOWN)); - } - }); - return; - } final class ManagedInternalSubchannelCallback extends InternalSubchannel.Callback { // All callbacks are run in syncContext @Override