diff --git a/api/src/main/java/io/grpc/LoadBalancer.java b/api/src/main/java/io/grpc/LoadBalancer.java index 9ce98d76a6a..f48bf9bf162 100644 --- a/api/src/main/java/io/grpc/LoadBalancer.java +++ b/api/src/main/java/io/grpc/LoadBalancer.java @@ -16,14 +16,12 @@ package io.grpc; -import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; import com.google.common.base.MoreObjects; import com.google.common.base.Objects; import com.google.common.base.Preconditions; import java.util.ArrayList; -import java.util.Arrays; import java.util.Collections; import java.util.List; import java.util.Map; @@ -337,16 +335,9 @@ public boolean equals(Object obj) { * @param subchannel the involved Subchannel * @param stateInfo the new state * @since 1.2.0 - * @deprecated This method will be removed. Stop overriding it. Instead, pass {@link - * SubchannelStateListener} to {@link Helper#createSubchannel(CreateSubchannelArgs)} - * to receive Subchannel state updates */ - @Deprecated - public void handleSubchannelState( - Subchannel subchannel, ConnectivityStateInfo stateInfo) { - // Do nothing. If the implemetation doesn't implement this, it will get subchannel states from - // the new API. We don't throw because there may be forwarding LoadBalancers still plumb this. - } + public abstract void handleSubchannelState( + Subchannel subchannel, ConnectivityStateInfo stateInfo); /** * The channel asks the load-balancer to shutdown. No more callbacks will be called after this @@ -660,243 +651,6 @@ public boolean equals(Object other) { } } - /** - * Arguments for {@link Helper#createSubchannel(CreateSubchannelArgs)}. - * - * @since 1.21.0 - */ - @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1771") - public static final class CreateSubchannelArgs { - private final List addrs; - private final Attributes attrs; - private final SubchannelStateListener stateListener; - private final Object[][] customOptions; - - private CreateSubchannelArgs( - List addrs, Attributes attrs, Object[][] customOptions, - SubchannelStateListener stateListener) { - this.addrs = checkNotNull(addrs, "addresses are not set"); - this.attrs = checkNotNull(attrs, "attrs"); - this.customOptions = checkNotNull(customOptions, "customOptions"); - this.stateListener = checkNotNull(stateListener, "SubchannelStateListener is not set"); - } - - /** - * Returns the addresses, which is an unmodifiable list. - */ - public List getAddresses() { - return addrs; - } - - /** - * Returns the attributes. - */ - public Attributes getAttributes() { - return attrs; - } - - /** - * Get the value for a custom option or its inherent default. - * - * @param key Key identifying option - */ - @SuppressWarnings("unchecked") - public T getOption(Key key) { - Preconditions.checkNotNull(key, "key"); - for (int i = 0; i < customOptions.length; i++) { - if (key.equals(customOptions[i][0])) { - return (T) customOptions[i][1]; - } - } - return key.defaultValue; - } - - /** - * Returns the state listener. - */ - public SubchannelStateListener getStateListener() { - return stateListener; - } - - /** - * Returns a builder with the same initial values as this object. - */ - public Builder toBuilder() { - return newBuilder().setAddresses(addrs).setAttributes(attrs).setStateListener(stateListener) - .copyCustomOptions(customOptions); - } - - /** - * Creates a new builder. - */ - public static Builder newBuilder() { - return new Builder(); - } - - @Override - public String toString() { - return MoreObjects.toStringHelper(this) - .add("addrs", addrs) - .add("attrs", attrs) - .add("listener", stateListener) - .add("customOptions", Arrays.deepToString(customOptions)) - .toString(); - } - - @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1771") - public static final class Builder { - - private List addrs; - private Attributes attrs = Attributes.EMPTY; - private SubchannelStateListener stateListener; - private Object[][] customOptions = new Object[0][2]; - - Builder() { - } - - private Builder copyCustomOptions(Object[][] options) { - customOptions = new Object[options.length][2]; - System.arraycopy(options, 0, customOptions, 0, options.length); - return this; - } - - /** - * Add a custom option. Any existing value for the key is overwritten. - * - *

This is an optional property. - * - * @param key the option key - * @param value the option value - */ - public Builder addOption(Key key, T value) { - Preconditions.checkNotNull(key, "key"); - Preconditions.checkNotNull(value, "value"); - - int existingIdx = -1; - for (int i = 0; i < customOptions.length; i++) { - if (key.equals(customOptions[i][0])) { - existingIdx = i; - break; - } - } - - if (existingIdx == -1) { - Object[][] newCustomOptions = new Object[customOptions.length + 1][2]; - System.arraycopy(customOptions, 0, newCustomOptions, 0, customOptions.length); - customOptions = newCustomOptions; - existingIdx = customOptions.length - 1; - } - customOptions[existingIdx] = new Object[]{key, value}; - return this; - } - - /** - * The addresses to connect to. All addresses are considered equivalent and will be tried - * in the order they are provided. - */ - public Builder setAddresses(EquivalentAddressGroup addrs) { - this.addrs = Collections.singletonList(addrs); - return this; - } - - /** - * The addresses to connect to. All addresses are considered equivalent and will - * be tried in the order they are provided. - * - *

This is a required property. - * - * @throws IllegalArgumentException if {@code addrs} is empty - */ - public Builder setAddresses(List addrs) { - checkArgument(!addrs.isEmpty(), "addrs is empty"); - this.addrs = Collections.unmodifiableList(new ArrayList<>(addrs)); - return this; - } - - /** - * Attributes provided here will be included in {@link Subchannel#getAttributes}. - * - *

This is an optional property. Default is empty if not set. - */ - public Builder setAttributes(Attributes attrs) { - this.attrs = checkNotNull(attrs, "attrs"); - return this; - } - - /** - * Receives state changes of the created Subchannel. The listener is called from - * the {@link Helper#getSynchronizationContext Synchronization Context}. It's safe to share - * the listener among multiple Subchannels. - * - *

This is a required property. - */ - public Builder setStateListener(SubchannelStateListener listener) { - this.stateListener = checkNotNull(listener, "listener"); - return this; - } - - /** - * Creates a new args object. - */ - public CreateSubchannelArgs build() { - return new CreateSubchannelArgs(addrs, attrs, customOptions, stateListener); - } - } - - /** - * Key for a key-value pair. Uses reference equality. - */ - @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1771") - public static final class Key { - - private final String debugString; - private final T defaultValue; - - private Key(String debugString, T defaultValue) { - this.debugString = debugString; - this.defaultValue = defaultValue; - } - - /** - * Factory method for creating instances of {@link Key}. The default value of the key is - * {@code null}. - * - * @param debugString a debug string that describes this key. - * @param Key type - * @return Key object - */ - public static Key create(String debugString) { - Preconditions.checkNotNull(debugString, "debugString"); - return new Key<>(debugString, /*defaultValue=*/ null); - } - - /** - * Factory method for creating instances of {@link Key}. - * - * @param debugString a debug string that describes this key. - * @param defaultValue default value to return when value for key not set - * @param Key type - * @return Key object - */ - public static Key createWithDefault(String debugString, T defaultValue) { - Preconditions.checkNotNull(debugString, "debugString"); - return new Key<>(debugString, defaultValue); - } - - /** - * Returns the user supplied default value for this key. - */ - public T getDefault() { - return defaultValue; - } - - @Override - public String toString() { - return debugString; - } - } - } - /** * Provides essentials for LoadBalancer implementations. * @@ -910,11 +664,7 @@ public abstract static class Helper { * EquivalentAddressGroup}. * * @since 1.2.0 - * @deprecated Use {@link #createSubchannel(CreateSubchannelArgs)} instead. Note the new API - * must be called from {@link #getSynchronizationContext the Synchronization - * Context}. */ - @Deprecated public final Subchannel createSubchannel(EquivalentAddressGroup addrs, Attributes attrs) { checkNotNull(addrs, "addrs"); return createSubchannel(Collections.singletonList(addrs), attrs); @@ -935,35 +685,11 @@ public final Subchannel createSubchannel(EquivalentAddressGroup addrs, Attribute * * @throws IllegalArgumentException if {@code addrs} is empty * @since 1.14.0 - * @deprecated Use {@link #createSubchannel(CreateSubchannelArgs)} instead. Note the new API - * must be called from {@link #getSynchronizationContext the Synchronization - * Context}. */ - @Deprecated public Subchannel createSubchannel(List addrs, Attributes attrs) { throw new UnsupportedOperationException(); } - /** - * Creates a Subchannel, which is a logical connection to the given group of addresses which are - * considered equivalent. The {@code attrs} are custom attributes associated with this - * Subchannel, and can be accessed later through {@link Subchannel#getAttributes - * Subchannel.getAttributes()}. - * - *

This method must be called from the {@link #getSynchronizationContext - * Synchronization Context}, otherwise it may throw. This is to avoid the race between - * the caller and {@link SubchannelStateListener#onSubchannelState}. See #5015 for more discussions. - * - *

The LoadBalancer is responsible for closing unused Subchannels, and closing all - * Subchannels within {@link #shutdown}. - * - * @since 1.21.0 - */ - public Subchannel createSubchannel(CreateSubchannelArgs args) { - throw new UnsupportedOperationException(); - } - /** * Equivalent to {@link #updateSubchannelAddresses(io.grpc.LoadBalancer.Subchannel, List)} with * the given single {@code EquivalentAddressGroup}. @@ -1180,7 +906,7 @@ public abstract static class Subchannel { */ public final EquivalentAddressGroup getAddresses() { List groups = getAllAddresses(); - Preconditions.checkState(groups.size() == 1, "%s does not have exactly one group", groups); + Preconditions.checkState(groups.size() == 1, "Does not have exactly one group"); return groups.get(0); } @@ -1241,39 +967,6 @@ public ChannelLogger getChannelLogger() { } } - /** - * Receives state changes for one or more {@link Subchannel}s. All methods are run under {@link - * Helper#getSynchronizationContext}. - * - * @since 1.21.0 - */ - public interface SubchannelStateListener { - - /** - * Handles a state change on a Subchannel. - * - *

The initial state of a Subchannel is IDLE. You won't get a notification for the initial - * IDLE state. - * - *

If the new state is not SHUTDOWN, this method should create a new picker and call {@link - * Helper#updateBalancingState Helper.updateBalancingState()}. Failing to do so may result in - * unnecessary delays of RPCs. Please refer to {@link PickResult#withSubchannel - * PickResult.withSubchannel()}'s javadoc for more information. - * - *

SHUTDOWN can only happen in two cases. One is that LoadBalancer called {@link - * Subchannel#shutdown} earlier, thus it should have already discarded this Subchannel. The - * other is that Channel is doing a {@link ManagedChannel#shutdownNow forced shutdown} or has - * already terminated, thus there won't be further requests to LoadBalancer. Therefore, - * SHUTDOWN can be safely ignored. - * - * @param subchannel the involved Subchannel - * @param newState the new state - * - * @since 1.21.0 - */ - void onSubchannelState(Subchannel subchannel, ConnectivityStateInfo newState); - } - /** * Factory to create {@link LoadBalancer} instance. * diff --git a/api/src/test/java/io/grpc/LoadBalancerTest.java b/api/src/test/java/io/grpc/LoadBalancerTest.java index bd37ae08786..64b2c6e2ab0 100644 --- a/api/src/test/java/io/grpc/LoadBalancerTest.java +++ b/api/src/test/java/io/grpc/LoadBalancerTest.java @@ -17,14 +17,11 @@ package io.grpc; import static com.google.common.truth.Truth.assertThat; -import static org.junit.Assert.fail; import static org.mockito.Mockito.mock; -import io.grpc.LoadBalancer.CreateSubchannelArgs; import io.grpc.LoadBalancer.PickResult; import io.grpc.LoadBalancer.ResolvedAddresses; import io.grpc.LoadBalancer.Subchannel; -import io.grpc.LoadBalancer.SubchannelStateListener; import java.net.SocketAddress; import java.util.Arrays; import java.util.List; @@ -38,8 +35,6 @@ public class LoadBalancerTest { private final Subchannel subchannel = mock(Subchannel.class); private final Subchannel subchannel2 = mock(Subchannel.class); - private final SubchannelStateListener subchannelStateListener = - mock(SubchannelStateListener.class); private final ClientStreamTracer.Factory tracerFactory = mock(ClientStreamTracer.Factory.class); private final Status status = Status.UNAVAILABLE.withDescription("for test"); private final Status status2 = Status.UNAVAILABLE.withDescription("for test 2"); @@ -125,9 +120,8 @@ public void pickResult_equals() { assertThat(error1).isNotEqualTo(drop1); } - @Deprecated @Test - public void helper_createSubchannel_old_delegates() { + public void helper_createSubchannel_delegates() { class OverrideCreateSubchannel extends NoopHelper { boolean ran; @@ -146,29 +140,9 @@ public Subchannel createSubchannel(List addrsIn, Attribu assertThat(helper.ran).isTrue(); } - @Test - @SuppressWarnings("deprecation") - public void helper_createSubchannelList_oldApi_throws() { - try { - new NoopHelper().createSubchannel(Arrays.asList(eag), attrs); - fail("Should throw"); - } catch (UnsupportedOperationException e) { - // expected - } - } - - @Test + @Test(expected = UnsupportedOperationException.class) public void helper_createSubchannelList_throws() { - try { - new NoopHelper().createSubchannel(CreateSubchannelArgs.newBuilder() - .setAddresses(eag) - .setAttributes(attrs) - .setStateListener(subchannelStateListener) - .build()); - fail("Should throw"); - } catch (UnsupportedOperationException e) { - // expected - } + new NoopHelper().createSubchannel(Arrays.asList(eag), attrs); } @Test @@ -225,82 +199,6 @@ public void subchannel_getAddresses_throwsOnTwoAddrs() { }.getAddresses(); } - @Test - public void createSubchannelArgs_option_keyOps() { - CreateSubchannelArgs.Key testKey = CreateSubchannelArgs.Key.create("test-key"); - String testValue = "test-value"; - CreateSubchannelArgs.Key testWithDefaultKey = CreateSubchannelArgs.Key - .createWithDefault("test-key", testValue); - CreateSubchannelArgs args = CreateSubchannelArgs.newBuilder() - .setAddresses(eag) - .setAttributes(attrs) - .setStateListener(subchannelStateListener) - .build(); - assertThat(args.getOption(testKey)).isNull(); - assertThat(args.getOption(testWithDefaultKey)).isSameInstanceAs(testValue); - } - - @Test - public void createSubchannelArgs_option_addGet() { - String testValue = "test-value"; - CreateSubchannelArgs.Key testKey = CreateSubchannelArgs.Key.create("test-key"); - CreateSubchannelArgs args = CreateSubchannelArgs.newBuilder() - .setAddresses(eag) - .setAttributes(attrs) - .setStateListener(subchannelStateListener) - .addOption(testKey, testValue) - .build(); - assertThat(args.getOption(testKey)).isEqualTo(testValue); - } - - @Test - public void createSubchannelArgs_option_lastOneWins() { - String testValue1 = "test-value-1"; - String testValue2 = "test-value-2"; - CreateSubchannelArgs.Key testKey = CreateSubchannelArgs.Key.create("test-key"); - CreateSubchannelArgs args = CreateSubchannelArgs.newBuilder() - .setAddresses(eag) - .setAttributes(attrs) - .setStateListener(subchannelStateListener) - .addOption(testKey, testValue1) - .addOption(testKey, testValue2) - .build(); - assertThat(args.getOption(testKey)).isEqualTo(testValue2); - } - - @Test - public void createSubchannelArgs_build() { - CreateSubchannelArgs.Key testKey = CreateSubchannelArgs.Key.create("test-key"); - Object testValue = new Object(); - CreateSubchannelArgs args = CreateSubchannelArgs.newBuilder() - .setAddresses(eag) - .setAttributes(attrs) - .setStateListener(subchannelStateListener) - .addOption(testKey, testValue) - .build(); - CreateSubchannelArgs rebuildedArgs = args.toBuilder().build(); - assertThat(rebuildedArgs.getAddresses()).containsExactly(eag); - assertThat(rebuildedArgs.getAttributes()).isSameInstanceAs(attrs); - assertThat(rebuildedArgs.getStateListener()).isSameInstanceAs(subchannelStateListener); - assertThat(rebuildedArgs.getOption(testKey)).isSameInstanceAs(testValue); - } - - @Test - public void createSubchannelArgs_toString() { - CreateSubchannelArgs.Key testKey = CreateSubchannelArgs.Key.create("test-key"); - CreateSubchannelArgs args = CreateSubchannelArgs.newBuilder() - .setAddresses(eag) - .setAttributes(attrs) - .setStateListener(subchannelStateListener) - .addOption(testKey, "test-value") - .build(); - String str = args.toString(); - assertThat(str).contains("addrs="); - assertThat(str).contains("attrs="); - assertThat(str).contains("listener="); - assertThat(str).contains("customOptions="); - } - @Deprecated @Test public void handleResolvedAddressGroups_delegatesToHandleResolvedAddresses() { diff --git a/core/src/main/java/io/grpc/internal/AutoConfiguredLoadBalancerFactory.java b/core/src/main/java/io/grpc/internal/AutoConfiguredLoadBalancerFactory.java index afc9302d276..cf006435aed 100644 --- a/core/src/main/java/io/grpc/internal/AutoConfiguredLoadBalancerFactory.java +++ b/core/src/main/java/io/grpc/internal/AutoConfiguredLoadBalancerFactory.java @@ -77,6 +77,9 @@ public void handleResolvedAddresses(ResolvedAddresses resolvedAddresses) {} @Override public void handleNameResolutionError(Status error) {} + @Override + public void handleSubchannelState(Subchannel subchannel, ConnectivityStateInfo stateInfo) {} + @Override public void shutdown() {} } @@ -162,7 +165,6 @@ public void handleNameResolutionError(Status error) { getDelegate().handleNameResolutionError(error); } - @Deprecated @Override public void handleSubchannelState(Subchannel subchannel, ConnectivityStateInfo stateInfo) { getDelegate().handleSubchannelState(subchannel, stateInfo); diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java index 9301856c5d7..5fda8413788 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java @@ -53,7 +53,6 @@ import io.grpc.InternalLogId; import io.grpc.InternalWithLogId; import io.grpc.LoadBalancer; -import io.grpc.LoadBalancer.CreateSubchannelArgs; import io.grpc.LoadBalancer.PickResult; import io.grpc.LoadBalancer.PickSubchannelArgs; import io.grpc.LoadBalancer.ResolvedAddresses; @@ -1043,7 +1042,6 @@ private void handleInternalSubchannelState(ConnectivityStateInfo newState) { } } - @Deprecated @Override public AbstractSubchannel createSubchannel( List addressGroups, Attributes attrs) { @@ -1055,36 +1053,17 @@ public AbstractSubchannel createSubchannel( + " Otherwise, it may race with handleSubchannelState()." + " See https://github.com/grpc/grpc-java/issues/5015", e); } - return createSubchannelInternal( - CreateSubchannelArgs.newBuilder() - .setAddresses(addressGroups) - .setAttributes(attrs) - .setStateListener(new LoadBalancer.SubchannelStateListener() { - @Override - public void onSubchannelState( - LoadBalancer.Subchannel subchannel, ConnectivityStateInfo newState) { - lb.handleSubchannelState(subchannel, newState); - } - }) - .build()); - } - - @Override - public AbstractSubchannel createSubchannel(CreateSubchannelArgs args) { - syncContext.throwIfNotInThisSynchronizationContext(); - return createSubchannelInternal(args); - } - - private AbstractSubchannel createSubchannelInternal(final CreateSubchannelArgs args) { + checkNotNull(addressGroups, "addressGroups"); + checkNotNull(attrs, "attrs"); // TODO(ejona): can we be even stricter? Like loadBalancer == null? checkState(!terminated, "Channel is terminated"); - final SubchannelImpl subchannel = new SubchannelImpl(args.getAttributes()); + final SubchannelImpl subchannel = new SubchannelImpl(attrs); long subchannelCreationTime = timeProvider.currentTimeNanos(); InternalLogId subchannelLogId = InternalLogId.allocate("Subchannel", /*details=*/ null); ChannelTracer subchannelTracer = new ChannelTracer( subchannelLogId, maxTraceEvents, subchannelCreationTime, - "Subchannel for " + args.getAddresses()); + "Subchannel for " + addressGroups); final class ManagedInternalSubchannelCallback extends InternalSubchannel.Callback { // All callbacks are run in syncContext @@ -1100,7 +1079,7 @@ void onStateChange(InternalSubchannel is, ConnectivityStateInfo newState) { handleInternalSubchannelState(newState); // Call LB only if it's not shutdown. If LB is shutdown, lbHelper won't match. if (LbHelperImpl.this == ManagedChannelImpl.this.lbHelper) { - args.getStateListener().onSubchannelState(subchannel, newState); + lb.handleSubchannelState(subchannel, newState); } } @@ -1116,7 +1095,7 @@ void onNotInUse(InternalSubchannel is) { } final InternalSubchannel internalSubchannel = new InternalSubchannel( - args.getAddresses(), + addressGroups, authority(), userAgent, backoffPolicyProvider, diff --git a/core/src/main/java/io/grpc/internal/PickFirstLoadBalancer.java b/core/src/main/java/io/grpc/internal/PickFirstLoadBalancer.java index ba3e9938f24..929c98a5576 100644 --- a/core/src/main/java/io/grpc/internal/PickFirstLoadBalancer.java +++ b/core/src/main/java/io/grpc/internal/PickFirstLoadBalancer.java @@ -21,11 +21,16 @@ import static io.grpc.ConnectivityState.SHUTDOWN; import static io.grpc.ConnectivityState.TRANSIENT_FAILURE; +import io.grpc.Attributes; import io.grpc.ConnectivityState; import io.grpc.ConnectivityStateInfo; import io.grpc.EquivalentAddressGroup; import io.grpc.LoadBalancer; -import io.grpc.LoadBalancer.SubchannelStateListener; +import io.grpc.LoadBalancer.Helper; +import io.grpc.LoadBalancer.PickResult; +import io.grpc.LoadBalancer.PickSubchannelArgs; +import io.grpc.LoadBalancer.Subchannel; +import io.grpc.LoadBalancer.SubchannelPicker; import io.grpc.Status; import java.util.List; @@ -34,7 +39,7 @@ * NameResolver}. The channel's default behavior is used, which is walking down the address list * and sticking to the first that works. */ -final class PickFirstLoadBalancer extends LoadBalancer implements SubchannelStateListener { +final class PickFirstLoadBalancer extends LoadBalancer { private final Helper helper; private Subchannel subchannel; @@ -46,11 +51,7 @@ final class PickFirstLoadBalancer extends LoadBalancer implements SubchannelStat public void handleResolvedAddresses(ResolvedAddresses resolvedAddresses) { List servers = resolvedAddresses.getAddresses(); if (subchannel == null) { - subchannel = helper.createSubchannel( - CreateSubchannelArgs.newBuilder() - .setAddresses(servers) - .setStateListener(this) - .build()); + subchannel = helper.createSubchannel(servers, Attributes.EMPTY); // The channel state does not get updated when doing name resolving today, so for the moment // let LB report CONNECTION and call subchannel.requestConnection() immediately. @@ -73,9 +74,9 @@ public void handleNameResolutionError(Status error) { } @Override - public void onSubchannelState(Subchannel subchannel, ConnectivityStateInfo stateInfo) { + public void handleSubchannelState(Subchannel subchannel, ConnectivityStateInfo stateInfo) { ConnectivityState currentState = stateInfo.getState(); - if (subchannel != PickFirstLoadBalancer.this.subchannel || currentState == SHUTDOWN) { + if (subchannel != this.subchannel || currentState == SHUTDOWN) { return; } @@ -98,6 +99,7 @@ public void onSubchannelState(Subchannel subchannel, ConnectivityStateInfo state default: throw new IllegalArgumentException("Unsupported state:" + currentState); } + helper.updateBalancingState(currentState, picker); } diff --git a/core/src/main/java/io/grpc/util/ForwardingLoadBalancer.java b/core/src/main/java/io/grpc/util/ForwardingLoadBalancer.java index 44ca8a68b76..a5c086fe131 100644 --- a/core/src/main/java/io/grpc/util/ForwardingLoadBalancer.java +++ b/core/src/main/java/io/grpc/util/ForwardingLoadBalancer.java @@ -51,7 +51,6 @@ public void handleNameResolutionError(Status error) { delegate().handleNameResolutionError(error); } - @Deprecated @Override public void handleSubchannelState( Subchannel subchannel, ConnectivityStateInfo stateInfo) { diff --git a/core/src/main/java/io/grpc/util/ForwardingLoadBalancerHelper.java b/core/src/main/java/io/grpc/util/ForwardingLoadBalancerHelper.java index 15a77a007d1..aeac77db007 100644 --- a/core/src/main/java/io/grpc/util/ForwardingLoadBalancerHelper.java +++ b/core/src/main/java/io/grpc/util/ForwardingLoadBalancerHelper.java @@ -22,7 +22,6 @@ import io.grpc.ConnectivityState; import io.grpc.EquivalentAddressGroup; import io.grpc.ExperimentalApi; -import io.grpc.LoadBalancer.CreateSubchannelArgs; import io.grpc.LoadBalancer.Subchannel; import io.grpc.LoadBalancer.SubchannelPicker; import io.grpc.LoadBalancer; @@ -39,17 +38,11 @@ public abstract class ForwardingLoadBalancerHelper extends LoadBalancer.Helper { */ protected abstract LoadBalancer.Helper delegate(); - @Deprecated @Override public Subchannel createSubchannel(List addrs, Attributes attrs) { return delegate().createSubchannel(addrs, attrs); } - @Override - public Subchannel createSubchannel(CreateSubchannelArgs args) { - return delegate().createSubchannel(args); - } - @Override public void updateSubchannelAddresses( Subchannel subchannel, List addrs) { diff --git a/core/src/main/java/io/grpc/util/RoundRobinLoadBalancer.java b/core/src/main/java/io/grpc/util/RoundRobinLoadBalancer.java index 0503bfedd2f..8a021098093 100644 --- a/core/src/main/java/io/grpc/util/RoundRobinLoadBalancer.java +++ b/core/src/main/java/io/grpc/util/RoundRobinLoadBalancer.java @@ -33,7 +33,10 @@ import io.grpc.ConnectivityStateInfo; import io.grpc.EquivalentAddressGroup; import io.grpc.LoadBalancer; -import io.grpc.LoadBalancer.SubchannelStateListener; +import io.grpc.LoadBalancer.PickResult; +import io.grpc.LoadBalancer.PickSubchannelArgs; +import io.grpc.LoadBalancer.Subchannel; +import io.grpc.LoadBalancer.SubchannelPicker; import io.grpc.Metadata; import io.grpc.Metadata.Key; import io.grpc.NameResolver; @@ -60,7 +63,7 @@ * A {@link LoadBalancer} that provides round-robin load-balancing over the {@link * EquivalentAddressGroup}s from the {@link NameResolver}. */ -final class RoundRobinLoadBalancer extends LoadBalancer implements SubchannelStateListener { +final class RoundRobinLoadBalancer extends LoadBalancer { @VisibleForTesting static final Attributes.Key> STATE_INFO = Attributes.Key.create("state-info"); @@ -127,12 +130,7 @@ public void handleResolvedAddresses(ResolvedAddresses resolvedAddresses) { } Subchannel subchannel = checkNotNull( - helper.createSubchannel(CreateSubchannelArgs.newBuilder() - .setAddresses(addressGroup) - .setAttributes(subchannelAttrs.build()) - .setStateListener(this) - .build()), - "subchannel"); + helper.createSubchannel(addressGroup, subchannelAttrs.build()), "subchannel"); if (stickyRef != null) { stickyRef.value = subchannel; } @@ -163,7 +161,7 @@ public void handleNameResolutionError(Status error) { } @Override - public void onSubchannelState(Subchannel subchannel, ConnectivityStateInfo stateInfo) { + public void handleSubchannelState(Subchannel subchannel, ConnectivityStateInfo stateInfo) { if (subchannels.get(subchannel.getAddresses()) != subchannel) { return; } diff --git a/core/src/test/java/io/grpc/internal/AutoConfiguredLoadBalancerFactoryTest.java b/core/src/test/java/io/grpc/internal/AutoConfiguredLoadBalancerFactoryTest.java index 180f83c17a8..1542cf1f7fe 100644 --- a/core/src/test/java/io/grpc/internal/AutoConfiguredLoadBalancerFactoryTest.java +++ b/core/src/test/java/io/grpc/internal/AutoConfiguredLoadBalancerFactoryTest.java @@ -41,12 +41,10 @@ import io.grpc.ConnectivityStateInfo; import io.grpc.EquivalentAddressGroup; import io.grpc.LoadBalancer; -import io.grpc.LoadBalancer.CreateSubchannelArgs; import io.grpc.LoadBalancer.Helper; import io.grpc.LoadBalancer.ResolvedAddresses; import io.grpc.LoadBalancer.Subchannel; import io.grpc.LoadBalancer.SubchannelPicker; -import io.grpc.LoadBalancer.SubchannelStateListener; import io.grpc.LoadBalancerProvider; import io.grpc.LoadBalancerRegistry; import io.grpc.ManagedChannel; @@ -136,7 +134,6 @@ public void defaultIsConfigurable() { assertThat(lb.getDelegate()).isSameInstanceAs(testLbBalancer); } - @SuppressWarnings("deprecation") @Test public void forwardsCalls() { AutoConfiguredLoadBalancer lb = @@ -179,9 +176,9 @@ public void handleResolvedAddressGroups_keepOldBalancer() { Collections.singletonList(new EquivalentAddressGroup(new SocketAddress(){})); Helper helper = new TestHelper() { @Override - public Subchannel createSubchannel(CreateSubchannelArgs args) { - assertThat(args.getAddresses()).isEqualTo(servers); - return new TestSubchannel(args); + public Subchannel createSubchannel(List addrs, Attributes attrs) { + assertThat(addrs).isEqualTo(servers); + return new TestSubchannel(addrs, attrs); } }; AutoConfiguredLoadBalancer lb = @@ -209,9 +206,9 @@ public void handleResolvedAddressGroups_shutsDownOldBalancer() { Collections.singletonList(new EquivalentAddressGroup(new SocketAddress(){})); Helper helper = new TestHelper() { @Override - public Subchannel createSubchannel(CreateSubchannelArgs args) { - assertThat(args.getAddresses()).isEqualTo(servers); - return new TestSubchannel(args); + public Subchannel createSubchannel(List addrs, Attributes attrs) { + assertThat(addrs).isEqualTo(servers); + return new TestSubchannel(addrs, attrs); } }; AutoConfiguredLoadBalancer lb = @@ -224,6 +221,11 @@ public void handleNameResolutionError(Status error) { // noop } + @Override + public void handleSubchannelState(Subchannel subchannel, ConnectivityStateInfo stateInfo) { + // noop + } + @Override public void shutdown() { shutdown.set(true); @@ -702,18 +704,8 @@ public void channelTracing_lbPolicyChanged() { Collections.singletonList(new EquivalentAddressGroup(new SocketAddress(){})); Helper helper = new TestHelper() { @Override - @Deprecated public Subchannel createSubchannel(List addrs, Attributes attrs) { - return new TestSubchannel(CreateSubchannelArgs.newBuilder() - .setAddresses(addrs) - .setAttributes(attrs) - .setStateListener(mock(SubchannelStateListener.class)) - .build()); - } - - @Override - public Subchannel createSubchannel(CreateSubchannelArgs args) { - return new TestSubchannel(args); + return new TestSubchannel(addrs, attrs); } @Override @@ -830,6 +822,11 @@ public void handleNameResolutionError(Status error) { delegate().handleNameResolutionError(error); } + @Override + public void handleSubchannelState(Subchannel subchannel, ConnectivityStateInfo stateInfo) { + delegate().handleSubchannelState(subchannel, stateInfo); + } + @Override public void shutdown() { delegate().shutdown(); @@ -865,9 +862,9 @@ public void updateBalancingState(ConnectivityState newState, SubchannelPicker ne } private static class TestSubchannel extends Subchannel { - TestSubchannel(CreateSubchannelArgs args) { - this.addrs = args.getAddresses(); - this.attrs = args.getAttributes(); + TestSubchannel(List addrs, Attributes attrs) { + this.addrs = addrs; + this.attrs = attrs; } final List addrs; diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplIdlenessTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplIdlenessTest.java index 84feec6fce7..43cc98ffcf0 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplIdlenessTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplIdlenessTest.java @@ -41,14 +41,12 @@ import io.grpc.EquivalentAddressGroup; import io.grpc.IntegerMarshaller; import io.grpc.LoadBalancer; -import io.grpc.LoadBalancer.CreateSubchannelArgs; import io.grpc.LoadBalancer.Helper; import io.grpc.LoadBalancer.PickResult; import io.grpc.LoadBalancer.PickSubchannelArgs; import io.grpc.LoadBalancer.ResolvedAddresses; import io.grpc.LoadBalancer.Subchannel; import io.grpc.LoadBalancer.SubchannelPicker; -import io.grpc.LoadBalancer.SubchannelStateListener; import io.grpc.LoadBalancerProvider; import io.grpc.LoadBalancerRegistry; import io.grpc.ManagedChannel; @@ -115,7 +113,6 @@ public class ManagedChannelImplIdlenessTest { @Mock private ClientTransportFactory mockTransportFactory; @Mock private LoadBalancer mockLoadBalancer; - @Mock private SubchannelStateListener subchannelStateListener; private final LoadBalancerProvider mockLoadBalancerProvider = mock(LoadBalancerProvider.class, delegatesTo(new LoadBalancerProvider() { @Override @@ -503,19 +500,14 @@ public String toString() { } // We need this because createSubchannel() should be called from the SynchronizationContext - private Subchannel createSubchannelSafely( + private static Subchannel createSubchannelSafely( final Helper helper, final EquivalentAddressGroup addressGroup, final Attributes attrs) { final AtomicReference resultCapture = new AtomicReference<>(); helper.getSynchronizationContext().execute( new Runnable() { @Override public void run() { - resultCapture.set( - helper.createSubchannel(CreateSubchannelArgs.newBuilder() - .setAddresses(addressGroup) - .setAttributes(attrs) - .setStateListener(subchannelStateListener) - .build())); + resultCapture.set(helper.createSubchannel(addressGroup, attrs)); } }); return resultCapture.get(); diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java index e21328c6544..94108ec7de1 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java @@ -78,14 +78,12 @@ import io.grpc.InternalChannelz.ChannelTrace; import io.grpc.InternalInstrumented; import io.grpc.LoadBalancer; -import io.grpc.LoadBalancer.CreateSubchannelArgs; import io.grpc.LoadBalancer.Helper; import io.grpc.LoadBalancer.PickResult; import io.grpc.LoadBalancer.PickSubchannelArgs; import io.grpc.LoadBalancer.ResolvedAddresses; import io.grpc.LoadBalancer.Subchannel; import io.grpc.LoadBalancer.SubchannelPicker; -import io.grpc.LoadBalancer.SubchannelStateListener; import io.grpc.LoadBalancerProvider; import io.grpc.LoadBalancerRegistry; import io.grpc.ManagedChannel; @@ -209,8 +207,6 @@ public boolean shouldAccept(Runnable command) { private ArgumentCaptor callOptionsCaptor; @Mock private LoadBalancer mockLoadBalancer; - @Mock - private SubchannelStateListener subchannelStateListener; private final LoadBalancerProvider mockLoadBalancerProvider = mock(LoadBalancerProvider.class, delegatesTo(new LoadBalancerProvider() { @Override @@ -338,9 +334,8 @@ public void cleanUp() { LoadBalancerRegistry.getDefaultRegistry().deregister(mockLoadBalancerProvider); } - @Deprecated @Test - public void createSubchannel_old_outsideSynchronizationContextShouldLogWarning() { + public void createSubchannelOutsideSynchronizationContextShouldLogWarning() { createChannel(); final AtomicReference logRef = new AtomicReference<>(); Handler handler = new Handler() { @@ -371,48 +366,6 @@ public void close() throws SecurityException { } } - @Deprecated - @Test - public void createSubchannel_old_propagateSubchannelStatesToOldApi() { - createChannel(); - final AtomicReference subchannelCapture = new AtomicReference<>(); - helper.getSynchronizationContext().execute(new Runnable() { - @Override - public void run() { - subchannelCapture.set(helper.createSubchannel(addressGroup, Attributes.EMPTY)); - } - }); - - Subchannel subchannel = subchannelCapture.get(); - subchannel.requestConnection(); - - verify(mockTransportFactory) - .newClientTransport( - any(SocketAddress.class), any(ClientTransportOptions.class), any(ChannelLogger.class)); - verify(mockLoadBalancer).handleSubchannelState( - same(subchannel), eq(ConnectivityStateInfo.forNonError(CONNECTING))); - - MockClientTransportInfo transportInfo = transports.poll(); - transportInfo.listener.transportReady(); - - verify(mockLoadBalancer).handleSubchannelState( - same(subchannel), eq(ConnectivityStateInfo.forNonError(READY))); - } - - @Test - public void createSubchannel_outsideSynchronizationContextShouldThrow() { - createChannel(); - try { - helper.createSubchannel(CreateSubchannelArgs.newBuilder() - .setAddresses(addressGroup) - .setStateListener(subchannelStateListener) - .build()); - fail("Should throw"); - } catch (IllegalStateException e) { - assertThat(e).hasMessageThat().isEqualTo("Not called from the SynchronizationContext"); - } - } - @Test @SuppressWarnings("unchecked") public void idleModeDisabled() { @@ -473,8 +426,7 @@ public void channelzMembership_subchannel() throws Exception { assertNotNull(channelz.getRootChannel(channel.getLogId().getId())); AbstractSubchannel subchannel = - (AbstractSubchannel) createSubchannelSafely( - helper, addressGroup, Attributes.EMPTY, subchannelStateListener); + (AbstractSubchannel) createSubchannelSafely(helper, addressGroup, Attributes.EMPTY); // subchannels are not root channels assertNull(channelz.getRootChannel(subchannel.getInternalSubchannel().getLogId().getId())); assertTrue(channelz.containsSubchannel(subchannel.getInternalSubchannel().getLogId())); @@ -566,8 +518,7 @@ private void subtestCallsAndShutdown(boolean shutdownNow, boolean shutdownNowAft // Configure the picker so that first RPC goes to delayed transport, and second RPC goes to // real transport. - Subchannel subchannel = - createSubchannelSafely(helper, addressGroup, Attributes.EMPTY, subchannelStateListener); + Subchannel subchannel = createSubchannelSafely(helper, addressGroup, Attributes.EMPTY); subchannel.requestConnection(); verify(mockTransportFactory) .newClientTransport( @@ -706,12 +657,8 @@ public void noMoreCallbackAfterLoadBalancerShutdown() { .setAttributes(Attributes.EMPTY) .build()); - SubchannelStateListener stateListener1 = mock(SubchannelStateListener.class); - SubchannelStateListener stateListener2 = mock(SubchannelStateListener.class); - Subchannel subchannel1 = - createSubchannelSafely(helper, addressGroup, Attributes.EMPTY, stateListener1); - Subchannel subchannel2 = - createSubchannelSafely(helper, addressGroup, Attributes.EMPTY, stateListener2); + Subchannel subchannel1 = createSubchannelSafely(helper, addressGroup, Attributes.EMPTY); + Subchannel subchannel2 = createSubchannelSafely(helper, addressGroup, Attributes.EMPTY); subchannel1.requestConnection(); subchannel2.requestConnection(); verify(mockTransportFactory, times(2)) @@ -722,14 +669,13 @@ public void noMoreCallbackAfterLoadBalancerShutdown() { // LoadBalancer receives all sorts of callbacks transportInfo1.listener.transportReady(); - - verify(stateListener1, times(2)) - .onSubchannelState(same(subchannel1), stateInfoCaptor.capture()); + verify(mockLoadBalancer, times(2)) + .handleSubchannelState(same(subchannel1), stateInfoCaptor.capture()); assertSame(CONNECTING, stateInfoCaptor.getAllValues().get(0).getState()); assertSame(READY, stateInfoCaptor.getAllValues().get(1).getState()); - verify(stateListener2) - .onSubchannelState(same(subchannel2), stateInfoCaptor.capture()); + verify(mockLoadBalancer) + .handleSubchannelState(same(subchannel2), stateInfoCaptor.capture()); assertSame(CONNECTING, stateInfoCaptor.getValue().getState()); resolver.observer.onError(resolutionError); @@ -778,8 +724,7 @@ public void callOptionsExecutor() { call.start(mockCallListener, headers); // Make the transport available - Subchannel subchannel = - createSubchannelSafely(helper, addressGroup, Attributes.EMPTY, subchannelStateListener); + Subchannel subchannel = createSubchannelSafely(helper, addressGroup, Attributes.EMPTY); verify(mockTransportFactory, never()) .newClientTransport( any(SocketAddress.class), any(ClientTransportOptions.class), any(ChannelLogger.class)); @@ -1016,7 +961,7 @@ public void firstResolvedServerFailedToConnect() throws Exception { return "badAddress"; } }; - InOrder inOrder = inOrder(mockLoadBalancer, subchannelStateListener); + InOrder inOrder = inOrder(mockLoadBalancer); List resolvedAddrs = Arrays.asList(badAddress, goodAddress); FakeNameResolverFactory nameResolverFactory = @@ -1038,13 +983,12 @@ public void firstResolvedServerFailedToConnect() throws Exception { ResolvedAddresses.newBuilder() .setAddresses(Arrays.asList(addressGroup)) .build()); - Subchannel subchannel = - createSubchannelSafely(helper, addressGroup, Attributes.EMPTY, subchannelStateListener); + Subchannel subchannel = createSubchannelSafely(helper, addressGroup, Attributes.EMPTY); when(mockPicker.pickSubchannel(any(PickSubchannelArgs.class))) .thenReturn(PickResult.withSubchannel(subchannel)); subchannel.requestConnection(); - inOrder.verify(subchannelStateListener) - .onSubchannelState(same(subchannel), stateInfoCaptor.capture()); + inOrder.verify(mockLoadBalancer).handleSubchannelState( + same(subchannel), stateInfoCaptor.capture()); assertEquals(CONNECTING, stateInfoCaptor.getValue().getState()); // The channel will starts with the first address (badAddress) @@ -1070,8 +1014,8 @@ public void firstResolvedServerFailedToConnect() throws Exception { .thenReturn(mock(ClientStream.class)); goodTransportInfo.listener.transportReady(); - inOrder.verify(subchannelStateListener) - .onSubchannelState(same(subchannel), stateInfoCaptor.capture()); + inOrder.verify(mockLoadBalancer).handleSubchannelState( + same(subchannel), stateInfoCaptor.capture()); assertEquals(READY, stateInfoCaptor.getValue().getState()); // A typical LoadBalancer will call this once the subchannel becomes READY @@ -1161,7 +1105,7 @@ public void allServersFailedToConnect() throws Exception { return "addr2"; } }; - InOrder inOrder = inOrder(mockLoadBalancer, subchannelStateListener); + InOrder inOrder = inOrder(mockLoadBalancer); List resolvedAddrs = Arrays.asList(addr1, addr2); @@ -1189,14 +1133,13 @@ public void allServersFailedToConnect() throws Exception { ResolvedAddresses.newBuilder() .setAddresses(Arrays.asList(addressGroup)) .build()); - Subchannel subchannel = - createSubchannelSafely(helper, addressGroup, Attributes.EMPTY, subchannelStateListener); + Subchannel subchannel = createSubchannelSafely(helper, addressGroup, Attributes.EMPTY); when(mockPicker.pickSubchannel(any(PickSubchannelArgs.class))) .thenReturn(PickResult.withSubchannel(subchannel)); subchannel.requestConnection(); - inOrder.verify(subchannelStateListener) - .onSubchannelState(same(subchannel), stateInfoCaptor.capture()); + inOrder.verify(mockLoadBalancer).handleSubchannelState( + same(subchannel), stateInfoCaptor.capture()); assertEquals(CONNECTING, stateInfoCaptor.getValue().getState()); // Connecting to server1, which will fail @@ -1219,8 +1162,8 @@ public void allServersFailedToConnect() throws Exception { // ... which makes the subchannel enter TRANSIENT_FAILURE. The last error Status is propagated // to LoadBalancer. - inOrder.verify(subchannelStateListener) - .onSubchannelState(same(subchannel), stateInfoCaptor.capture()); + inOrder.verify(mockLoadBalancer).handleSubchannelState( + same(subchannel), stateInfoCaptor.capture()); assertEquals(TRANSIENT_FAILURE, stateInfoCaptor.getValue().getState()); assertSame(server2Error, stateInfoCaptor.getValue().getStatus()); @@ -1249,10 +1192,8 @@ public void subchannels() { // createSubchannel() always return a new Subchannel Attributes attrs1 = Attributes.newBuilder().set(SUBCHANNEL_ATTR_KEY, "attr1").build(); Attributes attrs2 = Attributes.newBuilder().set(SUBCHANNEL_ATTR_KEY, "attr2").build(); - SubchannelStateListener listener1 = mock(SubchannelStateListener.class); - SubchannelStateListener listener2 = mock(SubchannelStateListener.class); - Subchannel sub1 = createSubchannelSafely(helper, addressGroup, attrs1, listener1); - Subchannel sub2 = createSubchannelSafely(helper, addressGroup, attrs2, listener2); + Subchannel sub1 = createSubchannelSafely(helper, addressGroup, attrs1); + Subchannel sub2 = createSubchannelSafely(helper, addressGroup, attrs2); assertNotSame(sub1, sub2); assertNotSame(attrs1, attrs2); assertSame(attrs1, sub1.getAttributes()); @@ -1319,10 +1260,8 @@ public void subchannels() { @Test public void subchannelsWhenChannelShutdownNow() { createChannel(); - Subchannel sub1 = - createSubchannelSafely(helper, addressGroup, Attributes.EMPTY, subchannelStateListener); - Subchannel sub2 = - createSubchannelSafely(helper, addressGroup, Attributes.EMPTY, subchannelStateListener); + Subchannel sub1 = createSubchannelSafely(helper, addressGroup, Attributes.EMPTY); + Subchannel sub2 = createSubchannelSafely(helper, addressGroup, Attributes.EMPTY); sub1.requestConnection(); sub2.requestConnection(); @@ -1349,10 +1288,8 @@ public void subchannelsWhenChannelShutdownNow() { @Test public void subchannelsNoConnectionShutdown() { createChannel(); - Subchannel sub1 = - createSubchannelSafely(helper, addressGroup, Attributes.EMPTY, subchannelStateListener); - Subchannel sub2 = - createSubchannelSafely(helper, addressGroup, Attributes.EMPTY, subchannelStateListener); + Subchannel sub1 = createSubchannelSafely(helper, addressGroup, Attributes.EMPTY); + Subchannel sub2 = createSubchannelSafely(helper, addressGroup, Attributes.EMPTY); channel.shutdown(); verify(mockLoadBalancer).shutdown(); @@ -1368,8 +1305,8 @@ public void subchannelsNoConnectionShutdown() { @Test public void subchannelsNoConnectionShutdownNow() { createChannel(); - createSubchannelSafely(helper, addressGroup, Attributes.EMPTY, subchannelStateListener); - createSubchannelSafely(helper, addressGroup, Attributes.EMPTY, subchannelStateListener); + createSubchannelSafely(helper, addressGroup, Attributes.EMPTY); + createSubchannelSafely(helper, addressGroup, Attributes.EMPTY); channel.shutdownNow(); verify(mockLoadBalancer).shutdown(); @@ -1551,8 +1488,7 @@ public void oobChannelsNoConnectionShutdownNow() { @Test public void subchannelChannel_normalUsage() { createChannel(); - Subchannel subchannel = - createSubchannelSafely(helper, addressGroup, Attributes.EMPTY, subchannelStateListener); + Subchannel subchannel = createSubchannelSafely(helper, addressGroup, Attributes.EMPTY); verify(balancerRpcExecutorPool, never()).getObject(); Channel sChannel = subchannel.asChannel(); @@ -1583,8 +1519,7 @@ public void subchannelChannel_normalUsage() { @Test public void subchannelChannel_failWhenNotReady() { createChannel(); - Subchannel subchannel = - createSubchannelSafely(helper, addressGroup, Attributes.EMPTY, subchannelStateListener); + Subchannel subchannel = createSubchannelSafely(helper, addressGroup, Attributes.EMPTY); Channel sChannel = subchannel.asChannel(); Metadata headers = new Metadata(); @@ -1612,8 +1547,7 @@ public void subchannelChannel_failWhenNotReady() { @Test public void subchannelChannel_failWaitForReady() { createChannel(); - Subchannel subchannel = - createSubchannelSafely(helper, addressGroup, Attributes.EMPTY, subchannelStateListener); + Subchannel subchannel = createSubchannelSafely(helper, addressGroup, Attributes.EMPTY); Channel sChannel = subchannel.asChannel(); Metadata headers = new Metadata(); @@ -1700,8 +1634,7 @@ private void subtestNameResolutionRefreshWhenConnectionFailed( OobChannel oobChannel = (OobChannel) helper.createOobChannel(addressGroup, "oobAuthority"); oobChannel.getSubchannel().requestConnection(); } else { - Subchannel subchannel = - createSubchannelSafely(helper, addressGroup, Attributes.EMPTY, subchannelStateListener); + Subchannel subchannel = createSubchannelSafely(helper, addressGroup, Attributes.EMPTY); subchannel.requestConnection(); } @@ -1788,8 +1721,7 @@ public Void answer(InvocationOnMock in) throws Throwable { // Simulate name resolution results EquivalentAddressGroup addressGroup = new EquivalentAddressGroup(socketAddress); - Subchannel subchannel = - createSubchannelSafely(helper, addressGroup, Attributes.EMPTY, subchannelStateListener); + Subchannel subchannel = createSubchannelSafely(helper, addressGroup, Attributes.EMPTY); subchannel.requestConnection(); verify(mockTransportFactory) .newClientTransport( @@ -1862,8 +1794,7 @@ public void pickerReturnsStreamTracer_noDelay() { ClientStreamTracer.Factory factory1 = mock(ClientStreamTracer.Factory.class); ClientStreamTracer.Factory factory2 = mock(ClientStreamTracer.Factory.class); createChannel(); - Subchannel subchannel = - createSubchannelSafely(helper, addressGroup, Attributes.EMPTY, subchannelStateListener); + Subchannel subchannel = createSubchannelSafely(helper, addressGroup, Attributes.EMPTY); subchannel.requestConnection(); MockClientTransportInfo transportInfo = transports.poll(); transportInfo.listener.transportReady(); @@ -1901,8 +1832,7 @@ public void pickerReturnsStreamTracer_delayed() { ClientCall call = channel.newCall(method, callOptions); call.start(mockCallListener, new Metadata()); - Subchannel subchannel = - createSubchannelSafely(helper, addressGroup, Attributes.EMPTY, subchannelStateListener); + Subchannel subchannel = createSubchannelSafely(helper, addressGroup, Attributes.EMPTY); subchannel.requestConnection(); MockClientTransportInfo transportInfo = transports.poll(); transportInfo.listener.transportReady(); @@ -2277,8 +2207,7 @@ public void idleMode_resetsDelayedTransportPicker() { Helper helper2 = helperCaptor.getValue(); // Establish a connection - Subchannel subchannel = - createSubchannelSafely(helper2, addressGroup, Attributes.EMPTY, subchannelStateListener); + Subchannel subchannel = createSubchannelSafely(helper2, addressGroup, Attributes.EMPTY); subchannel.requestConnection(); MockClientTransportInfo transportInfo = transports.poll(); ConnectionClientTransport mockTransport = transportInfo.transport; @@ -2346,8 +2275,7 @@ public void enterIdle_exitsIdleIfDelayedStreamPending() { Helper helper2 = helperCaptor.getValue(); // Establish a connection - Subchannel subchannel = - createSubchannelSafely(helper2, addressGroup, Attributes.EMPTY, subchannelStateListener); + Subchannel subchannel = createSubchannelSafely(helper2, addressGroup, Attributes.EMPTY); subchannel.requestConnection(); ClientStream mockStream = mock(ClientStream.class); MockClientTransportInfo transportInfo = transports.poll(); @@ -2376,10 +2304,8 @@ public void updateBalancingStateDoesUpdatePicker() { call.start(mockCallListener, new Metadata()); // Make the transport available with subchannel2 - Subchannel subchannel1 = - createSubchannelSafely(helper, addressGroup, Attributes.EMPTY, subchannelStateListener); - Subchannel subchannel2 = - createSubchannelSafely(helper, addressGroup, Attributes.EMPTY, subchannelStateListener); + Subchannel subchannel1 = createSubchannelSafely(helper, addressGroup, Attributes.EMPTY); + Subchannel subchannel2 = createSubchannelSafely(helper, addressGroup, Attributes.EMPTY); subchannel2.requestConnection(); MockClientTransportInfo transportInfo = transports.poll(); @@ -2518,8 +2444,7 @@ public void channelsAndSubchannels_instrumented_name() throws Exception { createChannel(); assertEquals(TARGET, getStats(channel).target); - Subchannel subchannel = - createSubchannelSafely(helper, addressGroup, Attributes.EMPTY, subchannelStateListener); + Subchannel subchannel = createSubchannelSafely(helper, addressGroup, Attributes.EMPTY); assertEquals(Collections.singletonList(addressGroup).toString(), getStats((AbstractSubchannel) subchannel).target); } @@ -2542,8 +2467,7 @@ public void channelTracing_subchannelCreationEvents() throws Exception { createChannel(); timer.forwardNanos(1234); AbstractSubchannel subchannel = - (AbstractSubchannel) createSubchannelSafely( - helper, addressGroup, Attributes.EMPTY, subchannelStateListener); + (AbstractSubchannel) createSubchannelSafely(helper, addressGroup, Attributes.EMPTY); assertThat(getStats(channel).channelTrace.events).contains(new ChannelTrace.Event.Builder() .setDescription("Child Subchannel created") .setSeverity(ChannelTrace.Event.Severity.CT_INFO) @@ -2716,8 +2640,7 @@ public void channelTracing_subchannelStateChangeEvent() throws Exception { channelBuilder.maxTraceEvents(10); createChannel(); AbstractSubchannel subchannel = - (AbstractSubchannel) createSubchannelSafely( - helper, addressGroup, Attributes.EMPTY, subchannelStateListener); + (AbstractSubchannel) createSubchannelSafely(helper, addressGroup, Attributes.EMPTY); timer.forwardNanos(1234); subchannel.obtainActiveTransport(); assertThat(getStats(subchannel).channelTrace.events).contains(new ChannelTrace.Event.Builder() @@ -2780,8 +2703,7 @@ public void channelsAndSubchannels_instrumented_state() throws Exception { assertEquals(CONNECTING, getStats(channel).state); AbstractSubchannel subchannel = - (AbstractSubchannel) createSubchannelSafely( - helper, addressGroup, Attributes.EMPTY, subchannelStateListener); + (AbstractSubchannel) createSubchannelSafely(helper, addressGroup, Attributes.EMPTY); assertEquals(IDLE, getStats(subchannel).state); subchannel.requestConnection(); @@ -2835,8 +2757,7 @@ private void channelsAndSubchannels_instrumented0(boolean success) throws Except ClientStream mockStream = mock(ClientStream.class); ClientStreamTracer.Factory factory = mock(ClientStreamTracer.Factory.class); AbstractSubchannel subchannel = - (AbstractSubchannel) createSubchannelSafely( - helper, addressGroup, Attributes.EMPTY, subchannelStateListener); + (AbstractSubchannel) createSubchannelSafely(helper, addressGroup, Attributes.EMPTY); subchannel.requestConnection(); MockClientTransportInfo transportInfo = transports.poll(); transportInfo.listener.transportReady(); @@ -3073,8 +2994,7 @@ public double nextDouble() { .build()); // simulating request connection and then transport ready after resolved address - Subchannel subchannel = - createSubchannelSafely(helper, addressGroup, Attributes.EMPTY, subchannelStateListener); + Subchannel subchannel = createSubchannelSafely(helper, addressGroup, Attributes.EMPTY); when(mockPicker.pickSubchannel(any(PickSubchannelArgs.class))) .thenReturn(PickResult.withSubchannel(subchannel)); subchannel.requestConnection(); @@ -3173,8 +3093,7 @@ public void hedgingScheduledThenChannelShutdown_hedgeShouldStillHappen_newCallSh .build()); // simulating request connection and then transport ready after resolved address - Subchannel subchannel = - createSubchannelSafely(helper, addressGroup, Attributes.EMPTY, subchannelStateListener); + Subchannel subchannel = helper.createSubchannel(addressGroup, Attributes.EMPTY); when(mockPicker.pickSubchannel(any(PickSubchannelArgs.class))) .thenReturn(PickResult.withSubchannel(subchannel)); subchannel.requestConnection(); @@ -3961,18 +3880,13 @@ private FakeClock.ScheduledTask getNameResolverRefresh() { // We need this because createSubchannel() should be called from the SynchronizationContext private static Subchannel createSubchannelSafely( - final Helper helper, final EquivalentAddressGroup addressGroup, final Attributes attrs, - final SubchannelStateListener stateListener) { + final Helper helper, final EquivalentAddressGroup addressGroup, final Attributes attrs) { final AtomicReference resultCapture = new AtomicReference<>(); helper.getSynchronizationContext().execute( new Runnable() { @Override public void run() { - resultCapture.set(helper.createSubchannel(CreateSubchannelArgs.newBuilder() - .setAddresses(addressGroup) - .setAttributes(attrs) - .setStateListener(stateListener) - .build())); + resultCapture.set(helper.createSubchannel(addressGroup, attrs)); } }); return resultCapture.get(); diff --git a/core/src/test/java/io/grpc/internal/PickFirstLoadBalancerTest.java b/core/src/test/java/io/grpc/internal/PickFirstLoadBalancerTest.java index 4ae2b08b2b8..c853cdb11f4 100644 --- a/core/src/test/java/io/grpc/internal/PickFirstLoadBalancerTest.java +++ b/core/src/test/java/io/grpc/internal/PickFirstLoadBalancerTest.java @@ -16,7 +16,6 @@ package io.grpc.internal; -import static com.google.common.truth.Truth.assertThat; import static io.grpc.ConnectivityState.CONNECTING; import static io.grpc.ConnectivityState.IDLE; import static io.grpc.ConnectivityState.READY; @@ -25,8 +24,10 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.ArgumentMatchers.isA; +import static org.mockito.Mockito.any; import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.never; +import static org.mockito.Mockito.reset; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; @@ -37,14 +38,12 @@ import io.grpc.ConnectivityState; import io.grpc.ConnectivityStateInfo; import io.grpc.EquivalentAddressGroup; -import io.grpc.LoadBalancer.CreateSubchannelArgs; import io.grpc.LoadBalancer.Helper; import io.grpc.LoadBalancer.PickResult; import io.grpc.LoadBalancer.PickSubchannelArgs; import io.grpc.LoadBalancer.ResolvedAddresses; import io.grpc.LoadBalancer.Subchannel; import io.grpc.LoadBalancer.SubchannelPicker; -import io.grpc.LoadBalancer.SubchannelStateListener; import io.grpc.Status; import java.net.SocketAddress; import java.util.List; @@ -77,7 +76,7 @@ public class PickFirstLoadBalancerTest { @Captor private ArgumentCaptor pickerCaptor; @Captor - private ArgumentCaptor createArgsCaptor; + private ArgumentCaptor attrsCaptor; @Mock private Helper mockHelper; @Mock @@ -94,17 +93,16 @@ public void setUp() { } when(mockSubchannel.getAllAddresses()).thenThrow(new UnsupportedOperationException()); - when(mockHelper.createSubchannel(any(CreateSubchannelArgs.class))).thenReturn(mockSubchannel); + when(mockHelper.createSubchannel( + ArgumentMatchers.anyList(), any(Attributes.class))) + .thenReturn(mockSubchannel); loadBalancer = new PickFirstLoadBalancer(mockHelper); } @After - @SuppressWarnings("deprecation") public void tearDown() throws Exception { verifyNoMoreInteractions(mockArgs); - verify(mockHelper, never()).createSubchannel( - ArgumentMatchers.anyList(), any(Attributes.class)); } @Test @@ -112,9 +110,7 @@ public void pickAfterResolved() throws Exception { loadBalancer.handleResolvedAddresses( ResolvedAddresses.newBuilder().setAddresses(servers).setAttributes(affinity).build()); - verify(mockHelper).createSubchannel(createArgsCaptor.capture()); - CreateSubchannelArgs args = createArgsCaptor.getValue(); - assertThat(args.getAddresses()).isEqualTo(servers); + verify(mockHelper).createSubchannel(eq(servers), attrsCaptor.capture()); verify(mockHelper).updateBalancingState(eq(CONNECTING), pickerCaptor.capture()); verify(mockSubchannel).requestConnection(); @@ -133,8 +129,8 @@ public void pickAfterResolvedAndUnchanged() throws Exception { ResolvedAddresses.newBuilder().setAddresses(servers).setAttributes(affinity).build()); verifyNoMoreInteractions(mockSubchannel); - verify(mockHelper).createSubchannel(createArgsCaptor.capture()); - assertThat(createArgsCaptor.getValue()).isNotNull(); + verify(mockHelper).createSubchannel(ArgumentMatchers.anyList(), + any(Attributes.class)); verify(mockHelper) .updateBalancingState(isA(ConnectivityState.class), isA(SubchannelPicker.class)); // Updating the subchannel addresses is unnecessary, but doesn't hurt anything @@ -154,9 +150,7 @@ public void pickAfterResolvedAndChanged() throws Exception { loadBalancer.handleResolvedAddresses( ResolvedAddresses.newBuilder().setAddresses(servers).setAttributes(affinity).build()); - inOrder.verify(mockHelper).createSubchannel(createArgsCaptor.capture()); - CreateSubchannelArgs args = createArgsCaptor.getValue(); - assertThat(args.getAddresses()).isEqualTo(servers); + inOrder.verify(mockHelper).createSubchannel(eq(servers), any(Attributes.class)); inOrder.verify(mockHelper).updateBalancingState(eq(CONNECTING), pickerCaptor.capture()); verify(mockSubchannel).requestConnection(); assertEquals(mockSubchannel, pickerCaptor.getValue().pickSubchannel(mockArgs).getSubchannel()); @@ -170,29 +164,33 @@ public void pickAfterResolvedAndChanged() throws Exception { } @Test - public void pickAfterStateChangeAfterResolution() throws Exception { - InOrder inOrder = inOrder(mockHelper); + public void stateChangeBeforeResolution() throws Exception { + loadBalancer.handleSubchannelState(mockSubchannel, ConnectivityStateInfo.forNonError(READY)); + + verifyNoMoreInteractions(mockHelper); + } + @Test + public void pickAfterStateChangeAfterResolution() throws Exception { loadBalancer.handleResolvedAddresses( ResolvedAddresses.newBuilder().setAddresses(servers).setAttributes(affinity).build()); - inOrder.verify(mockHelper).createSubchannel(createArgsCaptor.capture()); - CreateSubchannelArgs args = createArgsCaptor.getValue(); - assertThat(args.getAddresses()).isEqualTo(servers); - SubchannelStateListener stateListener = args.getStateListener(); verify(mockHelper).updateBalancingState(eq(CONNECTING), pickerCaptor.capture()); Subchannel subchannel = pickerCaptor.getValue().pickSubchannel(mockArgs).getSubchannel(); + reset(mockHelper); + + InOrder inOrder = inOrder(mockHelper); Status error = Status.UNAVAILABLE.withDescription("boom!"); - stateListener.onSubchannelState(subchannel, + loadBalancer.handleSubchannelState(subchannel, ConnectivityStateInfo.forTransientFailure(error)); inOrder.verify(mockHelper).updateBalancingState(eq(TRANSIENT_FAILURE), pickerCaptor.capture()); assertEquals(error, pickerCaptor.getValue().pickSubchannel(mockArgs).getStatus()); - stateListener.onSubchannelState(subchannel, ConnectivityStateInfo.forNonError(IDLE)); + loadBalancer.handleSubchannelState(subchannel, ConnectivityStateInfo.forNonError(IDLE)); inOrder.verify(mockHelper).updateBalancingState(eq(IDLE), pickerCaptor.capture()); assertEquals(Status.OK, pickerCaptor.getValue().pickSubchannel(mockArgs).getStatus()); - stateListener.onSubchannelState(subchannel, ConnectivityStateInfo.forNonError(READY)); + loadBalancer.handleSubchannelState(subchannel, ConnectivityStateInfo.forNonError(READY)); inOrder.verify(mockHelper).updateBalancingState(eq(READY), pickerCaptor.capture()); assertEquals(subchannel, pickerCaptor.getValue().pickSubchannel(mockArgs).getSubchannel()); @@ -222,10 +220,8 @@ public void nameResolutionSuccessAfterError() throws Exception { loadBalancer.handleResolvedAddresses( ResolvedAddresses.newBuilder().setAddresses(servers).setAttributes(affinity).build()); - inOrder.verify(mockHelper).createSubchannel(createArgsCaptor.capture()); - CreateSubchannelArgs args = createArgsCaptor.getValue(); - assertThat(args.getAddresses()).isEqualTo(servers); - assertThat(args.getAttributes()).isEqualTo(Attributes.EMPTY); + + inOrder.verify(mockHelper).createSubchannel(eq(servers), eq(Attributes.EMPTY)); inOrder.verify(mockHelper).updateBalancingState(eq(CONNECTING), pickerCaptor.capture()); verify(mockSubchannel).requestConnection(); @@ -241,21 +237,9 @@ public void nameResolutionSuccessAfterError() throws Exception { @Test public void nameResolutionErrorWithStateChanges() throws Exception { InOrder inOrder = inOrder(mockHelper); - loadBalancer.handleResolvedAddresses( - ResolvedAddresses.newBuilder().setAddresses(servers).setAttributes(affinity).build()); - inOrder.verify(mockHelper).createSubchannel(createArgsCaptor.capture()); - CreateSubchannelArgs args = createArgsCaptor.getValue(); - assertThat(args.getAddresses()).isEqualTo(servers); - inOrder.verify(mockHelper).updateBalancingState(eq(CONNECTING), any(SubchannelPicker.class)); - - SubchannelStateListener stateListener = args.getStateListener(); - - stateListener.onSubchannelState(mockSubchannel, + loadBalancer.handleSubchannelState(mockSubchannel, ConnectivityStateInfo.forTransientFailure(Status.UNAVAILABLE)); - inOrder.verify(mockHelper).updateBalancingState( - eq(TRANSIENT_FAILURE), any(SubchannelPicker.class)); - Status error = Status.NOT_FOUND.withDescription("nameResolutionError"); loadBalancer.handleNameResolutionError(error); inOrder.verify(mockHelper).updateBalancingState(eq(TRANSIENT_FAILURE), pickerCaptor.capture()); @@ -264,7 +248,7 @@ public void nameResolutionErrorWithStateChanges() throws Exception { assertEquals(null, pickResult.getSubchannel()); assertEquals(error, pickResult.getStatus()); - stateListener.onSubchannelState(mockSubchannel, ConnectivityStateInfo.forNonError(READY)); + loadBalancer.handleSubchannelState(mockSubchannel, ConnectivityStateInfo.forNonError(READY)); Status error2 = Status.NOT_FOUND.withDescription("nameResolutionError2"); loadBalancer.handleNameResolutionError(error2); inOrder.verify(mockHelper).updateBalancingState(eq(TRANSIENT_FAILURE), pickerCaptor.capture()); @@ -280,12 +264,7 @@ public void nameResolutionErrorWithStateChanges() throws Exception { public void requestConnection() { loadBalancer.handleResolvedAddresses( ResolvedAddresses.newBuilder().setAddresses(servers).setAttributes(affinity).build()); - verify(mockHelper).createSubchannel(createArgsCaptor.capture()); - CreateSubchannelArgs args = createArgsCaptor.getValue(); - assertThat(args.getAddresses()).isEqualTo(servers); - SubchannelStateListener stateListener = args.getStateListener(); - - stateListener.onSubchannelState(mockSubchannel, ConnectivityStateInfo.forNonError(IDLE)); + loadBalancer.handleSubchannelState(mockSubchannel, ConnectivityStateInfo.forNonError(IDLE)); verify(mockHelper).updateBalancingState(eq(IDLE), pickerCaptor.capture()); SubchannelPicker picker = pickerCaptor.getValue(); diff --git a/core/src/test/java/io/grpc/util/RoundRobinLoadBalancerTest.java b/core/src/test/java/io/grpc/util/RoundRobinLoadBalancerTest.java index a9e492fec7d..f31a9d5d8da 100644 --- a/core/src/test/java/io/grpc/util/RoundRobinLoadBalancerTest.java +++ b/core/src/test/java/io/grpc/util/RoundRobinLoadBalancerTest.java @@ -34,6 +34,7 @@ import static org.mockito.ArgumentMatchers.eq; import static org.mockito.ArgumentMatchers.isA; import static org.mockito.Mockito.atLeast; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.mock; @@ -50,14 +51,12 @@ import io.grpc.ConnectivityStateInfo; import io.grpc.EquivalentAddressGroup; import io.grpc.LoadBalancer; -import io.grpc.LoadBalancer.CreateSubchannelArgs; import io.grpc.LoadBalancer.Helper; import io.grpc.LoadBalancer.PickResult; import io.grpc.LoadBalancer.PickSubchannelArgs; import io.grpc.LoadBalancer.ResolvedAddresses; import io.grpc.LoadBalancer.Subchannel; import io.grpc.LoadBalancer.SubchannelPicker; -import io.grpc.LoadBalancer.SubchannelStateListener; import io.grpc.Metadata; import io.grpc.Metadata.Key; import io.grpc.Status; @@ -80,7 +79,6 @@ import org.junit.runner.RunWith; import org.junit.runners.JUnit4; import org.mockito.ArgumentCaptor; -import org.mockito.ArgumentMatchers; import org.mockito.Captor; import org.mockito.InOrder; import org.mockito.Mock; @@ -96,8 +94,6 @@ public class RoundRobinLoadBalancerTest { private RoundRobinLoadBalancer loadBalancer; private final List servers = Lists.newArrayList(); private final Map, Subchannel> subchannels = Maps.newLinkedHashMap(); - private final Map subchannelStateListeners = - Maps.newLinkedHashMap(); private final Attributes affinity = Attributes.newBuilder().set(MAJOR_KEY, "I got the keys").build(); @@ -106,7 +102,7 @@ public class RoundRobinLoadBalancerTest { @Captor private ArgumentCaptor stateCaptor; @Captor - private ArgumentCaptor createArgsCaptor; + private ArgumentCaptor> eagListCaptor; @Mock private Helper mockHelper; @@ -123,18 +119,17 @@ public void setUp() { EquivalentAddressGroup eag = new EquivalentAddressGroup(addr); servers.add(eag); Subchannel sc = mock(Subchannel.class); + when(sc.getAllAddresses()).thenReturn(Arrays.asList(eag)); subchannels.put(Arrays.asList(eag), sc); } - when(mockHelper.createSubchannel(any(CreateSubchannelArgs.class))) + when(mockHelper.createSubchannel(any(List.class), any(Attributes.class))) .then(new Answer() { @Override public Subchannel answer(InvocationOnMock invocation) throws Throwable { - CreateSubchannelArgs args = (CreateSubchannelArgs) invocation.getArguments()[0]; - Subchannel subchannel = subchannels.get(args.getAddresses()); - when(subchannel.getAllAddresses()).thenReturn(args.getAddresses()); - when(subchannel.getAttributes()).thenReturn(args.getAttributes()); - subchannelStateListeners.put(subchannel, args.getStateListener()); + Object[] args = invocation.getArguments(); + Subchannel subchannel = subchannels.get(args[0]); + when(subchannel.getAttributes()).thenReturn((Attributes) args[1]); return subchannel; } }); @@ -143,11 +138,8 @@ public Subchannel answer(InvocationOnMock invocation) throws Throwable { } @After - @SuppressWarnings("deprecation") public void tearDown() throws Exception { verifyNoMoreInteractions(mockArgs); - verify(mockHelper, never()).createSubchannel( - ArgumentMatchers.>any(), any(Attributes.class)); } @Test @@ -155,15 +147,12 @@ public void pickAfterResolved() throws Exception { final Subchannel readySubchannel = subchannels.values().iterator().next(); loadBalancer.handleResolvedAddresses( ResolvedAddresses.newBuilder().setAddresses(servers).setAttributes(affinity).build()); - deliverSubchannelState(readySubchannel, ConnectivityStateInfo.forNonError(READY)); + loadBalancer.handleSubchannelState(readySubchannel, ConnectivityStateInfo.forNonError(READY)); - verify(mockHelper, times(3)).createSubchannel(createArgsCaptor.capture()); - List> capturedAddrs = new ArrayList<>(); - for (CreateSubchannelArgs arg : createArgsCaptor.getAllValues()) { - capturedAddrs.add(arg.getAddresses()); - } + verify(mockHelper, times(3)).createSubchannel(eagListCaptor.capture(), + any(Attributes.class)); - assertThat(capturedAddrs).containsAtLeastElementsIn(subchannels.keySet()); + assertThat(eagListCaptor.getAllValues()).containsAtLeastElementsIn(subchannels.keySet()); for (Subchannel subchannel : subchannels.values()) { verify(subchannel).requestConnection(); verify(subchannel, never()).shutdown(); @@ -198,26 +187,36 @@ public void pickAfterResolvedUpdatedHosts() throws Exception { Subchannel subchannel = allSubchannels.get(i); List eagList = Arrays.asList(new EquivalentAddressGroup(allAddrs.get(i))); - subchannels.put(eagList, subchannel); + when(subchannel.getAttributes()).thenReturn(Attributes.newBuilder().set(STATE_INFO, + new Ref<>( + ConnectivityStateInfo.forNonError(READY))).build()); + when(subchannel.getAllAddresses()).thenReturn(eagList); } + final Map, Subchannel> subchannels2 = Maps.newHashMap(); + subchannels2.put(Arrays.asList(new EquivalentAddressGroup(removedAddr)), removedSubchannel); + subchannels2.put(Arrays.asList(new EquivalentAddressGroup(oldAddr)), oldSubchannel); + List currentServers = Lists.newArrayList( new EquivalentAddressGroup(removedAddr), new EquivalentAddressGroup(oldAddr)); - InOrder inOrder = inOrder(mockHelper); + doAnswer(new Answer() { + @Override + public Subchannel answer(InvocationOnMock invocation) throws Throwable { + Object[] args = invocation.getArguments(); + return subchannels2.get(args[0]); + } + }).when(mockHelper).createSubchannel(any(List.class), any(Attributes.class)); loadBalancer.handleResolvedAddresses( ResolvedAddresses.newBuilder().setAddresses(currentServers).setAttributes(affinity) .build()); - inOrder.verify(mockHelper).updateBalancingState(eq(CONNECTING), pickerCaptor.capture()); - - deliverSubchannelState(removedSubchannel, ConnectivityStateInfo.forNonError(READY)); - deliverSubchannelState(oldSubchannel, ConnectivityStateInfo.forNonError(READY)); + InOrder inOrder = inOrder(mockHelper); - inOrder.verify(mockHelper, times(2)).updateBalancingState(eq(READY), pickerCaptor.capture()); + inOrder.verify(mockHelper).updateBalancingState(eq(READY), pickerCaptor.capture()); SubchannelPicker picker = pickerCaptor.getValue(); assertThat(getList(picker)).containsExactly(removedSubchannel, oldSubchannel); @@ -227,6 +226,10 @@ public void pickAfterResolvedUpdatedHosts() throws Exception { assertThat(loadBalancer.getSubchannels()).containsExactly(removedSubchannel, oldSubchannel); + subchannels2.clear(); + subchannels2.put(Arrays.asList(new EquivalentAddressGroup(oldAddr)), oldSubchannel); + subchannels2.put(Arrays.asList(new EquivalentAddressGroup(newAddr)), newSubchannel); + List latestServers = Lists.newArrayList( new EquivalentAddressGroup(oldAddr), @@ -238,14 +241,14 @@ public void pickAfterResolvedUpdatedHosts() throws Exception { verify(newSubchannel, times(1)).requestConnection(); verify(removedSubchannel, times(1)).shutdown(); - deliverSubchannelState(removedSubchannel, ConnectivityStateInfo.forNonError(SHUTDOWN)); - deliverSubchannelState(newSubchannel, ConnectivityStateInfo.forNonError(READY)); + loadBalancer.handleSubchannelState(removedSubchannel, + ConnectivityStateInfo.forNonError(SHUTDOWN)); assertThat(loadBalancer.getSubchannels()).containsExactly(oldSubchannel, newSubchannel); - verify(mockHelper, times(3)).createSubchannel(any(CreateSubchannelArgs.class)); - inOrder.verify(mockHelper, times(2)).updateBalancingState(eq(READY), pickerCaptor.capture()); + verify(mockHelper, times(3)).createSubchannel(any(List.class), any(Attributes.class)); + inOrder.verify(mockHelper).updateBalancingState(eq(READY), pickerCaptor.capture()); picker = pickerCaptor.getValue(); assertThat(getList(picker)).containsExactly(oldSubchannel, newSubchannel); @@ -277,7 +280,7 @@ public void pickAfterStateChange() throws Exception { inOrder.verify(mockHelper).updateBalancingState(eq(CONNECTING), isA(EmptyPicker.class)); assertThat(subchannelStateInfo.value).isEqualTo(ConnectivityStateInfo.forNonError(IDLE)); - deliverSubchannelState(subchannel, + loadBalancer.handleSubchannelState(subchannel, ConnectivityStateInfo.forNonError(READY)); inOrder.verify(mockHelper).updateBalancingState(eq(READY), pickerCaptor.capture()); assertThat(pickerCaptor.getValue()).isInstanceOf(ReadyPicker.class); @@ -285,20 +288,20 @@ public void pickAfterStateChange() throws Exception { ConnectivityStateInfo.forNonError(READY)); Status error = Status.UNKNOWN.withDescription("¯\\_(ツ)_//¯"); - deliverSubchannelState(subchannel, + loadBalancer.handleSubchannelState(subchannel, ConnectivityStateInfo.forTransientFailure(error)); assertThat(subchannelStateInfo.value).isEqualTo( ConnectivityStateInfo.forTransientFailure(error)); inOrder.verify(mockHelper).updateBalancingState(eq(CONNECTING), pickerCaptor.capture()); assertThat(pickerCaptor.getValue()).isInstanceOf(EmptyPicker.class); - deliverSubchannelState(subchannel, + loadBalancer.handleSubchannelState(subchannel, ConnectivityStateInfo.forNonError(IDLE)); assertThat(subchannelStateInfo.value).isEqualTo( ConnectivityStateInfo.forNonError(IDLE)); verify(subchannel, times(2)).requestConnection(); - verify(mockHelper, times(3)).createSubchannel(any(CreateSubchannelArgs.class)); + verify(mockHelper, times(3)).createSubchannel(any(List.class), any(Attributes.class)); verifyNoMoreInteractions(mockHelper); } @@ -350,10 +353,10 @@ public void nameResolutionErrorWithActiveChannels() throws Exception { final Subchannel readySubchannel = subchannels.values().iterator().next(); loadBalancer.handleResolvedAddresses( ResolvedAddresses.newBuilder().setAddresses(servers).setAttributes(affinity).build()); - deliverSubchannelState(readySubchannel, ConnectivityStateInfo.forNonError(READY)); + loadBalancer.handleSubchannelState(readySubchannel, ConnectivityStateInfo.forNonError(READY)); loadBalancer.handleNameResolutionError(Status.NOT_FOUND.withDescription("nameResolutionError")); - verify(mockHelper, times(3)).createSubchannel(any(CreateSubchannelArgs.class)); + verify(mockHelper, times(3)).createSubchannel(any(List.class), any(Attributes.class)); verify(mockHelper, times(3)) .updateBalancingState(stateCaptor.capture(), pickerCaptor.capture()); @@ -385,11 +388,12 @@ public void subchannelStateIsolation() throws Exception { verify(sc2, times(1)).requestConnection(); verify(sc3, times(1)).requestConnection(); - deliverSubchannelState(sc1, ConnectivityStateInfo.forNonError(READY)); - deliverSubchannelState(sc2, ConnectivityStateInfo.forNonError(READY)); - deliverSubchannelState(sc3, ConnectivityStateInfo.forNonError(READY)); - deliverSubchannelState(sc2, ConnectivityStateInfo.forNonError(IDLE)); - deliverSubchannelState(sc3, ConnectivityStateInfo.forTransientFailure(Status.UNAVAILABLE)); + loadBalancer.handleSubchannelState(sc1, ConnectivityStateInfo.forNonError(READY)); + loadBalancer.handleSubchannelState(sc2, ConnectivityStateInfo.forNonError(READY)); + loadBalancer.handleSubchannelState(sc3, ConnectivityStateInfo.forNonError(READY)); + loadBalancer.handleSubchannelState(sc2, ConnectivityStateInfo.forNonError(IDLE)); + loadBalancer + .handleSubchannelState(sc3, ConnectivityStateInfo.forTransientFailure(Status.UNAVAILABLE)); verify(mockHelper, times(6)) .updateBalancingState(stateCaptor.capture(), pickerCaptor.capture()); @@ -422,7 +426,7 @@ public void noStickinessEnabled_withStickyHeader() { ResolvedAddresses.newBuilder().setAddresses(servers).setAttributes(Attributes.EMPTY) .build()); for (Subchannel subchannel : subchannels.values()) { - deliverSubchannelState(subchannel, ConnectivityStateInfo.forNonError(READY)); + loadBalancer.handleSubchannelState(subchannel, ConnectivityStateInfo.forNonError(READY)); } verify(mockHelper, times(4)) .updateBalancingState(any(ConnectivityState.class), pickerCaptor.capture()); @@ -456,7 +460,7 @@ public void stickinessEnabled_withoutStickyHeader() { loadBalancer.handleResolvedAddresses( ResolvedAddresses.newBuilder().setAddresses(servers).setAttributes(attributes).build()); for (Subchannel subchannel : subchannels.values()) { - deliverSubchannelState(subchannel, ConnectivityStateInfo.forNonError(READY)); + loadBalancer.handleSubchannelState(subchannel, ConnectivityStateInfo.forNonError(READY)); } verify(mockHelper, times(4)) .updateBalancingState(stateCaptor.capture(), pickerCaptor.capture()); @@ -489,7 +493,7 @@ public void stickinessEnabled_withStickyHeader() { loadBalancer.handleResolvedAddresses( ResolvedAddresses.newBuilder().setAddresses(servers).setAttributes(attributes).build()); for (Subchannel subchannel : subchannels.values()) { - deliverSubchannelState(subchannel, ConnectivityStateInfo.forNonError(READY)); + loadBalancer.handleSubchannelState(subchannel, ConnectivityStateInfo.forNonError(READY)); } verify(mockHelper, times(4)) .updateBalancingState(stateCaptor.capture(), pickerCaptor.capture()); @@ -520,7 +524,7 @@ public void stickinessEnabled_withDifferentStickyHeaders() { loadBalancer.handleResolvedAddresses( ResolvedAddresses.newBuilder().setAddresses(servers).setAttributes(attributes).build()); for (Subchannel subchannel : subchannels.values()) { - deliverSubchannelState(subchannel, ConnectivityStateInfo.forNonError(READY)); + loadBalancer.handleSubchannelState(subchannel, ConnectivityStateInfo.forNonError(READY)); } verify(mockHelper, times(4)) .updateBalancingState(stateCaptor.capture(), pickerCaptor.capture()); @@ -566,7 +570,7 @@ public void stickiness_goToTransientFailure_pick_backToReady() { loadBalancer.handleResolvedAddresses( ResolvedAddresses.newBuilder().setAddresses(servers).setAttributes(attributes).build()); for (Subchannel subchannel : subchannels.values()) { - deliverSubchannelState(subchannel, ConnectivityStateInfo.forNonError(READY)); + loadBalancer.handleSubchannelState(subchannel, ConnectivityStateInfo.forNonError(READY)); } verify(mockHelper, times(4)) .updateBalancingState(stateCaptor.capture(), pickerCaptor.capture()); @@ -581,7 +585,8 @@ public void stickiness_goToTransientFailure_pick_backToReady() { Subchannel sc1 = picker.pickSubchannel(mockArgs).getSubchannel(); // go to transient failure - deliverSubchannelState(sc1, ConnectivityStateInfo.forTransientFailure(Status.UNAVAILABLE)); + loadBalancer + .handleSubchannelState(sc1, ConnectivityStateInfo.forTransientFailure(Status.UNAVAILABLE)); verify(mockHelper, times(5)) .updateBalancingState(stateCaptor.capture(), pickerCaptor.capture()); @@ -591,7 +596,7 @@ public void stickiness_goToTransientFailure_pick_backToReady() { Subchannel sc2 = picker.pickSubchannel(mockArgs).getSubchannel(); // go back to ready - deliverSubchannelState(sc1, ConnectivityStateInfo.forNonError(READY)); + loadBalancer.handleSubchannelState(sc1, ConnectivityStateInfo.forNonError(READY)); verify(mockHelper, times(6)) .updateBalancingState(stateCaptor.capture(), pickerCaptor.capture()); @@ -614,7 +619,7 @@ public void stickiness_goToTransientFailure_backToReady_pick() { loadBalancer.handleResolvedAddresses( ResolvedAddresses.newBuilder().setAddresses(servers).setAttributes(attributes).build()); for (Subchannel subchannel : subchannels.values()) { - deliverSubchannelState(subchannel, ConnectivityStateInfo.forNonError(READY)); + loadBalancer.handleSubchannelState(subchannel, ConnectivityStateInfo.forNonError(READY)); } verify(mockHelper, times(4)) .updateBalancingState(stateCaptor.capture(), pickerCaptor.capture()); @@ -629,7 +634,8 @@ public void stickiness_goToTransientFailure_backToReady_pick() { Subchannel sc1 = picker.pickSubchannel(mockArgs).getSubchannel(); // go to transient failure - deliverSubchannelState(sc1, ConnectivityStateInfo.forTransientFailure(Status.UNAVAILABLE)); + loadBalancer + .handleSubchannelState(sc1, ConnectivityStateInfo.forTransientFailure(Status.UNAVAILABLE)); Metadata headerWithStickinessValue2 = new Metadata(); headerWithStickinessValue2.put(stickinessKey, "my-sticky-value2"); @@ -643,7 +649,7 @@ public void stickiness_goToTransientFailure_backToReady_pick() { Subchannel sc2 = picker.pickSubchannel(mockArgs).getSubchannel(); // go back to ready - deliverSubchannelState(sc1, ConnectivityStateInfo.forNonError(READY)); + loadBalancer.handleSubchannelState(sc1, ConnectivityStateInfo.forNonError(READY)); doReturn(headerWithStickinessValue1).when(mockArgs).getHeaders(); verify(mockHelper, times(6)) @@ -668,7 +674,7 @@ public void stickiness_oneSubchannelShutdown() { loadBalancer.handleResolvedAddresses( ResolvedAddresses.newBuilder().setAddresses(servers).setAttributes(attributes).build()); for (Subchannel subchannel : subchannels.values()) { - deliverSubchannelState(subchannel, ConnectivityStateInfo.forNonError(READY)); + loadBalancer.handleSubchannelState(subchannel, ConnectivityStateInfo.forNonError(READY)); } verify(mockHelper, times(4)) .updateBalancingState(stateCaptor.capture(), pickerCaptor.capture()); @@ -684,7 +690,8 @@ public void stickiness_oneSubchannelShutdown() { Subchannel sc1 = picker.pickSubchannel(mockArgs).getSubchannel(); // shutdown channel directly - deliverSubchannelState(sc1, ConnectivityStateInfo.forNonError(ConnectivityState.SHUTDOWN)); + loadBalancer + .handleSubchannelState(sc1, ConnectivityStateInfo.forNonError(ConnectivityState.SHUTDOWN)); assertNull(loadBalancer.getStickinessMapForTest().get("my-sticky-value").value); @@ -706,7 +713,7 @@ public void stickiness_oneSubchannelShutdown() { verify(sc2, times(1)).shutdown(); - deliverSubchannelState(sc2, ConnectivityStateInfo.forNonError(SHUTDOWN)); + loadBalancer.handleSubchannelState(sc2, ConnectivityStateInfo.forNonError(SHUTDOWN)); assertNull(loadBalancer.getStickinessMapForTest().get("my-sticky-value").value); @@ -792,10 +799,6 @@ private static List getList(SubchannelPicker picker) { Collections.emptyList(); } - private void deliverSubchannelState(Subchannel subchannel, ConnectivityStateInfo newState) { - subchannelStateListeners.get(subchannel).onSubchannelState(subchannel, newState); - } - private static class FakeSocketAddress extends SocketAddress { final String name; diff --git a/grpclb/src/main/java/io/grpc/grpclb/CachedSubchannelPool.java b/grpclb/src/main/java/io/grpc/grpclb/CachedSubchannelPool.java index a63ce4089be..b70b5d59077 100644 --- a/grpclb/src/main/java/io/grpc/grpclb/CachedSubchannelPool.java +++ b/grpclb/src/main/java/io/grpc/grpclb/CachedSubchannelPool.java @@ -16,7 +16,6 @@ package io.grpc.grpclb; -import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; @@ -24,10 +23,9 @@ import io.grpc.Attributes; import io.grpc.ConnectivityStateInfo; import io.grpc.EquivalentAddressGroup; -import io.grpc.LoadBalancer.CreateSubchannelArgs; +import io.grpc.LoadBalancer; import io.grpc.LoadBalancer.Helper; import io.grpc.LoadBalancer.Subchannel; -import io.grpc.LoadBalancer.SubchannelStateListener; import io.grpc.SynchronizationContext.ScheduledHandle; import java.util.HashMap; import java.util.concurrent.TimeUnit; @@ -41,41 +39,33 @@ final class CachedSubchannelPool implements SubchannelPool { new HashMap<>(); private Helper helper; + private LoadBalancer lb; @VisibleForTesting static final long SHUTDOWN_TIMEOUT_MS = 10000; @Override - public void init(Helper helper) { + public void init(Helper helper, LoadBalancer lb) { this.helper = checkNotNull(helper, "helper"); + this.lb = checkNotNull(lb, "lb"); } @Override public Subchannel takeOrCreateSubchannel( - EquivalentAddressGroup eag, Attributes defaultAttributes, SubchannelStateListener listener) { - final CacheEntry entry = cache.get(eag); + EquivalentAddressGroup eag, Attributes defaultAttributes) { + final CacheEntry entry = cache.remove(eag); final Subchannel subchannel; if (entry == null) { - final CacheEntry newEntry = new CacheEntry(); - subchannel = helper.createSubchannel(CreateSubchannelArgs.newBuilder() - .setAddresses(eag) - .setAttributes(defaultAttributes) - .setStateListener(new StateListener(newEntry)) - .build()); - newEntry.init(subchannel); - cache.put(eag, newEntry); - newEntry.taken(listener); + subchannel = helper.createSubchannel(eag, defaultAttributes); } else { subchannel = entry.subchannel; - checkState(eag.equals(subchannel.getAddresses()), - "Unexpected address change from %s to %s", eag, subchannel.getAddresses()); - entry.taken(listener); - // Make the listener up-to-date with the latest state in case it has changed while it's in the - // cache. + entry.shutdownTimer.cancel(); + // Make the balancer up-to-date with the latest state in case it has changed while it's + // in the cache. helper.getSynchronizationContext().execute(new Runnable() { @Override public void run() { - entry.maybeNotifyStateListener(); + lb.handleSubchannelState(subchannel, entry.state); } }); } @@ -83,19 +73,40 @@ public void run() { } @Override - public void returnSubchannel(Subchannel subchannel) { - CacheEntry entry = cache.get(subchannel.getAddresses()); - checkArgument(entry != null, "Cache record for %s not found", subchannel); - checkArgument(entry.subchannel == subchannel, - "Subchannel being returned (%s) doesn't match the cache (%s)", - subchannel, entry.subchannel); - entry.returned(); + public void handleSubchannelState(Subchannel subchannel, ConnectivityStateInfo newStateInfo) { + CacheEntry cached = cache.get(subchannel.getAddresses()); + if (cached == null || cached.subchannel != subchannel) { + // Given subchannel is not cached. Not our responsibility. + return; + } + cached.state = newStateInfo; + } + + @Override + public void returnSubchannel(Subchannel subchannel, ConnectivityStateInfo lastKnownState) { + CacheEntry prev = cache.get(subchannel.getAddresses()); + if (prev != null) { + // Returning the same Subchannel twice has no effect. + // Returning a different Subchannel for an already cached EAG will cause the + // latter Subchannel to be shutdown immediately. + if (prev.subchannel != subchannel) { + subchannel.shutdown(); + } + return; + } + final ShutdownSubchannelTask shutdownTask = new ShutdownSubchannelTask(subchannel); + ScheduledHandle shutdownTimer = + helper.getSynchronizationContext().schedule( + shutdownTask, SHUTDOWN_TIMEOUT_MS, TimeUnit.MILLISECONDS, + helper.getScheduledExecutorService()); + CacheEntry entry = new CacheEntry(subchannel, shutdownTimer, lastKnownState); + cache.put(subchannel.getAddresses(), entry); } @Override public void clear() { for (CacheEntry entry : cache.values()) { - entry.cancelShutdownTimer(); + entry.shutdownTimer.cancel(); entry.subchannel.shutdown(); } cache.clear(); @@ -114,65 +125,19 @@ private ShutdownSubchannelTask(Subchannel subchannel) { public void run() { CacheEntry entry = cache.remove(subchannel.getAddresses()); checkState(entry.subchannel == subchannel, "Inconsistent state"); - entry.cancelShutdownTimer(); subchannel.shutdown(); } } - private class CacheEntry { - Subchannel subchannel; - ScheduledHandle shutdownTimer; + private static class CacheEntry { + final Subchannel subchannel; + final ScheduledHandle shutdownTimer; ConnectivityStateInfo state; - // Not null if outside of pool - SubchannelStateListener stateListener; - void init(Subchannel subchannel) { + CacheEntry(Subchannel subchannel, ScheduledHandle shutdownTimer, ConnectivityStateInfo state) { this.subchannel = checkNotNull(subchannel, "subchannel"); - } - - void cancelShutdownTimer() { - if (shutdownTimer != null) { - shutdownTimer.cancel(); - shutdownTimer = null; - } - } - - void taken(SubchannelStateListener listener) { - checkState(stateListener == null, "Already out of pool"); - stateListener = checkNotNull(listener, "listener"); - cancelShutdownTimer(); - } - - void returned() { - checkState(stateListener != null, "Already in pool"); - if (shutdownTimer == null) { - shutdownTimer = helper.getSynchronizationContext().schedule( - new ShutdownSubchannelTask(subchannel), SHUTDOWN_TIMEOUT_MS, TimeUnit.MILLISECONDS, - helper.getScheduledExecutorService()); - } else { - checkState(shutdownTimer.isPending()); - } - stateListener = null; - } - - void maybeNotifyStateListener() { - if (stateListener != null && state != null) { - stateListener.onSubchannelState(subchannel, state); - } - } - } - - private static final class StateListener implements SubchannelStateListener { - private final CacheEntry entry; - - StateListener(CacheEntry entry) { - this.entry = checkNotNull(entry, "entry"); - } - - @Override - public void onSubchannelState(Subchannel subchannel, ConnectivityStateInfo newState) { - entry.state = newState; - entry.maybeNotifyStateListener(); + this.shutdownTimer = checkNotNull(shutdownTimer, "shutdownTimer"); + this.state = checkNotNull(state, "state"); } } } diff --git a/grpclb/src/main/java/io/grpc/grpclb/GrpclbLoadBalancer.java b/grpclb/src/main/java/io/grpc/grpclb/GrpclbLoadBalancer.java index 8797c82d790..88dd4378dc4 100644 --- a/grpclb/src/main/java/io/grpc/grpclb/GrpclbLoadBalancer.java +++ b/grpclb/src/main/java/io/grpc/grpclb/GrpclbLoadBalancer.java @@ -24,6 +24,7 @@ import io.grpc.Attributes; import io.grpc.ChannelLogger; import io.grpc.ChannelLogger.ChannelLogLevel; +import io.grpc.ConnectivityStateInfo; import io.grpc.EquivalentAddressGroup; import io.grpc.LoadBalancer; import io.grpc.Status; @@ -74,11 +75,18 @@ class GrpclbLoadBalancer extends LoadBalancer { this.stopwatch = checkNotNull(stopwatch, "stopwatch"); this.backoffPolicyProvider = checkNotNull(backoffPolicyProvider, "backoffPolicyProvider"); this.subchannelPool = checkNotNull(subchannelPool, "subchannelPool"); - this.subchannelPool.init(helper); + this.subchannelPool.init(helper, this); recreateStates(); checkNotNull(grpclbState, "grpclbState"); } + @Override + public void handleSubchannelState(Subchannel subchannel, ConnectivityStateInfo newState) { + // grpclbState should never be null here since handleSubchannelState cannot be called while the + // lb is shutdown. + grpclbState.handleSubchannelState(subchannel, newState); + } + @Override public void handleResolvedAddresses(ResolvedAddresses resolvedAddresses) { List updatedServers = resolvedAddresses.getAddresses(); diff --git a/grpclb/src/main/java/io/grpc/grpclb/GrpclbState.java b/grpclb/src/main/java/io/grpc/grpclb/GrpclbState.java index a9360099312..14295aca0b8 100644 --- a/grpclb/src/main/java/io/grpc/grpclb/GrpclbState.java +++ b/grpclb/src/main/java/io/grpc/grpclb/GrpclbState.java @@ -35,13 +35,11 @@ import io.grpc.ConnectivityState; import io.grpc.ConnectivityStateInfo; import io.grpc.EquivalentAddressGroup; -import io.grpc.LoadBalancer.CreateSubchannelArgs; import io.grpc.LoadBalancer.Helper; import io.grpc.LoadBalancer.PickResult; import io.grpc.LoadBalancer.PickSubchannelArgs; import io.grpc.LoadBalancer.Subchannel; import io.grpc.LoadBalancer.SubchannelPicker; -import io.grpc.LoadBalancer.SubchannelStateListener; import io.grpc.ManagedChannel; import io.grpc.Metadata; import io.grpc.Status; @@ -83,7 +81,7 @@ * switches away from GRPCLB mode. */ @NotThreadSafe -final class GrpclbState implements SubchannelStateListener { +final class GrpclbState { static final long FALLBACK_TIMEOUT_MS = TimeUnit.SECONDS.toMillis(10); private static final Attributes LB_PROVIDED_BACKEND_ATTRS = Attributes.newBuilder().set(GrpcAttributes.ATTR_LB_PROVIDED_BACKEND, true).build(); @@ -173,12 +171,14 @@ static enum Mode { this.logger = checkNotNull(helper.getChannelLogger(), "logger"); } - @Override - public void onSubchannelState(Subchannel subchannel, ConnectivityStateInfo newState) { + void handleSubchannelState(Subchannel subchannel, ConnectivityStateInfo newState) { if (newState.getState() == SHUTDOWN) { return; } if (!subchannels.values().contains(subchannel)) { + if (subchannelPool != null ) { + subchannelPool.handleSubchannelState(subchannel, newState); + } return; } if (mode == Mode.ROUND_ROBIN && newState.getState() == IDLE) { @@ -323,7 +323,7 @@ void shutdown() { // We close the subchannels through subchannelPool instead of helper just for convenience of // testing. for (Subchannel subchannel : subchannels.values()) { - subchannelPool.returnSubchannel(subchannel); + returnSubchannelToPool(subchannel); } subchannelPool.clear(); break; @@ -347,6 +347,10 @@ void propagateError(Status status) { } } + private void returnSubchannelToPool(Subchannel subchannel) { + subchannelPool.returnSubchannel(subchannel, subchannel.getAttributes().get(STATE_INFO).get()); + } + @VisibleForTesting @Nullable GrpclbClientLoadRecorder getLoadRecorder() { @@ -377,8 +381,7 @@ private void useRoundRobinLists( if (subchannel == null) { subchannel = subchannels.get(eagAsList); if (subchannel == null) { - subchannel = subchannelPool.takeOrCreateSubchannel( - eag, createSubchannelAttrs(), this); + subchannel = subchannelPool.takeOrCreateSubchannel(eag, createSubchannelAttrs()); subchannel.requestConnection(); } newSubchannelMap.put(eagAsList, subchannel); @@ -396,7 +399,7 @@ private void useRoundRobinLists( for (Entry, Subchannel> entry : subchannels.entrySet()) { List eagList = entry.getKey(); if (!newSubchannelMap.containsKey(eagList)) { - subchannelPool.returnSubchannel(entry.getValue()); + returnSubchannelToPool(entry.getValue()); } } subchannels = Collections.unmodifiableMap(newSubchannelMap); @@ -419,12 +422,7 @@ private void useRoundRobinLists( } Subchannel subchannel; if (subchannels.isEmpty()) { - subchannel = - helper.createSubchannel(CreateSubchannelArgs.newBuilder() - .setAddresses(eagList) - .setAttributes(createSubchannelAttrs()) - .setStateListener(this) - .build()); + subchannel = helper.createSubchannel(eagList, createSubchannelAttrs()); } else { checkState(subchannels.size() == 1, "Unexpected Subchannel count: %s", subchannels); subchannel = subchannels.values().iterator().next(); diff --git a/grpclb/src/main/java/io/grpc/grpclb/SubchannelPool.java b/grpclb/src/main/java/io/grpc/grpclb/SubchannelPool.java index f210dfdf5a4..0d328fdb090 100644 --- a/grpclb/src/main/java/io/grpc/grpclb/SubchannelPool.java +++ b/grpclb/src/main/java/io/grpc/grpclb/SubchannelPool.java @@ -17,10 +17,11 @@ package io.grpc.grpclb; import io.grpc.Attributes; +import io.grpc.ConnectivityStateInfo; import io.grpc.EquivalentAddressGroup; +import io.grpc.LoadBalancer; import io.grpc.LoadBalancer.Helper; import io.grpc.LoadBalancer.Subchannel; -import io.grpc.LoadBalancer.SubchannelStateListener; import javax.annotation.concurrent.NotThreadSafe; /** @@ -33,33 +34,26 @@ interface SubchannelPool { /** * Pass essential utilities and the balancer that's using this pool. */ - void init(Helper helper); + void init(Helper helper, LoadBalancer lb); /** * Takes a {@link Subchannel} from the pool for the given {@code eag} if there is one available. * Otherwise, creates and returns a new {@code Subchannel} with the given {@code eag} and {@code * defaultAttributes}. - * - *

There can be at most one Subchannel for each EAG. After a Subchannel is taken out of the - * pool, it must be returned before the same EAG can be used to call this method. - * - * @param defaultAttributes the attributes used to create the Subchannel. Not used if a pooled - * subchannel is returned. - * @param stateListener receives state updates from now on */ - Subchannel takeOrCreateSubchannel( - EquivalentAddressGroup eag, Attributes defaultAttributes, - SubchannelStateListener stateListener); + Subchannel takeOrCreateSubchannel(EquivalentAddressGroup eag, Attributes defaultAttributes); + + /** + * Gets notified about a state change of Subchannel that is possibly cached in this pool. Do + * nothing if this pool doesn't own this Subchannel. + */ + void handleSubchannelState(Subchannel subchannel, ConnectivityStateInfo newStateInfo); /** * Puts a {@link Subchannel} back to the pool. From this point the Subchannel is owned by the - * pool, and the caller should stop referencing to this Subchannel. The {@link - * SubchannelStateListener} will not receive any more updates. - * - *

Can only be called with a Subchannel created by this pool. Must not be called if the - * Subchannel is already in the pool. + * pool, and the caller should stop referencing to this Subchannel. */ - void returnSubchannel(Subchannel subchannel); + void returnSubchannel(Subchannel subchannel, ConnectivityStateInfo lastKnownState); /** * Shuts down all subchannels in the pool immediately. diff --git a/grpclb/src/test/java/io/grpc/grpclb/CachedSubchannelPoolTest.java b/grpclb/src/test/java/io/grpc/grpclb/CachedSubchannelPoolTest.java index beb17c07010..85657531473 100644 --- a/grpclb/src/test/java/io/grpc/grpclb/CachedSubchannelPoolTest.java +++ b/grpclb/src/test/java/io/grpc/grpclb/CachedSubchannelPoolTest.java @@ -19,8 +19,9 @@ import static com.google.common.truth.Truth.assertThat; import static io.grpc.grpclb.CachedSubchannelPool.SHUTDOWN_TIMEOUT_MS; import static java.util.concurrent.TimeUnit.MILLISECONDS; -import static org.junit.Assert.fail; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.any; +import static org.mockito.Mockito.atLeast; import static org.mockito.Mockito.atMost; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; @@ -32,26 +33,24 @@ import static org.mockito.Mockito.when; import io.grpc.Attributes; +import io.grpc.ConnectivityState; import io.grpc.ConnectivityStateInfo; import io.grpc.EquivalentAddressGroup; -import io.grpc.LoadBalancer.CreateSubchannelArgs; +import io.grpc.LoadBalancer; import io.grpc.LoadBalancer.Helper; import io.grpc.LoadBalancer.Subchannel; -import io.grpc.LoadBalancer.SubchannelStateListener; import io.grpc.Status; import io.grpc.SynchronizationContext; +import io.grpc.grpclb.CachedSubchannelPool.ShutdownSubchannelTask; import io.grpc.internal.FakeClock; import java.util.ArrayList; -import java.util.Collections; -import java.util.HashMap; +import java.util.Arrays; import java.util.List; -import java.util.Map; import org.junit.After; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; -import org.mockito.hamcrest.MockitoHamcrest; import org.mockito.invocation.InvocationOnMock; import org.mockito.stubbing.Answer; @@ -65,11 +64,23 @@ public class CachedSubchannelPoolTest { private static final Attributes.Key ATTR_KEY = Attributes.Key.create("test-attr"); private static final Attributes ATTRS1 = Attributes.newBuilder().set(ATTR_KEY, "1").build(); private static final Attributes ATTRS2 = Attributes.newBuilder().set(ATTR_KEY, "2").build(); + + private static final ConnectivityStateInfo READY_STATE = + ConnectivityStateInfo.forNonError(ConnectivityState.READY); private static final ConnectivityStateInfo TRANSIENT_FAILURE_STATE = ConnectivityStateInfo.forTransientFailure(Status.UNAVAILABLE.withDescription("Simulated")); + private static final FakeClock.TaskFilter SHUTDOWN_TASK_FILTER = + new FakeClock.TaskFilter() { + @Override + public boolean shouldAccept(Runnable command) { + // The task is wrapped by SynchronizationContext, so we can't compare the type + // directly. + return command.toString().contains(ShutdownSubchannelTask.class.getSimpleName()); + } + }; private final Helper helper = mock(Helper.class); - private final SubchannelStateListener mockListener = mock(SubchannelStateListener.class); + private final LoadBalancer balancer = mock(LoadBalancer.class); private final FakeClock clock = new FakeClock(); private final SynchronizationContext syncContext = new SynchronizationContext( new Thread.UncaughtExceptionHandler() { @@ -80,8 +91,6 @@ public void uncaughtException(Thread t, Throwable e) { }); private final CachedSubchannelPool pool = new CachedSubchannelPool(); private final ArrayList mockSubchannels = new ArrayList<>(); - // Listeners seen by the Helper - private final Map stateListeners = new HashMap<>(); @Before @SuppressWarnings("unchecked") @@ -90,17 +99,26 @@ public void setUp() { @Override public Subchannel answer(InvocationOnMock invocation) throws Throwable { Subchannel subchannel = mock(Subchannel.class); - CreateSubchannelArgs args = (CreateSubchannelArgs) invocation.getArguments()[0]; - when(subchannel.getAllAddresses()).thenReturn(args.getAddresses()); - when(subchannel.getAttributes()).thenReturn(args.getAttributes()); + List eagList = + (List) invocation.getArguments()[0]; + Attributes attrs = (Attributes) invocation.getArguments()[1]; + when(subchannel.getAllAddresses()).thenReturn(eagList); + when(subchannel.getAttributes()).thenReturn(attrs); mockSubchannels.add(subchannel); - stateListeners.put(subchannel, args.getStateListener()); return subchannel; } - }).when(helper).createSubchannel(any(CreateSubchannelArgs.class)); + }).when(helper).createSubchannel(any(List.class), any(Attributes.class)); + doAnswer(new Answer() { + @Override + public Void answer(InvocationOnMock invocation) throws Throwable { + syncContext.throwIfNotInThisSynchronizationContext(); + return null; + } + }).when(balancer).handleSubchannelState( + any(Subchannel.class), any(ConnectivityStateInfo.class)); when(helper.getSynchronizationContext()).thenReturn(syncContext); when(helper.getScheduledExecutorService()).thenReturn(clock.getScheduledExecutorService()); - pool.init(helper); + pool.init(helper, balancer); } @After @@ -109,26 +127,29 @@ public void wrapUp() { for (Subchannel subchannel : mockSubchannels) { verify(subchannel, atMost(1)).shutdown(); } + verify(balancer, atLeast(0)) + .handleSubchannelState(any(Subchannel.class), any(ConnectivityStateInfo.class)); + verifyNoMoreInteractions(balancer); } @Test public void subchannelExpireAfterReturned() { - Subchannel subchannel1 = pool.takeOrCreateSubchannel(EAG1, ATTRS1, mockListener); + Subchannel subchannel1 = pool.takeOrCreateSubchannel(EAG1, ATTRS1); assertThat(subchannel1).isNotNull(); - verify(helper).createSubchannel(argsWith(EAG1, "1")); + verify(helper).createSubchannel(eq(Arrays.asList(EAG1)), same(ATTRS1)); - Subchannel subchannel2 = pool.takeOrCreateSubchannel(EAG2, ATTRS2, mockListener); + Subchannel subchannel2 = pool.takeOrCreateSubchannel(EAG2, ATTRS2); assertThat(subchannel2).isNotNull(); assertThat(subchannel2).isNotSameInstanceAs(subchannel1); - verify(helper).createSubchannel(argsWith(EAG2, "2")); + verify(helper).createSubchannel(eq(Arrays.asList(EAG2)), same(ATTRS2)); - pool.returnSubchannel(subchannel1); + pool.returnSubchannel(subchannel1, READY_STATE); // subchannel1 is 1ms away from expiration. clock.forwardTime(SHUTDOWN_TIMEOUT_MS - 1, MILLISECONDS); verify(subchannel1, never()).shutdown(); - pool.returnSubchannel(subchannel2); + pool.returnSubchannel(subchannel2, READY_STATE); // subchannel1 expires. subchannel2 is (SHUTDOWN_TIMEOUT_MS - 1) away from expiration. clock.forwardTime(1, MILLISECONDS); @@ -143,25 +164,25 @@ public void subchannelExpireAfterReturned() { @Test public void subchannelReused() { - Subchannel subchannel1 = pool.takeOrCreateSubchannel(EAG1, ATTRS1, mockListener); + Subchannel subchannel1 = pool.takeOrCreateSubchannel(EAG1, ATTRS1); assertThat(subchannel1).isNotNull(); - verify(helper).createSubchannel(argsWith(EAG1, "1")); + verify(helper).createSubchannel(eq(Arrays.asList(EAG1)), same(ATTRS1)); - Subchannel subchannel2 = pool.takeOrCreateSubchannel(EAG2, ATTRS2, mockListener); + Subchannel subchannel2 = pool.takeOrCreateSubchannel(EAG2, ATTRS2); assertThat(subchannel2).isNotNull(); assertThat(subchannel2).isNotSameInstanceAs(subchannel1); - verify(helper).createSubchannel(argsWith(EAG2, "2")); + verify(helper).createSubchannel(eq(Arrays.asList(EAG2)), same(ATTRS2)); - pool.returnSubchannel(subchannel1); + pool.returnSubchannel(subchannel1, READY_STATE); // subchannel1 is 1ms away from expiration. clock.forwardTime(SHUTDOWN_TIMEOUT_MS - 1, MILLISECONDS); // This will cancel the shutdown timer for subchannel1 - Subchannel subchannel1a = pool.takeOrCreateSubchannel(EAG1, ATTRS1, mockListener); + Subchannel subchannel1a = pool.takeOrCreateSubchannel(EAG1, ATTRS1); assertThat(subchannel1a).isSameInstanceAs(subchannel1); - pool.returnSubchannel(subchannel2); + pool.returnSubchannel(subchannel2, READY_STATE); // subchannel2 expires SHUTDOWN_TIMEOUT_MS after being returned clock.forwardTime(SHUTDOWN_TIMEOUT_MS - 1, MILLISECONDS); @@ -170,12 +191,12 @@ public void subchannelReused() { verify(subchannel2).shutdown(); // pool will create a new channel for EAG2 when requested - Subchannel subchannel2a = pool.takeOrCreateSubchannel(EAG2, ATTRS2, mockListener); + Subchannel subchannel2a = pool.takeOrCreateSubchannel(EAG2, ATTRS2); assertThat(subchannel2a).isNotSameInstanceAs(subchannel2); - verify(helper, times(2)).createSubchannel(argsWith(EAG2, "2")); + verify(helper, times(2)).createSubchannel(eq(Arrays.asList(EAG2)), same(ATTRS2)); // subchannel1 expires SHUTDOWN_TIMEOUT_MS after being returned - pool.returnSubchannel(subchannel1a); + pool.returnSubchannel(subchannel1a, READY_STATE); clock.forwardTime(SHUTDOWN_TIMEOUT_MS - 1, MILLISECONDS); verify(subchannel1a, never()).shutdown(); clock.forwardTime(1, MILLISECONDS); @@ -186,138 +207,93 @@ public void subchannelReused() { @Test public void updateStateWhileInPool() { - Subchannel subchannel1 = pool.takeOrCreateSubchannel(EAG1, ATTRS1, mockListener); - Subchannel subchannel2 = pool.takeOrCreateSubchannel(EAG2, ATTRS2, mockListener); - - // Simulate state updates while they are in the pool - stateListeners.get(subchannel1).onSubchannelState(subchannel1, TRANSIENT_FAILURE_STATE); - stateListeners.get(subchannel2).onSubchannelState(subchannel1, TRANSIENT_FAILURE_STATE); - - verify(mockListener).onSubchannelState(same(subchannel1), same(TRANSIENT_FAILURE_STATE)); - verify(mockListener).onSubchannelState(same(subchannel2), same(TRANSIENT_FAILURE_STATE)); - - pool.returnSubchannel(subchannel1); - pool.returnSubchannel(subchannel2); + Subchannel subchannel1 = pool.takeOrCreateSubchannel(EAG1, ATTRS1); + Subchannel subchannel2 = pool.takeOrCreateSubchannel(EAG2, ATTRS2); + pool.returnSubchannel(subchannel1, READY_STATE); + pool.returnSubchannel(subchannel2, TRANSIENT_FAILURE_STATE); ConnectivityStateInfo anotherFailureState = ConnectivityStateInfo.forTransientFailure(Status.UNAVAILABLE.withDescription("Another")); - // Simulate a subchannel state update while it's in the pool - stateListeners.get(subchannel1).onSubchannelState(subchannel1, anotherFailureState); + pool.handleSubchannelState(subchannel1, anotherFailureState); - SubchannelStateListener mockListener1 = mock(SubchannelStateListener.class); - SubchannelStateListener mockListener2 = mock(SubchannelStateListener.class); + verify(balancer, never()) + .handleSubchannelState(any(Subchannel.class), any(ConnectivityStateInfo.class)); - // Saved state is populated to new mockListeners - assertThat(pool.takeOrCreateSubchannel(EAG1, ATTRS1, mockListener1)) - .isSameInstanceAs(subchannel1); - verify(mockListener1).onSubchannelState(same(subchannel1), same(anotherFailureState)); - verifyNoMoreInteractions(mockListener1); + assertThat(pool.takeOrCreateSubchannel(EAG1, ATTRS1)).isSameInstanceAs(subchannel1); + verify(balancer).handleSubchannelState(same(subchannel1), same(anotherFailureState)); + verifyNoMoreInteractions(balancer); - assertThat(pool.takeOrCreateSubchannel(EAG2, ATTRS2, mockListener2)) - .isSameInstanceAs(subchannel2); - verify(mockListener2).onSubchannelState(same(subchannel2), same(TRANSIENT_FAILURE_STATE)); - verifyNoMoreInteractions(mockListener2); - - // The old mockListener doesn't receive more updates - verifyNoMoreInteractions(mockListener); + assertThat(pool.takeOrCreateSubchannel(EAG2, ATTRS2)).isSameInstanceAs(subchannel2); + verify(balancer).handleSubchannelState(same(subchannel2), same(TRANSIENT_FAILURE_STATE)); + verifyNoMoreInteractions(balancer); } @Test - public void takeTwice_willThrow() { - Subchannel unused = pool.takeOrCreateSubchannel(EAG1, ATTRS1, mockListener); - try { - pool.takeOrCreateSubchannel(EAG1, ATTRS1, mockListener); - fail("Should throw"); - } catch (IllegalStateException e) { - assertThat(e).hasMessageThat().contains("Already out of pool"); - } - } + public void updateStateWhileInPool_notSameObject() { + Subchannel subchannel1 = pool.takeOrCreateSubchannel(EAG1, ATTRS1); + pool.returnSubchannel(subchannel1, READY_STATE); - @Test - public void returnTwice_willThrow() { - Subchannel subchannel1 = pool.takeOrCreateSubchannel(EAG1, ATTRS1, mockListener); - pool.returnSubchannel(subchannel1); - try { - pool.returnSubchannel(subchannel1); - fail("Should throw"); - } catch (IllegalStateException e) { - assertThat(e).hasMessageThat().contains("Already in pool"); - } - } + Subchannel subchannel2 = helper.createSubchannel(EAG1, ATTRS1); + Subchannel subchannel3 = helper.createSubchannel(EAG2, ATTRS2); - @Test - public void returnNonPoolSubchannelWillThrow_noSuchAddress() { - Subchannel subchannel1 = helper.createSubchannel( - CreateSubchannelArgs.newBuilder() - .setAddresses(EAG1).setStateListener(mockListener) - .build()); - try { - pool.returnSubchannel(subchannel1); - fail("Should throw"); - } catch (IllegalArgumentException e) { - assertThat(e).hasMessageThat().contains("not found"); - } + // subchannel2 is not in the pool, although with the same address + pool.handleSubchannelState(subchannel2, TRANSIENT_FAILURE_STATE); + + // subchannel3 is not in the pool. In fact its address is not in the pool + pool.handleSubchannelState(subchannel3, TRANSIENT_FAILURE_STATE); + + assertThat(pool.takeOrCreateSubchannel(EAG1, ATTRS1)).isSameInstanceAs(subchannel1); + + // subchannel1's state is unchanged + verify(balancer).handleSubchannelState(same(subchannel1), same(READY_STATE)); + verifyNoMoreInteractions(balancer); } @Test - public void returnNonPoolSubchannelWillThrow_unmatchedSubchannel() { - Subchannel subchannel1 = helper.createSubchannel( - CreateSubchannelArgs.newBuilder() - .setAddresses(EAG1).setStateListener(mockListener) - .build()); - Subchannel subchannel1c = pool.takeOrCreateSubchannel(EAG1, ATTRS1, mockListener); - assertThat(subchannel1).isNotSameInstanceAs(subchannel1c); - try { - pool.returnSubchannel(subchannel1); - fail("Should throw"); - } catch (IllegalArgumentException e) { - assertThat(e).hasMessageThat().contains("doesn't match the cache"); - } + public void returnDuplicateAddressSubchannel() { + Subchannel subchannel1 = pool.takeOrCreateSubchannel(EAG1, ATTRS1); + Subchannel subchannel2 = pool.takeOrCreateSubchannel(EAG1, ATTRS2); + Subchannel subchannel3 = pool.takeOrCreateSubchannel(EAG2, ATTRS1); + assertThat(subchannel1).isNotSameInstanceAs(subchannel2); + + assertThat(clock.getPendingTasks(SHUTDOWN_TASK_FILTER)).isEmpty(); + pool.returnSubchannel(subchannel2, READY_STATE); + assertThat(clock.getPendingTasks(SHUTDOWN_TASK_FILTER)).hasSize(1); + + // If the subchannel being returned has an address that is the same as a subchannel in the pool, + // the returned subchannel will be shut down. + verify(subchannel1, never()).shutdown(); + pool.returnSubchannel(subchannel1, READY_STATE); + assertThat(clock.getPendingTasks(SHUTDOWN_TASK_FILTER)).hasSize(1); + verify(subchannel1).shutdown(); + + pool.returnSubchannel(subchannel3, READY_STATE); + assertThat(clock.getPendingTasks(SHUTDOWN_TASK_FILTER)).hasSize(2); + // Returning the same subchannel twice has no effect. + pool.returnSubchannel(subchannel3, READY_STATE); + assertThat(clock.getPendingTasks(SHUTDOWN_TASK_FILTER)).hasSize(2); + + verify(subchannel2, never()).shutdown(); + verify(subchannel3, never()).shutdown(); } @Test public void clear() { - Subchannel subchannel1 = pool.takeOrCreateSubchannel(EAG1, ATTRS1, mockListener); - Subchannel subchannel2 = pool.takeOrCreateSubchannel(EAG2, ATTRS2, mockListener); + Subchannel subchannel1 = pool.takeOrCreateSubchannel(EAG1, ATTRS1); + Subchannel subchannel2 = pool.takeOrCreateSubchannel(EAG2, ATTRS2); + Subchannel subchannel3 = pool.takeOrCreateSubchannel(EAG2, ATTRS2); - pool.returnSubchannel(subchannel1); + pool.returnSubchannel(subchannel1, READY_STATE); + pool.returnSubchannel(subchannel2, READY_STATE); verify(subchannel1, never()).shutdown(); verify(subchannel2, never()).shutdown(); pool.clear(); verify(subchannel1).shutdown(); verify(subchannel2).shutdown(); - assertThat(clock.numPendingTasks()).isEqualTo(0); - } - private CreateSubchannelArgs argsWith( - final EquivalentAddressGroup expectedEag, final Object expectedValue) { - return MockitoHamcrest.argThat( - new org.hamcrest.BaseMatcher() { - @Override - public boolean matches(Object item) { - if (!(item instanceof CreateSubchannelArgs)) { - return false; - } - CreateSubchannelArgs that = (CreateSubchannelArgs) item; - List expectedEagList = Collections.singletonList(expectedEag); - if (!expectedEagList.equals(that.getAddresses())) { - return false; - } - if (!expectedValue.equals(that.getAttributes().get(ATTR_KEY))) { - return false; - } - return true; - } - - @Override - public void describeTo(org.hamcrest.Description desc) { - desc.appendText( - "Matches Attributes that includes " + expectedEag + " and " - + ATTR_KEY + "=" + expectedValue); - } - }); + verify(subchannel3, never()).shutdown(); + assertThat(clock.numPendingTasks()).isEqualTo(0); } - } diff --git a/grpclb/src/test/java/io/grpc/grpclb/GrpclbLoadBalancerTest.java b/grpclb/src/test/java/io/grpc/grpclb/GrpclbLoadBalancerTest.java index a995e735d0a..7db539cc2a6 100644 --- a/grpclb/src/test/java/io/grpc/grpclb/GrpclbLoadBalancerTest.java +++ b/grpclb/src/test/java/io/grpc/grpclb/GrpclbLoadBalancerTest.java @@ -20,6 +20,7 @@ import static io.grpc.ConnectivityState.CONNECTING; import static io.grpc.ConnectivityState.IDLE; import static io.grpc.ConnectivityState.READY; +import static io.grpc.ConnectivityState.SHUTDOWN; import static io.grpc.ConnectivityState.TRANSIENT_FAILURE; import static io.grpc.grpclb.GrpclbLoadBalancer.retrieveModeFromLbConfig; import static io.grpc.grpclb.GrpclbState.BUFFER_ENTRY; @@ -55,14 +56,12 @@ import io.grpc.ConnectivityStateInfo; import io.grpc.EquivalentAddressGroup; import io.grpc.LoadBalancer; -import io.grpc.LoadBalancer.CreateSubchannelArgs; import io.grpc.LoadBalancer.Helper; import io.grpc.LoadBalancer.PickResult; import io.grpc.LoadBalancer.PickSubchannelArgs; import io.grpc.LoadBalancer.ResolvedAddresses; import io.grpc.LoadBalancer.Subchannel; import io.grpc.LoadBalancer.SubchannelPicker; -import io.grpc.LoadBalancer.SubchannelStateListener; import io.grpc.ManagedChannel; import io.grpc.Metadata; import io.grpc.Status; @@ -96,7 +95,6 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; -import java.util.HashMap; import java.util.LinkedList; import java.util.List; import java.util.Map; @@ -166,13 +164,10 @@ public void log(ChannelLogLevel level, String template, Object... args) { private LoadBalancerGrpc.LoadBalancerImplBase mockLbService; @Captor private ArgumentCaptor> lbResponseObserverCaptor; - @Captor - private ArgumentCaptor createArgsCaptor; private final FakeClock fakeClock = new FakeClock(); private final LinkedList> lbRequestObservers = new LinkedList<>(); private final LinkedList mockSubchannels = new LinkedList<>(); - private final Map subchannelStateListeners = new HashMap<>(); private final LinkedList fakeOobChannels = new LinkedList<>(); private final ArrayList pooledSubchannelTracker = new ArrayList<>(); private final ArrayList unpooledSubchannelTracker = new ArrayList<>(); @@ -251,26 +246,24 @@ public Subchannel answer(InvocationOnMock invocation) throws Throwable { when(subchannel.getAttributes()).thenReturn(attrs); mockSubchannels.add(subchannel); pooledSubchannelTracker.add(subchannel); - subchannelStateListeners.put( - subchannel, (SubchannelStateListener) invocation.getArguments()[2]); return subchannel; } }).when(subchannelPool).takeOrCreateSubchannel( - any(EquivalentAddressGroup.class), any(Attributes.class), - any(SubchannelStateListener.class)); + any(EquivalentAddressGroup.class), any(Attributes.class)); doAnswer(new Answer() { @Override public Subchannel answer(InvocationOnMock invocation) throws Throwable { Subchannel subchannel = mock(Subchannel.class); - CreateSubchannelArgs args = (CreateSubchannelArgs) invocation.getArguments()[0]; - when(subchannel.getAllAddresses()).thenReturn(args.getAddresses()); - when(subchannel.getAttributes()).thenReturn(args.getAttributes()); + List eagList = + (List) invocation.getArguments()[0]; + Attributes attrs = (Attributes) invocation.getArguments()[1]; + when(subchannel.getAllAddresses()).thenReturn(eagList); + when(subchannel.getAttributes()).thenReturn(attrs); mockSubchannels.add(subchannel); unpooledSubchannelTracker.add(subchannel); - subchannelStateListeners.put(subchannel, args.getStateListener()); return subchannel; } - }).when(helper).createSubchannel(any(CreateSubchannelArgs.class)); + }).when(helper).createSubchannel(any(List.class), any(Attributes.class)); when(helper.getSynchronizationContext()).thenReturn(syncContext); when(helper.getScheduledExecutorService()).thenReturn(fakeClock.getScheduledExecutorService()); when(helper.getChannelLogger()).thenReturn(channelLogger); @@ -289,7 +282,7 @@ public Void answer(InvocationOnMock invocation) throws Throwable { balancer = new GrpclbLoadBalancer(helper, subchannelPool, fakeClock.getTimeProvider(), fakeClock.getStopwatchSupplier().get(), backoffPolicyProvider); - verify(subchannelPool).init(same(helper)); + verify(subchannelPool).init(same(helper), same(balancer)); } @After @@ -311,7 +304,7 @@ public void run() { } // GRPCLB manages subchannels only through subchannelPool for (Subchannel subchannel : pooledSubchannelTracker) { - verify(subchannelPool).returnSubchannel(same(subchannel)); + verify(subchannelPool).returnSubchannel(same(subchannel), any(ConnectivityStateInfo.class)); // Our mock subchannelPool never calls Subchannel.shutdown(), thus we can tell if // LoadBalancer has called it expectedly. verify(subchannel, never()).shutdown(); @@ -686,8 +679,7 @@ public void loadReporting() { // Same backends, thus no new subchannels helperInOrder.verify(subchannelPool, never()).takeOrCreateSubchannel( - any(EquivalentAddressGroup.class), any(Attributes.class), - any(SubchannelStateListener.class)); + any(EquivalentAddressGroup.class), any(Attributes.class)); // But the new RoundRobinEntries have a new loadRecorder, thus considered different from // the previous list, thus a new picker is created helperInOrder.verify(helper).updateBalancingState(eq(READY), pickerCaptor.capture()); @@ -868,12 +860,10 @@ public void grpclbThenNameResolutionFails() { inOrder.verify(subchannelPool).takeOrCreateSubchannel( eq(new EquivalentAddressGroup(backends.get(0).addr, LB_BACKEND_ATTRS)), - any(Attributes.class), - any(SubchannelStateListener.class)); + any(Attributes.class)); inOrder.verify(subchannelPool).takeOrCreateSubchannel( eq(new EquivalentAddressGroup(backends.get(1).addr, LB_BACKEND_ATTRS)), - any(Attributes.class), - any(SubchannelStateListener.class)); + any(Attributes.class)); } @Test @@ -956,12 +946,10 @@ public void grpclbWorking() { inOrder.verify(subchannelPool).takeOrCreateSubchannel( eq(new EquivalentAddressGroup(backends1.get(0).addr, LB_BACKEND_ATTRS)), - any(Attributes.class), - any(SubchannelStateListener.class)); + any(Attributes.class)); inOrder.verify(subchannelPool).takeOrCreateSubchannel( eq(new EquivalentAddressGroup(backends1.get(1).addr, LB_BACKEND_ATTRS)), - any(Attributes.class), - any(SubchannelStateListener.class)); + any(Attributes.class)); assertEquals(2, mockSubchannels.size()); Subchannel subchannel1 = mockSubchannels.poll(); Subchannel subchannel2 = mockSubchannels.poll(); @@ -1069,7 +1057,8 @@ public void grpclbWorking() { new ServerEntry("127.0.0.1", 2010, "token0004"), // Existing address with token changed new ServerEntry("127.0.0.1", 2030, "token0005"), // New address appearing second time new ServerEntry("token0006")); // drop - verify(subchannelPool, never()).returnSubchannel(same(subchannel1)); + verify(subchannelPool, never()) + .returnSubchannel(same(subchannel1), any(ConnectivityStateInfo.class)); lbResponseObserver.onNext(buildLbResponse(backends2)); assertThat(logs).containsExactly( @@ -1085,23 +1074,23 @@ public void grpclbWorking() { logs.clear(); // not in backends2, closed - verify(subchannelPool).returnSubchannel(same(subchannel1)); + verify(subchannelPool).returnSubchannel(same(subchannel1), same(errorState1)); // backends2[2], will be kept - verify(subchannelPool, never()).returnSubchannel(same(subchannel2)); + verify(subchannelPool, never()) + .returnSubchannel(same(subchannel2), any(ConnectivityStateInfo.class)); inOrder.verify(subchannelPool, never()).takeOrCreateSubchannel( eq(new EquivalentAddressGroup(backends2.get(2).addr, LB_BACKEND_ATTRS)), - any(Attributes.class), - any(SubchannelStateListener.class)); + any(Attributes.class)); inOrder.verify(subchannelPool).takeOrCreateSubchannel( eq(new EquivalentAddressGroup(backends2.get(0).addr, LB_BACKEND_ATTRS)), - any(Attributes.class), - any(SubchannelStateListener.class)); + any(Attributes.class)); ConnectivityStateInfo errorOnCachedSubchannel1 = ConnectivityStateInfo.forTransientFailure( Status.UNAVAILABLE.withDescription("You can get this error even if you are cached")); deliverSubchannelState(subchannel1, errorOnCachedSubchannel1); + verify(subchannelPool).handleSubchannelState(same(subchannel1), same(errorOnCachedSubchannel1)); assertEquals(1, mockSubchannels.size()); Subchannel subchannel3 = mockSubchannels.poll(); @@ -1119,6 +1108,17 @@ public void grpclbWorking() { new DropEntry(getLoadRecorder(), "token0006")).inOrder(); assertThat(picker7.pickList).containsExactly(BUFFER_ENTRY); + // State updates on obsolete subchannel1 will only be passed to the pool + deliverSubchannelState(subchannel1, ConnectivityStateInfo.forNonError(READY)); + deliverSubchannelState( + subchannel1, ConnectivityStateInfo.forTransientFailure(Status.UNAVAILABLE)); + deliverSubchannelState(subchannel1, ConnectivityStateInfo.forNonError(SHUTDOWN)); + inOrder.verify(subchannelPool) + .handleSubchannelState(same(subchannel1), eq(ConnectivityStateInfo.forNonError(READY))); + inOrder.verify(subchannelPool).handleSubchannelState( + same(subchannel1), eq(ConnectivityStateInfo.forTransientFailure(Status.UNAVAILABLE))); + inOrder.verifyNoMoreInteractions(); + deliverSubchannelState(subchannel3, ConnectivityStateInfo.forNonError(READY)); inOrder.verify(helper).updateBalancingState(eq(READY), pickerCaptor.capture()); RoundRobinPicker picker8 = (RoundRobinPicker) pickerCaptor.getValue(); @@ -1146,12 +1146,15 @@ public void grpclbWorking() { new BackendEntry(subchannel3, getLoadRecorder(), "token0003"), new BackendEntry(subchannel2, getLoadRecorder(), "token0004"), new BackendEntry(subchannel3, getLoadRecorder(), "token0005")).inOrder(); - verify(subchannelPool, never()).returnSubchannel(same(subchannel3)); + verify(subchannelPool, never()) + .returnSubchannel(same(subchannel3), any(ConnectivityStateInfo.class)); // Update backends, with no entry lbResponseObserver.onNext(buildLbResponse(Collections.emptyList())); - verify(subchannelPool).returnSubchannel(same(subchannel2)); - verify(subchannelPool).returnSubchannel(same(subchannel3)); + verify(subchannelPool) + .returnSubchannel(same(subchannel2), eq(ConnectivityStateInfo.forNonError(READY))); + verify(subchannelPool) + .returnSubchannel(same(subchannel3), eq(ConnectivityStateInfo.forNonError(READY))); inOrder.verify(helper).updateBalancingState(eq(CONNECTING), pickerCaptor.capture()); RoundRobinPicker picker10 = (RoundRobinPicker) pickerCaptor.getValue(); assertThat(picker10.dropList).isEmpty(); @@ -1497,9 +1500,9 @@ private void subtestGrpclbFallbackConnectionLost( if (!(balancerBroken && allSubchannelsBroken)) { verify(subchannelPool, never()).takeOrCreateSubchannel( - eq(resolutionList.get(0)), any(Attributes.class), any(SubchannelStateListener.class)); + eq(resolutionList.get(0)), any(Attributes.class)); verify(subchannelPool, never()).takeOrCreateSubchannel( - eq(resolutionList.get(2)), any(Attributes.class), any(SubchannelStateListener.class)); + eq(resolutionList.get(2)), any(Attributes.class)); } } @@ -1526,8 +1529,7 @@ private List fallbackTestVerifyUseOfBackendLists( assertEquals(addrs.size(), tokens.size()); } for (EquivalentAddressGroup addr : addrs) { - inOrder.verify(subchannelPool).takeOrCreateSubchannel( - eq(addr), any(Attributes.class), any(SubchannelStateListener.class)); + inOrder.verify(subchannelPool).takeOrCreateSubchannel(eq(addr), any(Attributes.class)); } RoundRobinPicker picker = (RoundRobinPicker) currentPicker; assertThat(picker.dropList).containsExactlyElementsIn(Collections.nCopies(addrs.size(), null)); @@ -1730,11 +1732,11 @@ public void grpclbWorking_pickFirstMode() throws Exception { lbResponseObserver.onNext(buildInitialResponse()); lbResponseObserver.onNext(buildLbResponse(backends1)); - inOrder.verify(helper).createSubchannel(createArgsCaptor.capture()); - assertThat(createArgsCaptor.getValue().getAddresses()).containsExactly( - new EquivalentAddressGroup(backends1.get(0).addr, eagAttrsWithToken("token0001")), - new EquivalentAddressGroup(backends1.get(1).addr, eagAttrsWithToken("token0002"))) - .inOrder(); + inOrder.verify(helper).createSubchannel( + eq(Arrays.asList( + new EquivalentAddressGroup(backends1.get(0).addr, eagAttrsWithToken("token0001")), + new EquivalentAddressGroup(backends1.get(1).addr, eagAttrsWithToken("token0002")))), + any(Attributes.class)); // Initially IDLE inOrder.verify(helper).updateBalancingState(eq(IDLE), pickerCaptor.capture()); @@ -1785,7 +1787,7 @@ public void grpclbWorking_pickFirstMode() throws Exception { // new addresses will be updated to the existing subchannel // createSubchannel() has ever been called only once - verify(helper, times(1)).createSubchannel(any(CreateSubchannelArgs.class)); + verify(helper, times(1)).createSubchannel(any(List.class), any(Attributes.class)); assertThat(mockSubchannels).isEmpty(); inOrder.verify(helper).updateSubchannelAddresses( same(subchannel), @@ -1817,10 +1819,10 @@ public void grpclbWorking_pickFirstMode() throws Exception { verify(subchannel, times(2)).requestConnection(); // PICK_FIRST doesn't use subchannelPool - verify(subchannelPool, never()).takeOrCreateSubchannel( - any(EquivalentAddressGroup.class), any(Attributes.class), - any(SubchannelStateListener.class)); - verify(subchannelPool, never()).returnSubchannel(any(Subchannel.class)); + verify(subchannelPool, never()) + .takeOrCreateSubchannel(any(EquivalentAddressGroup.class), any(Attributes.class)); + verify(subchannelPool, never()) + .returnSubchannel(any(Subchannel.class), any(ConnectivityStateInfo.class)); } @SuppressWarnings("unchecked") @@ -1847,9 +1849,9 @@ public void pickFirstMode_fallback() throws Exception { fakeClock.forwardTime(GrpclbState.FALLBACK_TIMEOUT_MS, TimeUnit.MILLISECONDS); // Entering fallback mode - inOrder.verify(helper).createSubchannel(createArgsCaptor.capture()); - assertThat(createArgsCaptor.getValue().getAddresses()).containsExactly( - grpclbResolutionList.get(0), grpclbResolutionList.get(2)).inOrder(); + inOrder.verify(helper).createSubchannel( + eq(Arrays.asList(grpclbResolutionList.get(0), grpclbResolutionList.get(2))), + any(Attributes.class)); assertThat(mockSubchannels).hasSize(1); Subchannel subchannel = mockSubchannels.poll(); @@ -1881,7 +1883,7 @@ public void pickFirstMode_fallback() throws Exception { // new addresses will be updated to the existing subchannel // createSubchannel() has ever been called only once - verify(helper, times(1)).createSubchannel(any(CreateSubchannelArgs.class)); + verify(helper, times(1)).createSubchannel(any(List.class), any(Attributes.class)); assertThat(mockSubchannels).isEmpty(); inOrder.verify(helper).updateSubchannelAddresses( same(subchannel), @@ -1896,10 +1898,10 @@ public void pickFirstMode_fallback() throws Exception { new BackendEntry(subchannel, new TokenAttachingTracerFactory(getLoadRecorder()))); // PICK_FIRST doesn't use subchannelPool - verify(subchannelPool, never()).takeOrCreateSubchannel( - any(EquivalentAddressGroup.class), any(Attributes.class), - any(SubchannelStateListener.class)); - verify(subchannelPool, never()).returnSubchannel(any(Subchannel.class)); + verify(subchannelPool, never()) + .takeOrCreateSubchannel(any(EquivalentAddressGroup.class), any(Attributes.class)); + verify(subchannelPool, never()) + .returnSubchannel(any(Subchannel.class), any(ConnectivityStateInfo.class)); } @Test @@ -1936,15 +1938,16 @@ public void switchMode() throws Exception { // ROUND_ROBIN: create one subchannel per server verify(subchannelPool).takeOrCreateSubchannel( eq(new EquivalentAddressGroup(backends1.get(0).addr, LB_BACKEND_ATTRS)), - any(Attributes.class), any(SubchannelStateListener.class)); + any(Attributes.class)); verify(subchannelPool).takeOrCreateSubchannel( eq(new EquivalentAddressGroup(backends1.get(1).addr, LB_BACKEND_ATTRS)), - any(Attributes.class), any(SubchannelStateListener.class)); + any(Attributes.class)); inOrder.verify(helper).updateBalancingState(eq(CONNECTING), any(SubchannelPicker.class)); assertEquals(2, mockSubchannels.size()); Subchannel subchannel1 = mockSubchannels.poll(); Subchannel subchannel2 = mockSubchannels.poll(); - verify(subchannelPool, never()).returnSubchannel(any(Subchannel.class)); + verify(subchannelPool, never()) + .returnSubchannel(any(Subchannel.class), any(ConnectivityStateInfo.class)); // Switch to PICK_FIRST lbConfig = "{\"childPolicy\" : [ {\"pick_first\" : {}} ]}"; @@ -1955,8 +1958,10 @@ public void switchMode() throws Exception { // GrpclbState will be shutdown, and a new one will be created assertThat(oobChannel.isShutdown()).isTrue(); - verify(subchannelPool).returnSubchannel(same(subchannel1)); - verify(subchannelPool).returnSubchannel(same(subchannel2)); + verify(subchannelPool) + .returnSubchannel(same(subchannel1), eq(ConnectivityStateInfo.forNonError(IDLE))); + verify(subchannelPool) + .returnSubchannel(same(subchannel2), eq(ConnectivityStateInfo.forNonError(IDLE))); // A new LB stream is created assertEquals(1, fakeOobChannels.size()); @@ -1976,11 +1981,11 @@ public void switchMode() throws Exception { lbResponseObserver.onNext(buildLbResponse(backends1)); // PICK_FIRST Subchannel - inOrder.verify(helper).createSubchannel(createArgsCaptor.capture()); - assertThat(createArgsCaptor.getValue().getAddresses()).containsExactly( - new EquivalentAddressGroup(backends1.get(0).addr, eagAttrsWithToken("token0001")), - new EquivalentAddressGroup(backends1.get(1).addr, eagAttrsWithToken("token0002"))) - .inOrder(); + inOrder.verify(helper).createSubchannel( + eq(Arrays.asList( + new EquivalentAddressGroup(backends1.get(0).addr, eagAttrsWithToken("token0001")), + new EquivalentAddressGroup(backends1.get(1).addr, eagAttrsWithToken("token0002")))), + any(Attributes.class)); inOrder.verify(helper).updateBalancingState(eq(IDLE), any(SubchannelPicker.class)); } @@ -2065,7 +2070,7 @@ private void deliverSubchannelState( syncContext.execute(new Runnable() { @Override public void run() { - subchannelStateListeners.get(subchannel).onSubchannelState(subchannel, newState); + balancer.handleSubchannelState(subchannel, newState); } }); } diff --git a/services/src/main/java/io/grpc/services/HealthCheckingLoadBalancerFactory.java b/services/src/main/java/io/grpc/services/HealthCheckingLoadBalancerFactory.java index 1d680d73f6b..d199002a563 100644 --- a/services/src/main/java/io/grpc/services/HealthCheckingLoadBalancerFactory.java +++ b/services/src/main/java/io/grpc/services/HealthCheckingLoadBalancerFactory.java @@ -16,7 +16,6 @@ package io.grpc.services; -import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; import static io.grpc.ConnectivityState.CONNECTING; @@ -28,17 +27,17 @@ import com.google.common.base.Objects; import com.google.common.base.Stopwatch; import com.google.common.base.Supplier; +import io.grpc.Attributes; import io.grpc.CallOptions; import io.grpc.ChannelLogger; import io.grpc.ChannelLogger.ChannelLogLevel; import io.grpc.ClientCall; import io.grpc.ConnectivityStateInfo; +import io.grpc.EquivalentAddressGroup; import io.grpc.LoadBalancer; -import io.grpc.LoadBalancer.CreateSubchannelArgs; import io.grpc.LoadBalancer.Factory; import io.grpc.LoadBalancer.Helper; import io.grpc.LoadBalancer.Subchannel; -import io.grpc.LoadBalancer.SubchannelStateListener; import io.grpc.Metadata; import io.grpc.Status; import io.grpc.Status.Code; @@ -54,6 +53,7 @@ import io.grpc.util.ForwardingLoadBalancer; import io.grpc.util.ForwardingLoadBalancerHelper; import java.util.HashSet; +import java.util.List; import java.util.Map; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; @@ -70,6 +70,8 @@ * SynchronizationContext, or it will throw. */ final class HealthCheckingLoadBalancerFactory extends Factory { + private static final Attributes.Key KEY_HEALTH_CHECK_STATE = + Attributes.Key.create("io.grpc.services.HealthCheckingLoadBalancerFactory.healthCheckState"); private static final Logger logger = Logger.getLogger(HealthCheckingLoadBalancerFactory.class.getName()); @@ -89,13 +91,15 @@ public HealthCheckingLoadBalancerFactory( public LoadBalancer newLoadBalancer(Helper helper) { HelperImpl wrappedHelper = new HelperImpl(helper); LoadBalancer delegateBalancer = delegateFactory.newLoadBalancer(wrappedHelper); + wrappedHelper.init(delegateBalancer); return new HealthCheckingLoadBalancer(wrappedHelper, delegateBalancer); } private final class HelperImpl extends ForwardingLoadBalancerHelper { private final Helper delegate; private final SynchronizationContext syncContext; - + + private LoadBalancer delegateBalancer; @Nullable String healthCheckedService; private boolean balancerShutdown; @@ -106,21 +110,26 @@ private final class HelperImpl extends ForwardingLoadBalancerHelper { this.syncContext = checkNotNull(delegate.getSynchronizationContext(), "syncContext"); } + void init(LoadBalancer delegateBalancer) { + checkState(this.delegateBalancer == null, "init() already called"); + this.delegateBalancer = checkNotNull(delegateBalancer, "delegateBalancer"); + } + @Override protected Helper delegate() { return delegate; } @Override - public Subchannel createSubchannel(CreateSubchannelArgs args) { + public Subchannel createSubchannel(List addrs, Attributes attrs) { // HealthCheckState is not thread-safe, we are requiring the original LoadBalancer calls // createSubchannel() from the SynchronizationContext. syncContext.throwIfNotInThisSynchronizationContext(); HealthCheckState hcState = new HealthCheckState( - this, args.getStateListener(), syncContext, delegate.getScheduledExecutorService()); + this, delegateBalancer, syncContext, delegate.getScheduledExecutorService()); hcStates.add(hcState); - Subchannel subchannel = - super.createSubchannel(args.toBuilder().setStateListener(hcState).build()); + Subchannel subchannel = super.createSubchannel( + addrs, attrs.toBuilder().set(KEY_HEALTH_CHECK_STATE, hcState).build()); hcState.init(subchannel); if (healthCheckedService != null) { hcState.setServiceName(healthCheckedService); @@ -168,15 +177,27 @@ public void handleResolvedAddresses(ResolvedAddresses resolvedAddresses) { super.handleResolvedAddresses(resolvedAddresses); } + @Override + public void handleSubchannelState( + Subchannel subchannel, ConnectivityStateInfo stateInfo) { + HealthCheckState hcState = + checkNotNull(subchannel.getAttributes().get(KEY_HEALTH_CHECK_STATE), "hcState"); + hcState.updateRawState(stateInfo); + + if (Objects.equal(stateInfo.getState(), SHUTDOWN)) { + helper.hcStates.remove(hcState); + } + } + @Override public void shutdown() { super.shutdown(); helper.balancerShutdown = true; for (HealthCheckState hcState : helper.hcStates) { - // ManagedChannel will stop calling onSubchannelState() after shutdown() is called, + // ManagedChannel will stop calling handleSubchannelState() after shutdown() is called, // which is required by LoadBalancer API semantics. We need to deliver the final SHUTDOWN // signal to health checkers so that they can cancel the streams. - hcState.onSubchannelState(hcState.subchannel, ConnectivityStateInfo.forNonError(SHUTDOWN)); + hcState.updateRawState(ConnectivityStateInfo.forNonError(SHUTDOWN)); } helper.hcStates.clear(); } @@ -189,7 +210,7 @@ public String toString() { // All methods are run from syncContext - private final class HealthCheckState implements SubchannelStateListener { + private final class HealthCheckState { private final Runnable retryTask = new Runnable() { @Override public void run() { @@ -197,7 +218,7 @@ public void run() { } }; - private final SubchannelStateListener stateListener; + private final LoadBalancer delegate; private final SynchronizationContext syncContext; private final ScheduledExecutorService timerService; private final HelperImpl helperImpl; @@ -225,10 +246,10 @@ public void run() { HealthCheckState( HelperImpl helperImpl, - SubchannelStateListener stateListener, SynchronizationContext syncContext, + LoadBalancer delegate, SynchronizationContext syncContext, ScheduledExecutorService timerService) { this.helperImpl = checkNotNull(helperImpl, "helperImpl"); - this.stateListener = checkNotNull(stateListener, "stateListener"); + this.delegate = checkNotNull(delegate, "delegate"); this.syncContext = checkNotNull(syncContext, "syncContext"); this.timerService = checkNotNull(timerService, "timerService"); } @@ -253,19 +274,13 @@ void setServiceName(@Nullable String newServiceName) { adjustHealthCheck(); } - @Override - public void onSubchannelState(Subchannel subchannel, ConnectivityStateInfo rawState) { - checkArgument(subchannel == this.subchannel, - "Subchannel mismatch: %s vs %s", subchannel, this.subchannel); + void updateRawState(ConnectivityStateInfo rawState) { if (Objects.equal(this.rawState.getState(), READY) && !Objects.equal(rawState.getState(), READY)) { // A connection was lost. We will reset disabled flag because health check // may be available on the new connection. disabled = false; } - if (Objects.equal(rawState.getState(), SHUTDOWN)) { - helperImpl.hcStates.remove(this); - } this.rawState = rawState; adjustHealthCheck(); } @@ -324,7 +339,7 @@ private void gotoState(ConnectivityStateInfo newState) { checkState(subchannel != null, "init() not called"); if (!helperImpl.balancerShutdown && !Objects.equal(concludedState, newState)) { concludedState = newState; - stateListener.onSubchannelState(subchannel, concludedState); + delegate.handleSubchannelState(subchannel, concludedState); } } diff --git a/services/src/test/java/io/grpc/services/HealthCheckingLoadBalancerFactoryTest.java b/services/src/test/java/io/grpc/services/HealthCheckingLoadBalancerFactoryTest.java index 4dbe5c3c449..dbdf9911bd7 100644 --- a/services/src/test/java/io/grpc/services/HealthCheckingLoadBalancerFactoryTest.java +++ b/services/src/test/java/io/grpc/services/HealthCheckingLoadBalancerFactoryTest.java @@ -24,6 +24,7 @@ import static io.grpc.ConnectivityState.READY; import static io.grpc.ConnectivityState.SHUTDOWN; import static io.grpc.ConnectivityState.TRANSIENT_FAILURE; +import static org.junit.Assert.fail; import static org.mockito.AdditionalAnswers.delegatesTo; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; @@ -31,7 +32,6 @@ import static org.mockito.Mockito.atLeast; import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.verifyZeroInteractions; @@ -47,13 +47,11 @@ import io.grpc.Context.CancellationListener; import io.grpc.EquivalentAddressGroup; import io.grpc.LoadBalancer; -import io.grpc.LoadBalancer.CreateSubchannelArgs; import io.grpc.LoadBalancer.Factory; import io.grpc.LoadBalancer.Helper; import io.grpc.LoadBalancer.ResolvedAddresses; import io.grpc.LoadBalancer.Subchannel; import io.grpc.LoadBalancer.SubchannelPicker; -import io.grpc.LoadBalancer.SubchannelStateListener; import io.grpc.ManagedChannel; import io.grpc.NameResolver; import io.grpc.Server; @@ -76,6 +74,7 @@ import java.util.ArrayDeque; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Queue; @@ -109,13 +108,8 @@ public class HealthCheckingLoadBalancerFactoryTest { private final EquivalentAddressGroup[] eags = new EquivalentAddressGroup[NUM_SUBCHANNELS]; @SuppressWarnings({"rawtypes", "unchecked"}) private final List[] eagLists = new List[NUM_SUBCHANNELS]; - private final SubchannelStateListener[] mockStateListeners = - new SubchannelStateListener[NUM_SUBCHANNELS]; private List resolvedAddressList; private final FakeSubchannel[] subchannels = new FakeSubchannel[NUM_SUBCHANNELS]; - // State listeners seen by the real Helper. Use them to simulate raw Subchannel updates. - private final SubchannelStateListener[] stateListeners = - new SubchannelStateListener[NUM_SUBCHANNELS]; private final ManagedChannel[] channels = new ManagedChannel[NUM_SUBCHANNELS]; private final Server[] servers = new Server[NUM_SUBCHANNELS]; private final HealthImpl[] healthImpls = new HealthImpl[NUM_SUBCHANNELS]; @@ -145,7 +139,7 @@ public LoadBalancer newLoadBalancer(Helper helper) { private LoadBalancer origLb; private LoadBalancer hcLb; @Captor - ArgumentCaptor createArgsCaptor; + ArgumentCaptor attrsCaptor; @Mock private BackoffPolicy.Provider backoffPolicyProvider; @Mock @@ -177,7 +171,6 @@ public void setup() throws Exception { eags[i] = eag; List eagList = Arrays.asList(eag); eagLists[i] = eagList; - mockStateListeners[i] = mock(SubchannelStateListener.class); } resolvedAddressList = Arrays.asList(eags); @@ -206,6 +199,19 @@ public void run() { }); } + @Override + public void handleSubchannelState( + final Subchannel subchannel, final ConnectivityStateInfo stateInfo) { + syncContext.execute(new Runnable() { + @Override + public void run() { + if (!shutdown) { + hcLb.handleSubchannelState(subchannel, stateInfo); + } + } + }); + } + @Override public void handleNameResolutionError(Status error) { throw new AssertionError("Not supposed to be called"); @@ -231,7 +237,7 @@ public void run() { public void teardown() throws Exception { // All scheduled tasks have been accounted for assertThat(clock.getPendingTasks()).isEmpty(); - // Health-check streams are usually not closed in the tests because onSubchannelState() is + // Health-check streams are usually not closed in the tests because handleSubchannelState() is // faked. Force closing for clean up. for (Server server : servers) { server.shutdownNow(); @@ -246,6 +252,16 @@ public void teardown() throws Exception { } } + @Test + public void createSubchannelThrowsIfCalledOutsideSynchronizationContext() { + try { + wrappedHelper.createSubchannel(eagLists[0], Attributes.EMPTY); + fail("Should throw"); + } catch (IllegalStateException e) { + assertThat(e.getMessage()).isEqualTo("Not called from the SynchronizationContext"); + } + } + @Test public void typicalWorkflow() { Attributes resolutionAttrs = attrsWithHealthCheckService("FooService"); @@ -269,41 +285,41 @@ public void typicalWorkflow() { .set(SUBCHANNEL_ATTR_KEY, subchannelAttrValue).build(); // We don't wrap Subchannels, thus origLb gets the original Subchannels. assertThat(createSubchannel(i, attrs)).isSameInstanceAs(subchannels[i]); - verify(origHelper, times(i + 1)).createSubchannel(createArgsCaptor.capture()); - assertThat(createArgsCaptor.getValue().getAddresses()).isEqualTo(eagLists[i]); - assertThat(createArgsCaptor.getValue().getAttributes().get(SUBCHANNEL_ATTR_KEY)) - .isEqualTo(subchannelAttrValue); + verify(origHelper).createSubchannel(same(eagLists[i]), attrsCaptor.capture()); + assertThat(attrsCaptor.getValue().get(SUBCHANNEL_ATTR_KEY)).isEqualTo(subchannelAttrValue); } for (int i = NUM_SUBCHANNELS - 1; i >= 0; i--) { // Not starting health check until underlying Subchannel is READY FakeSubchannel subchannel = subchannels[i]; HealthImpl healthImpl = healthImpls[i]; - SubchannelStateListener mockStateListener = mockStateListeners[i]; - InOrder inOrder = inOrder(mockStateListener); - deliverSubchannelState(i, ConnectivityStateInfo.forNonError(CONNECTING)); - deliverSubchannelState(i, ConnectivityStateInfo.forTransientFailure(Status.UNAVAILABLE)); - deliverSubchannelState(i, ConnectivityStateInfo.forNonError(IDLE)); - - inOrder.verify(mockStateListener).onSubchannelState( + InOrder inOrder = inOrder(origLb); + hcLbEventDelivery.handleSubchannelState( + subchannel, ConnectivityStateInfo.forNonError(CONNECTING)); + hcLbEventDelivery.handleSubchannelState( + subchannel, ConnectivityStateInfo.forTransientFailure(Status.UNAVAILABLE)); + hcLbEventDelivery.handleSubchannelState( + subchannel, ConnectivityStateInfo.forNonError(IDLE)); + + inOrder.verify(origLb).handleSubchannelState( same(subchannel), eq(ConnectivityStateInfo.forNonError(CONNECTING))); - inOrder.verify(mockStateListener).onSubchannelState( + inOrder.verify(origLb).handleSubchannelState( same(subchannel), eq(ConnectivityStateInfo.forTransientFailure(Status.UNAVAILABLE))); - inOrder.verify(mockStateListener).onSubchannelState( + inOrder.verify(origLb).handleSubchannelState( same(subchannel), eq(ConnectivityStateInfo.forNonError(IDLE))); - verifyNoMoreInteractions(mockStateListener); + verifyNoMoreInteractions(origLb); assertThat(subchannel.logs).isEmpty(); assertThat(healthImpl.calls).isEmpty(); - deliverSubchannelState(i, ConnectivityStateInfo.forNonError(READY)); + hcLbEventDelivery.handleSubchannelState(subchannel, ConnectivityStateInfo.forNonError(READY)); assertThat(healthImpl.calls).hasSize(1); ServerSideCall serverCall = healthImpl.calls.peek(); assertThat(serverCall.request).isEqualTo(makeRequest("FooService")); // Starting the health check will make the Subchannel appear CONNECTING to the origLb. - inOrder.verify(mockStateListener).onSubchannelState( + inOrder.verify(origLb).handleSubchannelState( same(subchannel), eq(ConnectivityStateInfo.forNonError(CONNECTING))); - verifyNoMoreInteractions(mockStateListener); + verifyNoMoreInteractions(origLb); assertThat(subchannel.logs).containsExactly( "INFO: CONNECTING: Starting health-check for \"FooService\""); @@ -317,36 +333,35 @@ public void typicalWorkflow() { serverCall.responseObserver.onNext(makeResponse(servingStatus)); // SERVING is mapped to READY, while other statuses are mapped to TRANSIENT_FAILURE if (servingStatus == ServingStatus.SERVING) { - inOrder.verify(mockStateListener).onSubchannelState( + inOrder.verify(origLb).handleSubchannelState( same(subchannel), eq(ConnectivityStateInfo.forNonError(READY))); assertThat(subchannel.logs).containsExactly( "INFO: READY: health-check responded SERVING"); } else { - inOrder.verify(mockStateListener).onSubchannelState( + inOrder.verify(origLb).handleSubchannelState( same(subchannel),unavailableStateWithMsg( "Health-check service responded " + servingStatus + " for 'FooService'")); assertThat(subchannel.logs).containsExactly( "INFO: TRANSIENT_FAILURE: health-check responded " + servingStatus); } subchannel.logs.clear(); - verifyNoMoreInteractions(mockStateListener); + verifyNoMoreInteractions(origLb); } } // origLb shuts down Subchannels for (int i = 0; i < NUM_SUBCHANNELS; i++) { FakeSubchannel subchannel = subchannels[i]; - SubchannelStateListener mockStateListener = mockStateListeners[i]; ServerSideCall serverCall = healthImpls[i].calls.peek(); assertThat(serverCall.cancelled).isFalse(); - verifyNoMoreInteractions(mockStateListener); + verifyNoMoreInteractions(origLb); // Subchannel enters SHUTDOWN state as a response to shutdown(), and that will cancel the // health check RPC subchannel.shutdown(); assertThat(serverCall.cancelled).isTrue(); - verify(mockStateListener).onSubchannelState( + verify(origLb).handleSubchannelState( same(subchannel), eq(ConnectivityStateInfo.forNonError(SHUTDOWN))); assertThat(subchannel.logs).isEmpty(); } @@ -375,12 +390,13 @@ public void healthCheckDisabledWhenServiceNotImplemented() { createSubchannel(i, Attributes.EMPTY); } - InOrder inOrder = inOrder(mockStateListeners[0], mockStateListeners[1]); + InOrder inOrder = inOrder(origLb); for (int i = 0; i < 2; i++) { - deliverSubchannelState(i, ConnectivityStateInfo.forNonError(READY)); + hcLbEventDelivery.handleSubchannelState( + subchannels[i], ConnectivityStateInfo.forNonError(READY)); assertThat(healthImpls[i].calls).hasSize(1); - inOrder.verify(mockStateListeners[i]).onSubchannelState( + inOrder.verify(origLb).handleSubchannelState( same(subchannels[i]), eq(ConnectivityStateInfo.forNonError(CONNECTING))); } @@ -393,7 +409,7 @@ public void healthCheckDisabledWhenServiceNotImplemented() { // In reality UNIMPLEMENTED is generated by GRPC server library, but the client can't tell // whether it's the server library or the service implementation that returned this status. serverCall0.responseObserver.onError(Status.UNIMPLEMENTED.asException()); - inOrder.verify(mockStateListeners[0]).onSubchannelState( + inOrder.verify(origLb).handleSubchannelState( same(subchannels[0]), eq(ConnectivityStateInfo.forNonError(READY))); assertThat(subchannels[0].logs).containsExactly( "ERROR: Health-check disabled: " + Status.UNIMPLEMENTED, @@ -401,31 +417,32 @@ public void healthCheckDisabledWhenServiceNotImplemented() { // subchannels[1] has normal health checking serverCall1.responseObserver.onNext(makeResponse(ServingStatus.NOT_SERVING)); - inOrder.verify(mockStateListeners[1]).onSubchannelState( + inOrder.verify(origLb).handleSubchannelState( same(subchannels[1]), unavailableStateWithMsg("Health-check service responded NOT_SERVING for 'BarService'")); - // Without health checking, states from underlying Subchannel are delivered directly to the mock - // listeners. - deliverSubchannelState(0, ConnectivityStateInfo.forNonError(IDLE)); - inOrder.verify(mockStateListeners[0]).onSubchannelState( + // Without health checking, states from underlying Subchannel are delivered directly to origLb + hcLbEventDelivery.handleSubchannelState( + subchannels[0], ConnectivityStateInfo.forNonError(IDLE)); + inOrder.verify(origLb).handleSubchannelState( same(subchannels[0]), eq(ConnectivityStateInfo.forNonError(IDLE))); // Re-connecting on a Subchannel will reset the "disabled" flag. assertThat(healthImpls[0].calls).hasSize(0); - deliverSubchannelState(0, ConnectivityStateInfo.forNonError(READY)); + hcLbEventDelivery.handleSubchannelState( + subchannels[0], ConnectivityStateInfo.forNonError(READY)); assertThat(healthImpls[0].calls).hasSize(1); serverCall0 = healthImpls[0].calls.poll(); - inOrder.verify(mockStateListeners[0]).onSubchannelState( + inOrder.verify(origLb).handleSubchannelState( same(subchannels[0]), eq(ConnectivityStateInfo.forNonError(CONNECTING))); // Health check now works as normal serverCall0.responseObserver.onNext(makeResponse(ServingStatus.SERVICE_UNKNOWN)); - inOrder.verify(mockStateListeners[0]).onSubchannelState( + inOrder.verify(origLb).handleSubchannelState( same(subchannels[0]), unavailableStateWithMsg("Health-check service responded SERVICE_UNKNOWN for 'BarService'")); - verifyNoMoreInteractions(origLb, mockStateListeners[0], mockStateListeners[1]); + verifyNoMoreInteractions(origLb); verifyZeroInteractions(backoffPolicyProvider); } @@ -443,11 +460,10 @@ public void backoffRetriesWhenServerErroneouslyClosesRpcBeforeAnyResponse() { FakeSubchannel subchannel = (FakeSubchannel) createSubchannel(0, Attributes.EMPTY); assertThat(subchannel).isSameInstanceAs(subchannels[0]); - SubchannelStateListener mockListener = mockStateListeners[0]; - InOrder inOrder = inOrder(mockListener, backoffPolicyProvider, backoffPolicy1, backoffPolicy2); + InOrder inOrder = inOrder(origLb, backoffPolicyProvider, backoffPolicy1, backoffPolicy2); - deliverSubchannelState(0, ConnectivityStateInfo.forNonError(READY)); - inOrder.verify(mockListener).onSubchannelState( + hcLbEventDelivery.handleSubchannelState(subchannel, ConnectivityStateInfo.forNonError(READY)); + inOrder.verify(origLb).handleSubchannelState( same(subchannel), eq(ConnectivityStateInfo.forNonError(CONNECTING))); HealthImpl healthImpl = healthImpls[0]; assertThat(healthImpl.calls).hasSize(1); @@ -458,7 +474,7 @@ public void backoffRetriesWhenServerErroneouslyClosesRpcBeforeAnyResponse() { healthImpl.calls.poll().responseObserver.onCompleted(); // which results in TRANSIENT_FAILURE - inOrder.verify(mockListener).onSubchannelState( + inOrder.verify(origLb).handleSubchannelState( same(subchannel), unavailableStateWithMsg( "Health-check stream unexpectedly closed with " + Status.OK + " for 'TeeService'")); @@ -471,7 +487,7 @@ public void backoffRetriesWhenServerErroneouslyClosesRpcBeforeAnyResponse() { inOrder.verify(backoffPolicy1).nextBackoffNanos(); assertThat(clock.getPendingTasks()).hasSize(1); - verifyRetryAfterNanos(inOrder, mockListener, subchannel, healthImpl, 11); + verifyRetryAfterNanos(inOrder, subchannel, healthImpl, 11); assertThat(clock.getPendingTasks()).isEmpty(); subchannel.logs.clear(); @@ -479,7 +495,7 @@ public void backoffRetriesWhenServerErroneouslyClosesRpcBeforeAnyResponse() { healthImpl.calls.poll().responseObserver.onError(Status.CANCELLED.asException()); // which also results in TRANSIENT_FAILURE, with a different description - inOrder.verify(mockListener).onSubchannelState( + inOrder.verify(origLb).handleSubchannelState( same(subchannel), unavailableStateWithMsg( "Health-check stream unexpectedly closed with " @@ -491,15 +507,15 @@ public void backoffRetriesWhenServerErroneouslyClosesRpcBeforeAnyResponse() { // Retry with backoff inOrder.verify(backoffPolicy1).nextBackoffNanos(); - verifyRetryAfterNanos(inOrder, mockListener, subchannel, healthImpl, 21); + verifyRetryAfterNanos(inOrder, subchannel, healthImpl, 21); // Server responds this time healthImpl.calls.poll().responseObserver.onNext(makeResponse(ServingStatus.SERVING)); - inOrder.verify(mockListener).onSubchannelState( + inOrder.verify(origLb).handleSubchannelState( same(subchannel), eq(ConnectivityStateInfo.forNonError(READY))); - verifyNoMoreInteractions(origLb, mockListener, backoffPolicyProvider, backoffPolicy1); + verifyNoMoreInteractions(origLb, backoffPolicyProvider, backoffPolicy1); } @Test @@ -514,14 +530,12 @@ public void serverRespondResetsBackoff() { verify(origLb).handleResolvedAddresses(result); verifyNoMoreInteractions(origLb); - SubchannelStateListener mockStateListener = mockStateListeners[0]; Subchannel subchannel = createSubchannel(0, Attributes.EMPTY); assertThat(subchannel).isSameInstanceAs(subchannels[0]); - InOrder inOrder = - inOrder(mockStateListener, backoffPolicyProvider, backoffPolicy1, backoffPolicy2); + InOrder inOrder = inOrder(origLb, backoffPolicyProvider, backoffPolicy1, backoffPolicy2); - deliverSubchannelState(0, ConnectivityStateInfo.forNonError(READY)); - inOrder.verify(mockStateListener).onSubchannelState( + hcLbEventDelivery.handleSubchannelState(subchannel, ConnectivityStateInfo.forNonError(READY)); + inOrder.verify(origLb).handleSubchannelState( same(subchannel), eq(ConnectivityStateInfo.forNonError(CONNECTING))); HealthImpl healthImpl = healthImpls[0]; assertThat(healthImpl.calls).hasSize(1); @@ -531,7 +545,7 @@ public void serverRespondResetsBackoff() { healthImpl.calls.poll().responseObserver.onError(Status.CANCELLED.asException()); // which results in TRANSIENT_FAILURE - inOrder.verify(mockStateListener).onSubchannelState( + inOrder.verify(origLb).handleSubchannelState( same(subchannel), unavailableStateWithMsg( "Health-check stream unexpectedly closed with " @@ -542,19 +556,19 @@ public void serverRespondResetsBackoff() { inOrder.verify(backoffPolicy1).nextBackoffNanos(); assertThat(clock.getPendingTasks()).hasSize(1); - verifyRetryAfterNanos(inOrder, mockStateListener, subchannel, healthImpl, 11); + verifyRetryAfterNanos(inOrder, subchannel, healthImpl, 11); assertThat(clock.getPendingTasks()).isEmpty(); // Server responds healthImpl.calls.peek().responseObserver.onNext(makeResponse(ServingStatus.SERVING)); - inOrder.verify(mockStateListener).onSubchannelState( + inOrder.verify(origLb).handleSubchannelState( same(subchannel), eq(ConnectivityStateInfo.forNonError(READY))); - verifyNoMoreInteractions(mockStateListener); + verifyNoMoreInteractions(origLb); // then closes the stream healthImpl.calls.poll().responseObserver.onError(Status.UNAVAILABLE.asException()); - inOrder.verify(mockStateListener).onSubchannelState( + inOrder.verify(origLb).handleSubchannelState( same(subchannel), unavailableStateWithMsg( "Health-check stream unexpectedly closed with " @@ -563,7 +577,7 @@ public void serverRespondResetsBackoff() { // Because server has responded, the first retry is not subject to backoff. // But the backoff policy has been reset. A new backoff policy will be used for // the next backed-off retry. - inOrder.verify(mockStateListener).onSubchannelState( + inOrder.verify(origLb).handleSubchannelState( same(subchannel), eq(ConnectivityStateInfo.forNonError(CONNECTING))); assertThat(healthImpl.calls).hasSize(1); assertThat(clock.getPendingTasks()).isEmpty(); @@ -571,7 +585,7 @@ public void serverRespondResetsBackoff() { // then closes the stream for this retry healthImpl.calls.poll().responseObserver.onError(Status.UNAVAILABLE.asException()); - inOrder.verify(mockStateListener).onSubchannelState( + inOrder.verify(origLb).handleSubchannelState( same(subchannel), unavailableStateWithMsg( "Health-check stream unexpectedly closed with " @@ -582,19 +596,18 @@ public void serverRespondResetsBackoff() { // Retry with a new backoff policy inOrder.verify(backoffPolicy2).nextBackoffNanos(); - verifyRetryAfterNanos(inOrder, mockStateListener, subchannel, healthImpl, 12); + verifyRetryAfterNanos(inOrder, subchannel, healthImpl, 12); } private void verifyRetryAfterNanos( - InOrder inOrder, SubchannelStateListener listener, Subchannel subchannel, HealthImpl impl, - long nanos) { + InOrder inOrder, Subchannel subchannel, HealthImpl impl, long nanos) { assertThat(impl.calls).isEmpty(); clock.forwardNanos(nanos - 1); assertThat(impl.calls).isEmpty(); inOrder.verifyNoMoreInteractions(); - verifyNoMoreInteractions(listener); + verifyNoMoreInteractions(origLb); clock.forwardNanos(1); - inOrder.verify(listener).onSubchannelState( + inOrder.verify(origLb).handleSubchannelState( same(subchannel), eq(ConnectivityStateInfo.forNonError(CONNECTING))); assertThat(impl.calls).hasSize(1); } @@ -615,12 +628,13 @@ public void serviceConfigHasNoHealthCheckingInitiallyButDoesLater() { createSubchannel(0, Attributes.EMPTY); // No health check activity. Underlying Subchannel states are directly propagated - deliverSubchannelState(0, ConnectivityStateInfo.forNonError(READY)); + hcLbEventDelivery.handleSubchannelState( + subchannels[0], ConnectivityStateInfo.forNonError(READY)); assertThat(healthImpls[0].calls).isEmpty(); - verify(mockStateListeners[0]).onSubchannelState( + verify(origLb).handleSubchannelState( same(subchannels[0]), eq(ConnectivityStateInfo.forNonError(READY))); - verifyNoMoreInteractions(mockStateListeners[0]); + verifyNoMoreInteractions(origLb); // Service config enables health check Attributes resolutionAttrs = attrsWithHealthCheckService("FooService"); @@ -635,12 +649,13 @@ public void serviceConfigHasNoHealthCheckingInitiallyButDoesLater() { assertThat(healthImpls[0].calls).hasSize(1); // State stays in READY, instead of switching to CONNECTING. - verifyNoMoreInteractions(mockStateListeners[0]); + verifyNoMoreInteractions(origLb); // Start Subchannel 1, which will have health check createSubchannel(1, Attributes.EMPTY); assertThat(healthImpls[1].calls).isEmpty(); - deliverSubchannelState(1, ConnectivityStateInfo.forNonError(READY)); + hcLbEventDelivery.handleSubchannelState( + subchannels[1], ConnectivityStateInfo.forNonError(READY)); assertThat(healthImpls[1].calls).hasSize(1); } @@ -658,10 +673,10 @@ public void serviceConfigDisablesHealthCheckWhenRpcActive() { Subchannel subchannel = createSubchannel(0, Attributes.EMPTY); assertThat(subchannel).isSameInstanceAs(subchannels[0]); - InOrder inOrder = inOrder(origLb, mockStateListeners[0]); + InOrder inOrder = inOrder(origLb); - deliverSubchannelState(0, ConnectivityStateInfo.forNonError(READY)); - inOrder.verify(mockStateListeners[0]).onSubchannelState( + hcLbEventDelivery.handleSubchannelState(subchannel, ConnectivityStateInfo.forNonError(READY)); + inOrder.verify(origLb).handleSubchannelState( same(subchannel), eq(ConnectivityStateInfo.forNonError(CONNECTING))); inOrder.verifyNoMoreInteractions(); HealthImpl healthImpl = healthImpls[0]; @@ -679,12 +694,12 @@ public void serviceConfigDisablesHealthCheckWhenRpcActive() { // Health check RPC cancelled. assertThat(serverCall.cancelled).isTrue(); // Subchannel uses original state - inOrder.verify(mockStateListeners[0]).onSubchannelState( + inOrder.verify(origLb).handleSubchannelState( same(subchannel), eq(ConnectivityStateInfo.forNonError(READY))); inOrder.verify(origLb).handleResolvedAddresses(result2); - verifyNoMoreInteractions(origLb, mockStateListeners[0]); + verifyNoMoreInteractions(origLb); assertThat(healthImpl.calls).isEmpty(); } @@ -702,10 +717,10 @@ public void serviceConfigDisablesHealthCheckWhenRetryPending() { Subchannel subchannel = createSubchannel(0, Attributes.EMPTY); assertThat(subchannel).isSameInstanceAs(subchannels[0]); - InOrder inOrder = inOrder(origLb, mockStateListeners[0]); + InOrder inOrder = inOrder(origLb); - deliverSubchannelState(0, ConnectivityStateInfo.forNonError(READY)); - inOrder.verify(mockStateListeners[0]).onSubchannelState( + hcLbEventDelivery.handleSubchannelState(subchannel, ConnectivityStateInfo.forNonError(READY)); + inOrder.verify(origLb).handleSubchannelState( same(subchannel), eq(ConnectivityStateInfo.forNonError(CONNECTING))); inOrder.verifyNoMoreInteractions(); HealthImpl healthImpl = healthImpls[0]; @@ -715,7 +730,7 @@ public void serviceConfigDisablesHealthCheckWhenRetryPending() { assertThat(clock.getPendingTasks()).isEmpty(); healthImpl.calls.poll().responseObserver.onCompleted(); assertThat(clock.getPendingTasks()).hasSize(1); - inOrder.verify(mockStateListeners[0]).onSubchannelState( + inOrder.verify(origLb).handleSubchannelState( same(subchannel), unavailableStateWithMsg( "Health-check stream unexpectedly closed with " + Status.OK + " for 'TeeService'")); @@ -734,12 +749,12 @@ public void serviceConfigDisablesHealthCheckWhenRetryPending() { assertThat(healthImpl.calls).isEmpty(); // Subchannel uses original state - inOrder.verify(mockStateListeners[0]).onSubchannelState( + inOrder.verify(origLb).handleSubchannelState( same(subchannel), eq(ConnectivityStateInfo.forNonError(READY))); inOrder.verify(origLb).handleResolvedAddresses(result2); - verifyNoMoreInteractions(origLb, mockStateListeners[0]); + verifyNoMoreInteractions(origLb); } @Test @@ -756,15 +771,14 @@ public void serviceConfigDisablesHealthCheckWhenRpcInactive() { Subchannel subchannel = createSubchannel(0, Attributes.EMPTY); assertThat(subchannel).isSameInstanceAs(subchannels[0]); - InOrder inOrder = inOrder(origLb, mockStateListeners[0]); + InOrder inOrder = inOrder(origLb); // Underlying subchannel is not READY initially ConnectivityStateInfo underlyingErrorState = ConnectivityStateInfo.forTransientFailure( Status.UNAVAILABLE.withDescription("connection refused")); - deliverSubchannelState(0, underlyingErrorState); - inOrder.verify(mockStateListeners[0]) - .onSubchannelState(same(subchannel), same(underlyingErrorState)); + hcLbEventDelivery.handleSubchannelState(subchannel, underlyingErrorState); + inOrder.verify(origLb).handleSubchannelState(same(subchannel), same(underlyingErrorState)); inOrder.verifyNoMoreInteractions(); // NameResolver gives an update without service config, thus health check will be disabled @@ -777,16 +791,16 @@ public void serviceConfigDisablesHealthCheckWhenRpcInactive() { inOrder.verify(origLb).handleResolvedAddresses(result2); // Underlying subchannel is now ready - deliverSubchannelState(0, ConnectivityStateInfo.forNonError(READY)); + hcLbEventDelivery.handleSubchannelState(subchannel, ConnectivityStateInfo.forNonError(READY)); // Since health check is disabled, READY state is propagated directly. - inOrder.verify(mockStateListeners[0]).onSubchannelState( + inOrder.verify(origLb).handleSubchannelState( same(subchannel), eq(ConnectivityStateInfo.forNonError(READY))); // and there is no health check activity. assertThat(healthImpls[0].calls).isEmpty(); - verifyNoMoreInteractions(origLb, mockStateListeners[0]); + verifyNoMoreInteractions(origLb); } @Test @@ -802,12 +816,11 @@ public void serviceConfigChangesServiceNameWhenRpcActive() { verifyNoMoreInteractions(origLb); Subchannel subchannel = createSubchannel(0, Attributes.EMPTY); - SubchannelStateListener mockListener = mockStateListeners[0]; assertThat(subchannel).isSameInstanceAs(subchannels[0]); - InOrder inOrder = inOrder(origLb, mockListener); + InOrder inOrder = inOrder(origLb); - deliverSubchannelState(0, ConnectivityStateInfo.forNonError(READY)); - inOrder.verify(mockListener).onSubchannelState( + hcLbEventDelivery.handleSubchannelState(subchannel, ConnectivityStateInfo.forNonError(READY)); + inOrder.verify(origLb).handleSubchannelState( same(subchannel), eq(ConnectivityStateInfo.forNonError(CONNECTING))); HealthImpl healthImpl = healthImpls[0]; @@ -818,14 +831,14 @@ public void serviceConfigChangesServiceNameWhenRpcActive() { // Health check responded serverCall.responseObserver.onNext(makeResponse(ServingStatus.SERVING)); - inOrder.verify(mockListener).onSubchannelState( + inOrder.verify(origLb).handleSubchannelState( same(subchannel), eq(ConnectivityStateInfo.forNonError(READY))); // Service config returns with the same health check name. hcLbEventDelivery.handleResolvedAddresses(result1); // It's delivered to origLb, but nothing else happens inOrder.verify(origLb).handleResolvedAddresses(result1); - verifyNoMoreInteractions(origLb, mockListener); + verifyNoMoreInteractions(origLb); // Service config returns a different health check name. resolutionAttrs = attrsWithHealthCheckService("FooService"); @@ -846,7 +859,7 @@ public void serviceConfigChangesServiceNameWhenRpcActive() { assertThat(serverCall.request).isEqualTo(makeRequest("FooService")); // State stays in READY, instead of switching to CONNECTING. - verifyNoMoreInteractions(origLb, mockListener); + verifyNoMoreInteractions(origLb); } @Test @@ -862,12 +875,11 @@ public void serviceConfigChangesServiceNameWhenRetryPending() { verifyNoMoreInteractions(origLb); Subchannel subchannel = createSubchannel(0, Attributes.EMPTY); - SubchannelStateListener mockListener = mockStateListeners[0]; assertThat(subchannel).isSameInstanceAs(subchannels[0]); - InOrder inOrder = inOrder(origLb, mockListener); + InOrder inOrder = inOrder(origLb); - deliverSubchannelState(0, ConnectivityStateInfo.forNonError(READY)); - inOrder.verify(mockListener).onSubchannelState( + hcLbEventDelivery.handleSubchannelState(subchannel, ConnectivityStateInfo.forNonError(READY)); + inOrder.verify(origLb).handleSubchannelState( same(subchannel), eq(ConnectivityStateInfo.forNonError(CONNECTING))); HealthImpl healthImpl = healthImpls[0]; @@ -881,7 +893,7 @@ public void serviceConfigChangesServiceNameWhenRetryPending() { serverCall.responseObserver.onCompleted(); assertThat(clock.getPendingTasks()).hasSize(1); assertThat(healthImpl.calls).isEmpty(); - inOrder.verify(mockListener).onSubchannelState( + inOrder.verify(origLb).handleSubchannelState( same(subchannel), unavailableStateWithMsg( "Health-check stream unexpectedly closed with " + Status.OK + " for 'TeeService'")); @@ -891,7 +903,7 @@ public void serviceConfigChangesServiceNameWhenRetryPending() { hcLbEventDelivery.handleResolvedAddresses(result1); // It's delivered to origLb, but nothing else happens inOrder.verify(origLb).handleResolvedAddresses(result1); - verifyNoMoreInteractions(origLb, mockListener); + verifyNoMoreInteractions(origLb); assertThat(clock.getPendingTasks()).hasSize(1); assertThat(healthImpl.calls).isEmpty(); @@ -904,7 +916,7 @@ public void serviceConfigChangesServiceNameWhenRetryPending() { hcLbEventDelivery.handleResolvedAddresses(result2); // Concluded CONNECTING state - inOrder.verify(mockListener).onSubchannelState( + inOrder.verify(origLb).handleSubchannelState( same(subchannel), eq(ConnectivityStateInfo.forNonError(CONNECTING))); inOrder.verify(origLb).handleResolvedAddresses(result2); @@ -918,7 +930,7 @@ public void serviceConfigChangesServiceNameWhenRetryPending() { // with the new service name assertThat(serverCall.request).isEqualTo(makeRequest("FooService")); - verifyNoMoreInteractions(origLb, mockListener); + verifyNoMoreInteractions(origLb); } @Test @@ -934,17 +946,16 @@ public void serviceConfigChangesServiceNameWhenRpcInactive() { verifyNoMoreInteractions(origLb); Subchannel subchannel = createSubchannel(0, Attributes.EMPTY); - SubchannelStateListener mockListener = mockStateListeners[0]; assertThat(subchannel).isSameInstanceAs(subchannels[0]); - InOrder inOrder = inOrder(origLb, mockListener); + InOrder inOrder = inOrder(origLb); HealthImpl healthImpl = healthImpls[0]; // Underlying subchannel is not READY initially ConnectivityStateInfo underlyingErrorState = ConnectivityStateInfo.forTransientFailure( Status.UNAVAILABLE.withDescription("connection refused")); - deliverSubchannelState(0, underlyingErrorState); - inOrder.verify(mockListener).onSubchannelState(same(subchannel), same(underlyingErrorState)); + hcLbEventDelivery.handleSubchannelState(subchannel, underlyingErrorState); + inOrder.verify(origLb).handleSubchannelState(same(subchannel), same(underlyingErrorState)); inOrder.verifyNoMoreInteractions(); // Service config returns with the same health check name. @@ -965,10 +976,10 @@ public void serviceConfigChangesServiceNameWhenRpcInactive() { inOrder.verify(origLb).handleResolvedAddresses(result2); // Underlying subchannel is now ready - deliverSubchannelState(0, ConnectivityStateInfo.forNonError(READY)); + hcLbEventDelivery.handleSubchannelState(subchannel, ConnectivityStateInfo.forNonError(READY)); // Concluded CONNECTING state - inOrder.verify(mockListener).onSubchannelState( + inOrder.verify(origLb).handleSubchannelState( same(subchannel), eq(ConnectivityStateInfo.forNonError(CONNECTING))); // Health check RPC is started @@ -976,7 +987,7 @@ public void serviceConfigChangesServiceNameWhenRpcInactive() { // with the new service name assertThat(healthImpl.calls.poll().request).isEqualTo(makeRequest("FooService")); - verifyNoMoreInteractions(origLb, mockListener); + verifyNoMoreInteractions(origLb); } @Test @@ -1020,18 +1031,17 @@ public void balancerShutdown() { verifyNoMoreInteractions(origLb); Subchannel subchannel = createSubchannel(0, Attributes.EMPTY); - SubchannelStateListener mockListener = mockStateListeners[0]; assertThat(subchannel).isSameInstanceAs(subchannels[0]); // Trigger the health check - deliverSubchannelState(0, ConnectivityStateInfo.forNonError(READY)); + hcLbEventDelivery.handleSubchannelState(subchannel, ConnectivityStateInfo.forNonError(READY)); HealthImpl healthImpl = healthImpls[0]; assertThat(healthImpl.calls).hasSize(1); ServerSideCall serverCall = healthImpl.calls.poll(); assertThat(serverCall.cancelled).isFalse(); - verify(mockListener).onSubchannelState( + verify(origLb).handleSubchannelState( same(subchannel), eq(ConnectivityStateInfo.forNonError(CONNECTING))); // Shut down the balancer @@ -1042,7 +1052,7 @@ public void balancerShutdown() { assertThat(serverCall.cancelled).isTrue(); // LoadBalancer API requires no more callbacks on LoadBalancer after shutdown() is called. - verifyNoMoreInteractions(origLb, mockListener); + verifyNoMoreInteractions(origLb); // No more health check call is made or scheduled assertThat(healthImpl.calls).isEmpty(); @@ -1075,7 +1085,8 @@ public LoadBalancer newLoadBalancer(Helper helper) { verify(origLb).handleResolvedAddresses(result); createSubchannel(0, Attributes.EMPTY); assertThat(healthImpls[0].calls).isEmpty(); - deliverSubchannelState(0, ConnectivityStateInfo.forNonError(READY)); + hcLbEventDelivery.handleSubchannelState( + subchannels[0], ConnectivityStateInfo.forNonError(READY)); assertThat(healthImpls[0].calls).hasSize(1); } @@ -1168,7 +1179,6 @@ private class FakeSubchannel extends Subchannel { final Attributes attrs; final Channel channel; final ArrayList logs = new ArrayList<>(); - final int index; private final ChannelLogger logger = new ChannelLogger() { @Override public void log(ChannelLogLevel level, String msg) { @@ -1181,16 +1191,15 @@ public void log(ChannelLogLevel level, String template, Object... args) { } }; - FakeSubchannel(int index, CreateSubchannelArgs args, Channel channel) { - this.index = index; - this.eagList = args.getAddresses(); - this.attrs = args.getAttributes(); + FakeSubchannel(List eagList, Attributes attrs, Channel channel) { + this.eagList = Collections.unmodifiableList(eagList); + this.attrs = checkNotNull(attrs); this.channel = checkNotNull(channel); } @Override public void shutdown() { - deliverSubchannelState(index, ConnectivityStateInfo.forNonError(SHUTDOWN)); + hcLbEventDelivery.handleSubchannelState(this, ConnectivityStateInfo.forNonError(SHUTDOWN)); } @Override @@ -1221,19 +1230,18 @@ public ChannelLogger getChannelLogger() { private class FakeHelper extends Helper { @Override - public Subchannel createSubchannel(CreateSubchannelArgs args) { + public Subchannel createSubchannel(List addrs, Attributes attrs) { int index = -1; for (int i = 0; i < NUM_SUBCHANNELS; i++) { - if (eagLists[i].equals(args.getAddresses())) { + if (eagLists[i] == addrs) { index = i; break; } } - checkState(index >= 0, "addrs " + args.getAddresses() + " not found"); - FakeSubchannel subchannel = new FakeSubchannel(index, args, channels[index]); + checkState(index >= 0, "addrs " + addrs + " not found"); + FakeSubchannel subchannel = new FakeSubchannel(addrs, attrs, channels[index]); checkState(subchannels[index] == null, "subchannels[" + index + "] already created"); subchannels[index] = subchannel; - stateListeners[index] = args.getStateListener(); return subchannel; } @@ -1289,23 +1297,9 @@ private Subchannel createSubchannel(final int index, final Attributes attrs) { syncContext.execute(new Runnable() { @Override public void run() { - returnedSubchannel.set( - wrappedHelper.createSubchannel(CreateSubchannelArgs.newBuilder() - .setAddresses(eagLists[index]) - .setAttributes(attrs) - .setStateListener(mockStateListeners[index]) - .build())); + returnedSubchannel.set(wrappedHelper.createSubchannel(eagLists[index], attrs)); } }); return returnedSubchannel.get(); } - - private void deliverSubchannelState(final int index, final ConnectivityStateInfo newState) { - syncContext.execute(new Runnable() { - @Override - public void run() { - stateListeners[index].onSubchannelState(subchannels[index], newState); - } - }); - } }