From 1ded8aff814d4064902337f264acec8b5cde419c Mon Sep 17 00:00:00 2001 From: Kannan J Date: Mon, 7 Oct 2024 17:55:56 +0530 Subject: [PATCH] On result2 resolution result have addresses or error (#11330) Combined success / error status passed via ResolutionResult to the NameResolver.Listener2 interface's onResult2 method - Addresses in the success case or address resolution error in the failure case now get set in ResolutionResult::addressesOrError by the internal name resolvers. --- api/src/main/java/io/grpc/NameResolver.java | 71 +++++++---- api/src/main/java/io/grpc/StatusOr.java | 102 ++++++++++++++++ .../test/java/io/grpc/NameResolverTest.java | 110 ++++++++++++++++++ api/src/test/java/io/grpc/StatusOrTest.java | 81 +++++++++++++ build.gradle | 1 + .../io/grpc/internal/DnsNameResolver.java | 22 +++- .../io/grpc/internal/ManagedChannelImpl.java | 28 +++-- .../internal/ManagedChannelImplBuilder.java | 7 +- .../io/grpc/internal/DnsNameResolverTest.java | 65 ++++++++--- .../ManagedChannelImplIdlenessTest.java | 3 +- .../grpc/internal/ManagedChannelImplTest.java | 36 ++++-- .../grpc/grpclb/GrpclbNameResolverTest.java | 19 ++- .../java/io/grpc/netty/UdsNameResolver.java | 8 +- .../grpc/netty/UdsNameResolverProvider.java | 2 +- .../netty/UdsNameResolverProviderTest.java | 41 +++++-- .../io/grpc/netty/UdsNameResolverTest.java | 29 ++++- .../testing/FakeNameResolverProvider.java | 6 +- .../xds/ClusterResolverLoadBalancerTest.java | 4 +- .../java/io/grpc/xds/XdsNameResolverTest.java | 10 +- .../grpc/xds/XdsSecurityClientServerTest.java | 4 +- 20 files changed, 541 insertions(+), 108 deletions(-) create mode 100644 api/src/main/java/io/grpc/StatusOr.java create mode 100644 api/src/test/java/io/grpc/StatusOrTest.java diff --git a/api/src/main/java/io/grpc/NameResolver.java b/api/src/main/java/io/grpc/NameResolver.java index bfb9c2a43a..b9590ab5d5 100644 --- a/api/src/main/java/io/grpc/NameResolver.java +++ b/api/src/main/java/io/grpc/NameResolver.java @@ -20,13 +20,13 @@ 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.MoreObjects.ToStringHelper; import com.google.common.base.Objects; import com.google.errorprone.annotations.InlineMe; import java.lang.annotation.Documented; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; import java.net.URI; -import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.Map; @@ -95,7 +95,8 @@ public abstract class NameResolver { @Override public void onResult(ResolutionResult resolutionResult) { - listener.onAddresses(resolutionResult.getAddresses(), resolutionResult.getAttributes()); + listener.onAddresses(resolutionResult.getAddressesOrError().getValue(), + resolutionResult.getAttributes()); } }); } @@ -218,19 +219,21 @@ public abstract class NameResolver { @Override @Deprecated @InlineMe( - replacement = "this.onResult(ResolutionResult.newBuilder().setAddresses(servers)" - + ".setAttributes(attributes).build())", - imports = "io.grpc.NameResolver.ResolutionResult") + replacement = "this.onResult2(ResolutionResult.newBuilder().setAddressesOrError(" + + "StatusOr.fromValue(servers)).setAttributes(attributes).build())", + imports = {"io.grpc.NameResolver.ResolutionResult", "io.grpc.StatusOr"}) public final void onAddresses( List servers, @ResolutionResultAttr Attributes attributes) { // TODO(jihuncho) need to promote Listener2 if we want to use ConfigOrError - onResult( - ResolutionResult.newBuilder().setAddresses(servers).setAttributes(attributes).build()); + onResult2( + ResolutionResult.newBuilder().setAddressesOrError( + StatusOr.fromValue(servers)).setAttributes(attributes).build()); } /** * Handles updates on resolved addresses and attributes. If - * {@link ResolutionResult#getAddresses()} is empty, {@link #onError(Status)} will be called. + * {@link ResolutionResult#getAddressesOrError()} is empty, {@link #onError(Status)} will be + * called. * * @param resolutionResult the resolved server addresses, attributes, and Service Config. * @since 1.21.0 @@ -584,17 +587,17 @@ public abstract class NameResolver { */ @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1770") public static final class ResolutionResult { - private final List addresses; + private final StatusOr> addressesOrError; @ResolutionResultAttr private final Attributes attributes; @Nullable private final ConfigOrError serviceConfig; ResolutionResult( - List addresses, + StatusOr> addressesOrError, @ResolutionResultAttr Attributes attributes, ConfigOrError serviceConfig) { - this.addresses = Collections.unmodifiableList(new ArrayList<>(addresses)); + this.addressesOrError = addressesOrError; this.attributes = checkNotNull(attributes, "attributes"); this.serviceConfig = serviceConfig; } @@ -615,7 +618,7 @@ public abstract class NameResolver { */ public Builder toBuilder() { return newBuilder() - .setAddresses(addresses) + .setAddressesOrError(addressesOrError) .setAttributes(attributes) .setServiceConfig(serviceConfig); } @@ -624,9 +627,20 @@ public abstract class NameResolver { * Gets the addresses resolved by name resolution. * * @since 1.21.0 + * @deprecated Will be superseded by getAddressesOrError */ + @Deprecated public List getAddresses() { - return addresses; + return addressesOrError.getValue(); + } + + /** + * Gets the addresses resolved by name resolution or the error in doing so. + * + * @since 1.65.0 + */ + public StatusOr> getAddressesOrError() { + return addressesOrError; } /** @@ -652,11 +666,11 @@ public abstract class NameResolver { @Override public String toString() { - return MoreObjects.toStringHelper(this) - .add("addresses", addresses) - .add("attributes", attributes) - .add("serviceConfig", serviceConfig) - .toString(); + ToStringHelper stringHelper = MoreObjects.toStringHelper(this); + stringHelper.add("addressesOrError", addressesOrError.toString()); + stringHelper.add("attributes", attributes); + stringHelper.add("serviceConfigOrError", serviceConfig); + return stringHelper.toString(); } /** @@ -668,7 +682,7 @@ public abstract class NameResolver { return false; } ResolutionResult that = (ResolutionResult) obj; - return Objects.equal(this.addresses, that.addresses) + return Objects.equal(this.addressesOrError, that.addressesOrError) && Objects.equal(this.attributes, that.attributes) && Objects.equal(this.serviceConfig, that.serviceConfig); } @@ -678,7 +692,7 @@ public abstract class NameResolver { */ @Override public int hashCode() { - return Objects.hashCode(addresses, attributes, serviceConfig); + return Objects.hashCode(addressesOrError, attributes, serviceConfig); } /** @@ -688,7 +702,8 @@ public abstract class NameResolver { */ @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1770") public static final class Builder { - private List addresses = Collections.emptyList(); + private StatusOr> addresses = + StatusOr.fromValue(Collections.emptyList()); private Attributes attributes = Attributes.EMPTY; @Nullable private ConfigOrError serviceConfig; @@ -700,9 +715,21 @@ public abstract class NameResolver { * Sets the addresses resolved by name resolution. This field is required. * * @since 1.21.0 + * @deprecated Will be superseded by setAddressesOrError */ + @Deprecated public Builder setAddresses(List addresses) { - this.addresses = addresses; + setAddressesOrError(StatusOr.fromValue(addresses)); + return this; + } + + /** + * Sets the addresses resolved by name resolution or the error in doing so. This field is + * required. + * @param addresses Resolved addresses or an error in resolving addresses + */ + public Builder setAddressesOrError(StatusOr> addresses) { + this.addresses = checkNotNull(addresses, "StatusOr addresses cannot be null."); return this; } diff --git a/api/src/main/java/io/grpc/StatusOr.java b/api/src/main/java/io/grpc/StatusOr.java new file mode 100644 index 0000000000..8a88d9e62d --- /dev/null +++ b/api/src/main/java/io/grpc/StatusOr.java @@ -0,0 +1,102 @@ +/* + * Copyright 2024 The gRPC Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +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.MoreObjects.ToStringHelper; +import com.google.common.base.Objects; +import javax.annotation.Nullable; + +/** Either a Status or a value. */ +@ExperimentalApi("https://github.com/grpc/grpc-java/issues/11563") +public class StatusOr { + private StatusOr(Status status, T value) { + this.status = status; + this.value = value; + } + + /** Construct from a value. */ + public static StatusOr fromValue(@Nullable T value) { + StatusOr result = new StatusOr(null, value); + return result; + } + + /** Construct from a non-Ok status. */ + public static StatusOr fromStatus(Status status) { + StatusOr result = new StatusOr(checkNotNull(status, "status"), null); + checkArgument(!status.isOk(), "cannot use OK status: %s", status); + return result; + } + + /** Returns whether there is a value. */ + public boolean hasValue() { + return status == null; + } + + /** + * Returns the value if set or throws exception if there is no value set. This method is meant + * to be called after checking the return value of hasValue() first. + */ + public @Nullable T getValue() { + if (status != null) { + throw new IllegalStateException("No value present."); + } + return value; + } + + /** Returns the status. If there is a value (which can be null), returns OK. */ + public Status getStatus() { + return status == null ? Status.OK : status; + } + + @Override + public boolean equals(Object other) { + if (!(other instanceof StatusOr)) { + return false; + } + StatusOr otherStatus = (StatusOr) other; + if (hasValue() != otherStatus.hasValue()) { + return false; + } + if (hasValue()) { + return Objects.equal(value, otherStatus.value); + } + return Objects.equal(status, otherStatus.status); + } + + @Override + public int hashCode() { + return Objects.hashCode(status, value); + } + + @Override + public String toString() { + ToStringHelper stringHelper = MoreObjects.toStringHelper(this); + if (status == null) { + stringHelper.add("value", value); + } else { + stringHelper.add("error", status); + } + return stringHelper.toString(); + } + + private final Status status; + private final T value; +} diff --git a/api/src/test/java/io/grpc/NameResolverTest.java b/api/src/test/java/io/grpc/NameResolverTest.java index f825de354a..1bc32ee7b1 100644 --- a/api/src/test/java/io/grpc/NameResolverTest.java +++ b/api/src/test/java/io/grpc/NameResolverTest.java @@ -17,20 +17,43 @@ package io.grpc; import static com.google.common.truth.Truth.assertThat; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import com.google.common.base.Objects; +import io.grpc.NameResolver.ConfigOrError; +import io.grpc.NameResolver.Listener2; +import io.grpc.NameResolver.ResolutionResult; import io.grpc.NameResolver.ServiceConfigParser; import java.lang.Thread.UncaughtExceptionHandler; +import java.net.SocketAddress; +import java.util.Collections; +import java.util.List; import java.util.concurrent.Executor; import java.util.concurrent.Executors; import java.util.concurrent.ScheduledExecutorService; +import org.junit.Rule; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnit; +import org.mockito.junit.MockitoRule; /** Unit tests for the inner classes in {@link NameResolver}. */ @RunWith(JUnit4.class) public class NameResolverTest { + private static final List ADDRESSES = + Collections.singletonList( + new EquivalentAddressGroup(new FakeSocketAddress("fake-address-1"), Attributes.EMPTY)); + private static final Attributes.Key YOLO_KEY = Attributes.Key.create("yolo"); + private static Attributes ATTRIBUTES = Attributes.newBuilder() + .set(YOLO_KEY, "To be, or not to be?").build(); + private static ConfigOrError CONFIG = ConfigOrError.fromConfig("foo"); + + @Rule + public final MockitoRule mocks = MockitoJUnit.rule(); private final int defaultPort = 293; private final ProxyDetector proxyDetector = mock(ProxyDetector.class); private final SynchronizationContext syncContext = @@ -41,6 +64,7 @@ public class NameResolverTest { private final ChannelLogger channelLogger = mock(ChannelLogger.class); private final Executor executor = Executors.newSingleThreadExecutor(); private final String overrideAuthority = "grpc.io"; + @Mock NameResolver.Listener mockListener; @Test public void args() { @@ -80,4 +104,90 @@ public class NameResolverTest { .setOverrideAuthority(overrideAuthority) .build(); } + + @Test + @SuppressWarnings("deprecation") + public void startOnOldListener_wrapperListener2UsedToStart() { + final Listener2[] listener2 = new Listener2[1]; + NameResolver nameResolver = new NameResolver() { + @Override + public String getServiceAuthority() { + return null; + } + + @Override + public void shutdown() {} + + @Override + public void start(Listener2 listener2Arg) { + listener2[0] = listener2Arg; + } + }; + nameResolver.start(mockListener); + + listener2[0].onResult(ResolutionResult.newBuilder().setAddresses(ADDRESSES) + .setAttributes(ATTRIBUTES).build()); + verify(mockListener).onAddresses(eq(ADDRESSES), eq(ATTRIBUTES)); + listener2[0].onError(Status.CANCELLED); + verify(mockListener).onError(Status.CANCELLED); + } + + @Test + @SuppressWarnings({"deprecation", "InlineMeInliner"}) + public void listener2AddressesToListener2ResolutionResultConversion() { + final ResolutionResult[] resolutionResult = new ResolutionResult[1]; + NameResolver.Listener2 listener2 = new Listener2() { + @Override + public void onResult(ResolutionResult resolutionResultArg) { + resolutionResult[0] = resolutionResultArg; + } + + @Override + public void onError(Status error) {} + }; + + listener2.onAddresses(ADDRESSES, ATTRIBUTES); + + assertThat(resolutionResult[0].getAddressesOrError().getValue()).isEqualTo(ADDRESSES); + assertThat(resolutionResult[0].getAttributes()).isEqualTo(ATTRIBUTES); + } + + @Test + public void resolutionResult_toString_addressesAttributesAndConfig() { + ResolutionResult resolutionResult = ResolutionResult.newBuilder() + .setAddressesOrError(StatusOr.fromValue(ADDRESSES)) + .setAttributes(ATTRIBUTES) + .setServiceConfig(CONFIG) + .build(); + + assertThat(resolutionResult.toString()).isEqualTo( + "ResolutionResult{addressesOrError=StatusOr{value=" + + "[[[FakeSocketAddress-fake-address-1]/{}]]}, attributes={yolo=To be, or not to be?}, " + + "serviceConfigOrError=ConfigOrError{config=foo}}"); + } + + @Test + public void resolutionResult_hashCode() { + ResolutionResult resolutionResult = ResolutionResult.newBuilder() + .setAddressesOrError(StatusOr.fromValue(ADDRESSES)) + .setAttributes(ATTRIBUTES) + .setServiceConfig(CONFIG) + .build(); + + assertThat(resolutionResult.hashCode()).isEqualTo( + Objects.hashCode(StatusOr.fromValue(ADDRESSES), ATTRIBUTES, CONFIG)); + } + + private static class FakeSocketAddress extends SocketAddress { + final String name; + + FakeSocketAddress(String name) { + this.name = name; + } + + @Override + public String toString() { + return "FakeSocketAddress-" + name; + } + } } diff --git a/api/src/test/java/io/grpc/StatusOrTest.java b/api/src/test/java/io/grpc/StatusOrTest.java new file mode 100644 index 0000000000..f63a314a2b --- /dev/null +++ b/api/src/test/java/io/grpc/StatusOrTest.java @@ -0,0 +1,81 @@ +/* + * Copyright 2015 The gRPC Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.grpc; + +import static com.google.common.truth.Truth.assertThat; +import static junit.framework.TestCase.fail; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Unit tests for {@link StatusOr}. **/ +@RunWith(JUnit4.class) +public class StatusOrTest { + + @Test + public void getValue_throwsIfNoValuePresent() { + try { + StatusOr.fromStatus(Status.ABORTED).getValue(); + + fail("Expected exception."); + } catch (IllegalStateException expected) { } + } + + @Test + @SuppressWarnings("TruthIncompatibleType") + public void equals_differentValueTypes() { + assertThat(StatusOr.fromValue(1)).isNotEqualTo(StatusOr.fromValue("1")); + } + + @Test + public void equals_differentValues() { + assertThat(StatusOr.fromValue(1)).isNotEqualTo(StatusOr.fromValue(2)); + } + + @Test + public void equals_sameValues() { + assertThat(StatusOr.fromValue(1)).isEqualTo(StatusOr.fromValue(1)); + } + + @Test + public void equals_differentStatuses() { + assertThat(StatusOr.fromStatus(Status.ABORTED)).isNotEqualTo( + StatusOr.fromStatus(Status.CANCELLED)); + } + + @Test + public void equals_sameStatuses() { + assertThat(StatusOr.fromStatus(Status.ABORTED)).isEqualTo(StatusOr.fromStatus(Status.ABORTED)); + } + + @Test + public void toString_value() { + assertThat(StatusOr.fromValue(1).toString()).isEqualTo("StatusOr{value=1}"); + } + + @Test + public void toString_nullValue() { + assertThat(StatusOr.fromValue(null).toString()).isEqualTo("StatusOr{value=null}"); + } + + @Test + public void toString_errorStatus() { + assertThat(StatusOr.fromStatus(Status.ABORTED).toString()).isEqualTo( + "StatusOr{error=Status{code=ABORTED, description=null, cause=null}}"); + } +} \ No newline at end of file diff --git a/build.gradle b/build.gradle index 740f534e13..6afe010de4 100644 --- a/build.gradle +++ b/build.gradle @@ -500,3 +500,4 @@ configurations { } tasks.register('checkForUpdates', CheckForUpdatesTask, project.configurations.checkForUpdates, "libs") + diff --git a/core/src/main/java/io/grpc/internal/DnsNameResolver.java b/core/src/main/java/io/grpc/internal/DnsNameResolver.java index df51d6f2c5..b59de833d7 100644 --- a/core/src/main/java/io/grpc/internal/DnsNameResolver.java +++ b/core/src/main/java/io/grpc/internal/DnsNameResolver.java @@ -32,6 +32,7 @@ import io.grpc.NameResolver; import io.grpc.ProxiedSocketAddress; import io.grpc.ProxyDetector; import io.grpc.Status; +import io.grpc.StatusOr; import io.grpc.SynchronizationContext; import io.grpc.internal.SharedResourceHolder.Resource; import java.io.IOException; @@ -59,7 +60,7 @@ import javax.annotation.Nullable; * A DNS-based {@link NameResolver}. * *

Each {@code A} or {@code AAAA} record emits an {@link EquivalentAddressGroup} in the list - * passed to {@link NameResolver.Listener2#onResult(ResolutionResult)}. + * passed to {@link NameResolver.Listener2#onResult2(ResolutionResult)}. * * @see DnsNameResolverProvider */ @@ -313,15 +314,20 @@ public class DnsNameResolver extends NameResolver { if (logger.isLoggable(Level.FINER)) { logger.finer("Using proxy address " + proxiedAddr); } - resolutionResultBuilder.setAddresses(Collections.singletonList(proxiedAddr)); + resolutionResultBuilder.setAddressesOrError( + StatusOr.fromValue(Collections.singletonList(proxiedAddr))); } else { result = doResolve(false); if (result.error != null) { - savedListener.onError(result.error); + InternalResolutionResult finalResult = result; + syncContext.execute(() -> + savedListener.onResult2(ResolutionResult.newBuilder() + .setAddressesOrError(StatusOr.fromStatus(finalResult.error)) + .build())); return; } if (result.addresses != null) { - resolutionResultBuilder.setAddresses(result.addresses); + resolutionResultBuilder.setAddressesOrError(StatusOr.fromValue(result.addresses)); } if (result.config != null) { resolutionResultBuilder.setServiceConfig(result.config); @@ -334,8 +340,12 @@ public class DnsNameResolver extends NameResolver { savedListener.onResult2(resolutionResultBuilder.build()); }); } catch (IOException e) { - savedListener.onError( - Status.UNAVAILABLE.withDescription("Unable to resolve host " + host).withCause(e)); + syncContext.execute(() -> + savedListener.onResult2(ResolutionResult.newBuilder() + .setAddressesOrError( + StatusOr.fromStatus( + Status.UNAVAILABLE.withDescription( + "Unable to resolve host " + host).withCause(e))).build())); } finally { final boolean succeed = result != null && result.error == null; syncContext.execute(new Runnable() { diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java index 36d79f4011..cda4299ace 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java @@ -81,6 +81,7 @@ import io.grpc.NameResolverProvider; import io.grpc.NameResolverRegistry; import io.grpc.ProxyDetector; import io.grpc.Status; +import io.grpc.StatusOr; import io.grpc.SynchronizationContext; import io.grpc.SynchronizationContext.ScheduledHandle; import io.grpc.internal.AutoConfiguredLoadBalancerFactory.AutoConfiguredLoadBalancer; @@ -1701,7 +1702,13 @@ final class ManagedChannelImpl extends ManagedChannel implements return Status.OK; } - List servers = resolutionResult.getAddresses(); + StatusOr> serversOrError = + resolutionResult.getAddressesOrError(); + if (!serversOrError.hasValue()) { + handleErrorInSyncContext(serversOrError.getStatus()); + return serversOrError.getStatus(); + } + List servers = serversOrError.getValue(); channelLogger.log( ChannelLogLevel.DEBUG, "Resolved address: {0}, config={1}", @@ -1709,10 +1716,10 @@ final class ManagedChannelImpl extends ManagedChannel implements resolutionResult.getAttributes()); if (lastResolutionState != ResolutionState.SUCCESS) { - channelLogger.log(ChannelLogLevel.INFO, "Address resolved: {0}", servers); + channelLogger.log(ChannelLogLevel.INFO, "Address resolved: {0}", + servers); lastResolutionState = ResolutionState.SUCCESS; } - ConfigOrError configOrError = resolutionResult.getServiceConfig(); InternalConfigSelector resolvedConfigSelector = resolutionResult.getAttributes().get(InternalConfigSelector.KEY); @@ -1788,7 +1795,7 @@ final class ManagedChannelImpl extends ManagedChannel implements } try { - // TODO(creamsoup): when `servers` is empty and lastResolutionStateCopy == SUCCESS + // TODO(creamsoup): when `serversOrError` is empty and lastResolutionStateCopy == SUCCESS // and lbNeedAddress, it shouldn't call the handleServiceConfigUpdate. But, // lbNeedAddress is not deterministic serviceConfigUpdated = true; @@ -1814,12 +1821,13 @@ final class ManagedChannelImpl extends ManagedChannel implements } Attributes attributes = attrBuilder.build(); - return helper.lb.tryAcceptResolvedAddresses( - ResolvedAddresses.newBuilder() - .setAddresses(servers) - .setAttributes(attributes) - .setLoadBalancingPolicyConfig(effectiveServiceConfig.getLoadBalancingConfig()) - .build()); + ResolvedAddresses.Builder resolvedAddresses = ResolvedAddresses.newBuilder() + .setAddresses(serversOrError.getValue()) + .setAttributes(attributes) + .setLoadBalancingPolicyConfig(effectiveServiceConfig.getLoadBalancingConfig()); + Status addressAcceptanceStatus = helper.lb.tryAcceptResolvedAddresses( + resolvedAddresses.build()); + return addressAcceptanceStatus; } return Status.OK; } diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index 7da9125087..48a255472e 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -45,6 +45,7 @@ import io.grpc.NameResolver; import io.grpc.NameResolverProvider; import io.grpc.NameResolverRegistry; import io.grpc.ProxyDetector; +import io.grpc.StatusOr; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.net.SocketAddress; @@ -877,9 +878,11 @@ public final class ManagedChannelImplBuilder @Override public void start(Listener2 listener) { - listener.onResult( + listener.onResult2( ResolutionResult.newBuilder() - .setAddresses(Collections.singletonList(new EquivalentAddressGroup(address))) + .setAddressesOrError( + StatusOr.fromValue( + Collections.singletonList(new EquivalentAddressGroup(address)))) .setAttributes(Attributes.EMPTY) .build()); } diff --git a/core/src/test/java/io/grpc/internal/DnsNameResolverTest.java b/core/src/test/java/io/grpc/internal/DnsNameResolverTest.java index 0512171f4e..be304ad326 100644 --- a/core/src/test/java/io/grpc/internal/DnsNameResolverTest.java +++ b/core/src/test/java/io/grpc/internal/DnsNameResolverTest.java @@ -24,6 +24,7 @@ import static org.junit.Assert.assertNull; import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.isA; import static org.mockito.Mockito.mock; @@ -152,12 +153,11 @@ public class DnsNameResolverTest { private NameResolver.Listener2 mockListener; @Captor private ArgumentCaptor resultCaptor; - @Captor - private ArgumentCaptor errorCaptor; @Nullable private String networkaddressCacheTtlPropertyValue; @Mock private RecordFetcher recordFetcher; + @Mock private ProxyDetector mockProxyDetector; private RetryingNameResolver newResolver(String name, int defaultPort) { return newResolver( @@ -570,7 +570,7 @@ public class DnsNameResolverTest { ArgumentCaptor ac = ArgumentCaptor.forClass(ResolutionResult.class); verify(mockListener).onResult2(ac.capture()); verifyNoMoreInteractions(mockListener); - assertThat(ac.getValue().getAddresses()).isEmpty(); + assertThat(ac.getValue().getAddressesOrError().getValue()).isEmpty(); assertThat(ac.getValue().getServiceConfig()).isNull(); verify(mockResourceResolver, never()).resolveSrv(anyString()); @@ -578,6 +578,39 @@ public class DnsNameResolverTest { assertEquals(0, fakeExecutor.numPendingTasks()); } + @Test + public void resolve_addressResolutionError() throws Exception { + DnsNameResolver.enableTxt = true; + when(mockProxyDetector.proxyFor(any(SocketAddress.class))).thenThrow(new IOException()); + RetryingNameResolver resolver = newResolver( + "addr.fake:1234", 443, mockProxyDetector, Stopwatch.createUnstarted()); + DnsNameResolver dnsResolver = (DnsNameResolver) resolver.getRetriedNameResolver(); + dnsResolver.setAddressResolver(new AddressResolver() { + @Override + public List resolveAddress(String host) throws Exception { + return Collections.emptyList(); + } + }); + ResourceResolver mockResourceResolver = mock(ResourceResolver.class); + when(mockResourceResolver.resolveTxt(anyString())) + .thenReturn(Collections.emptyList()); + + dnsResolver.setResourceResolver(mockResourceResolver); + + resolver.start(mockListener); + assertThat(fakeExecutor.runDueTasks()).isEqualTo(1); + + ArgumentCaptor ac = ArgumentCaptor.forClass(ResolutionResult.class); + verify(mockListener).onResult2(ac.capture()); + verifyNoMoreInteractions(mockListener); + assertThat(ac.getValue().getAddressesOrError().getStatus().getCode()).isEqualTo( + Status.UNAVAILABLE.getCode()); + assertThat(ac.getValue().getAddressesOrError().getStatus().getDescription()).isEqualTo( + "Unable to resolve host addr.fake"); + assertThat(ac.getValue().getAddressesOrError().getStatus().getCause()) + .isInstanceOf(IOException.class); + } + // Load balancer rejects the empty addresses. @Test public void resolve_emptyResult_notAccepted() throws Exception { @@ -604,7 +637,7 @@ public class DnsNameResolverTest { ArgumentCaptor ac = ArgumentCaptor.forClass(ResolutionResult.class); verify(mockListener).onResult2(ac.capture()); verifyNoMoreInteractions(mockListener); - assertThat(ac.getValue().getAddresses()).isEmpty(); + assertThat(ac.getValue().getAddressesOrError().getValue()).isEmpty(); assertThat(ac.getValue().getServiceConfig()).isNull(); verify(mockResourceResolver, never()).resolveSrv(anyString()); @@ -632,7 +665,7 @@ public class DnsNameResolverTest { ResolutionResult result = resultCaptor.getValue(); InetSocketAddress resolvedBackendAddr = (InetSocketAddress) Iterables.getOnlyElement( - Iterables.getOnlyElement(result.getAddresses()).getAddresses()); + Iterables.getOnlyElement(result.getAddressesOrError().getValue()).getAddresses()); assertThat(resolvedBackendAddr.getAddress()).isEqualTo(backendAddr); verify(mockAddressResolver).resolveAddress(name); assertThat(result.getServiceConfig()).isNull(); @@ -647,6 +680,7 @@ public class DnsNameResolverTest { AddressResolver mockAddressResolver = mock(AddressResolver.class); when(mockAddressResolver.resolveAddress(anyString())) .thenThrow(new IOException("no addr")); + when(mockListener.onResult2(isA(ResolutionResult.class))).thenReturn(Status.UNAVAILABLE); String name = "foo.googleapis.com"; RetryingNameResolver resolver = newResolver(name, 81); @@ -655,8 +689,8 @@ public class DnsNameResolverTest { dnsResolver.setResourceResolver(null); resolver.start(mockListener); assertEquals(1, fakeExecutor.runDueTasks()); - verify(mockListener).onError(errorCaptor.capture()); - Status errorStatus = errorCaptor.getValue(); + verify(mockListener).onResult2(resultCaptor.capture()); + Status errorStatus = resultCaptor.getValue().getAddressesOrError().getStatus(); assertThat(errorStatus.getCode()).isEqualTo(Code.UNAVAILABLE); assertThat(errorStatus.getCause()).hasMessageThat().contains("no addr"); @@ -704,7 +738,7 @@ public class DnsNameResolverTest { ResolutionResult result = resultCaptor.getValue(); InetSocketAddress resolvedBackendAddr = (InetSocketAddress) Iterables.getOnlyElement( - Iterables.getOnlyElement(result.getAddresses()).getAddresses()); + Iterables.getOnlyElement(result.getAddressesOrError().getValue()).getAddresses()); assertThat(resolvedBackendAddr.getAddress()).isEqualTo(backendAddr); assertThat(result.getServiceConfig().getConfig()).isNotNull(); verify(mockAddressResolver).resolveAddress(name); @@ -720,6 +754,7 @@ public class DnsNameResolverTest { AddressResolver mockAddressResolver = mock(AddressResolver.class); when(mockAddressResolver.resolveAddress(anyString())) .thenThrow(new IOException("no addr")); + when(mockListener.onResult2(isA(ResolutionResult.class))).thenReturn(Status.UNAVAILABLE); String name = "foo.googleapis.com"; ResourceResolver mockResourceResolver = mock(ResourceResolver.class); @@ -729,8 +764,8 @@ public class DnsNameResolverTest { dnsResolver.setResourceResolver(mockResourceResolver); resolver.start(mockListener); assertEquals(1, fakeExecutor.runDueTasks()); - verify(mockListener).onError(errorCaptor.capture()); - Status errorStatus = errorCaptor.getValue(); + verify(mockListener).onResult2(resultCaptor.capture()); + Status errorStatus = resultCaptor.getValue().getAddressesOrError().getStatus(); assertThat(errorStatus.getCode()).isEqualTo(Code.UNAVAILABLE); assertThat(errorStatus.getCause()).hasMessageThat().contains("no addr"); verify(mockResourceResolver, never()).resolveTxt(anyString()); @@ -762,7 +797,7 @@ public class DnsNameResolverTest { ResolutionResult result = resultCaptor.getValue(); InetSocketAddress resolvedBackendAddr = (InetSocketAddress) Iterables.getOnlyElement( - Iterables.getOnlyElement(result.getAddresses()).getAddresses()); + Iterables.getOnlyElement(result.getAddressesOrError().getValue()).getAddresses()); assertThat(resolvedBackendAddr.getAddress()).isEqualTo(backendAddr); verify(mockAddressResolver).resolveAddress(name); assertThat(result.getServiceConfig()).isNull(); @@ -794,7 +829,7 @@ public class DnsNameResolverTest { ResolutionResult result = resultCaptor.getValue(); InetSocketAddress resolvedBackendAddr = (InetSocketAddress) Iterables.getOnlyElement( - Iterables.getOnlyElement(result.getAddresses()).getAddresses()); + Iterables.getOnlyElement(result.getAddressesOrError().getValue()).getAddresses()); assertThat(resolvedBackendAddr.getAddress()).isEqualTo(backendAddr); verify(mockAddressResolver).resolveAddress(name); assertThat(result.getServiceConfig()).isNotNull(); @@ -859,7 +894,7 @@ public class DnsNameResolverTest { assertEquals(1, fakeExecutor.runDueTasks()); verify(mockListener).onResult2(resultCaptor.capture()); - List result = resultCaptor.getValue().getAddresses(); + List result = resultCaptor.getValue().getAddressesOrError().getValue(); assertThat(result).hasSize(1); EquivalentAddressGroup eag = result.get(0); assertThat(eag.getAddresses()).hasSize(1); @@ -1299,9 +1334,9 @@ public class DnsNameResolverTest { private static void assertAnswerMatches( List addrs, int port, ResolutionResult resolutionResult) { - assertThat(resolutionResult.getAddresses()).hasSize(addrs.size()); + assertThat(resolutionResult.getAddressesOrError().getValue()).hasSize(addrs.size()); for (int i = 0; i < addrs.size(); i++) { - EquivalentAddressGroup addrGroup = resolutionResult.getAddresses().get(i); + EquivalentAddressGroup addrGroup = resolutionResult.getAddressesOrError().getValue().get(i); InetSocketAddress socketAddr = (InetSocketAddress) Iterables.getOnlyElement(addrGroup.getAddresses()); assertEquals("Addr " + i, port, socketAddr.getPort()); diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplIdlenessTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplIdlenessTest.java index 90008c1be3..293d0e7096 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplIdlenessTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplIdlenessTest.java @@ -63,6 +63,7 @@ import io.grpc.NameResolver; import io.grpc.NameResolver.ResolutionResult; import io.grpc.NameResolverProvider; import io.grpc.Status; +import io.grpc.StatusOr; import io.grpc.StringMarshaller; import io.grpc.internal.FakeClock.ScheduledTask; import io.grpc.internal.ManagedChannelImplBuilder.UnsupportedClientTransportFactoryBuilder; @@ -615,7 +616,7 @@ public class ManagedChannelImplIdlenessTest { // the NameResolver. ResolutionResult resolutionResult = ResolutionResult.newBuilder() - .setAddresses(servers) + .setAddressesOrError(StatusOr.fromValue(servers)) .setAttributes(Attributes.EMPTY) .build(); nameResolverListenerCaptor.getValue().onResult(resolutionResult); diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java index bea14bcef4..1670009682 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java @@ -116,6 +116,7 @@ import io.grpc.SecurityLevel; import io.grpc.ServerMethodDefinition; import io.grpc.Status; import io.grpc.Status.Code; +import io.grpc.StatusOr; import io.grpc.StringMarshaller; import io.grpc.SynchronizationContext; import io.grpc.internal.ClientTransportFactory.ClientTransportOptions; @@ -1056,7 +1057,7 @@ public class ManagedChannelImplTest { verifyNoMoreInteractions(mockLoadBalancer); } - @Test + @Test public void noMoreCallbackAfterLoadBalancerShutdown_configError() throws InterruptedException { FakeNameResolverFactory nameResolverFactory = new FakeNameResolverFactory.Builder(expectedUri) @@ -1095,7 +1096,10 @@ public class ManagedChannelImplTest { verify(stateListener2).onSubchannelState(stateInfoCaptor.capture()); assertSame(CONNECTING, stateInfoCaptor.getValue().getState()); - resolver.listener.onError(resolutionError); + channel.syncContext.execute(() -> + resolver.listener.onResult2( + ResolutionResult.newBuilder() + .setAddressesOrError(StatusOr.fromStatus(resolutionError)).build())); verify(mockLoadBalancer).handleNameResolutionError(resolutionError); verifyNoMoreInteractions(mockLoadBalancer); @@ -1117,13 +1121,10 @@ public class ManagedChannelImplTest { verifyNoMoreInteractions(stateListener1, stateListener2); // No more callback should be delivered to LoadBalancer after it's shut down - resolver.listener.onResult( - ResolutionResult.newBuilder() - .setAddresses(new ArrayList<>()) - .setServiceConfig( - ConfigOrError.fromError(Status.UNAVAILABLE.withDescription("Resolution failed"))) - .build()); - Thread.sleep(1100); + channel.syncContext.execute(() -> + resolver.listener.onResult2( + ResolutionResult.newBuilder() + .setAddressesOrError(StatusOr.fromStatus(resolutionError)).build())); assertThat(timer.getPendingTasks()).isEmpty(); resolver.resolved(); verifyNoMoreInteractions(mockLoadBalancer); @@ -3286,11 +3287,19 @@ public class ManagedChannelImplTest { assertThat(getStats(channel).channelTrace.events).hasSize(prevSize); prevSize = getStats(channel).channelTrace.events.size(); - nameResolverFactory.resolvers.get(0).listener.onError(Status.INTERNAL); + channel.syncContext.execute(() -> + nameResolverFactory.resolvers.get(0).listener.onResult2( + ResolutionResult.newBuilder() + .setAddressesOrError( + StatusOr.fromStatus(Status.INTERNAL)).build())); assertThat(getStats(channel).channelTrace.events).hasSize(prevSize + 1); prevSize = getStats(channel).channelTrace.events.size(); - nameResolverFactory.resolvers.get(0).listener.onError(Status.INTERNAL); + channel.syncContext.execute(() -> + nameResolverFactory.resolvers.get(0).listener.onResult2( + ResolutionResult.newBuilder() + .setAddressesOrError( + StatusOr.fromStatus(Status.INTERNAL)).build())); assertThat(getStats(channel).channelTrace.events).hasSize(prevSize); prevSize = getStats(channel).channelTrace.events.size(); @@ -4919,7 +4928,10 @@ public class ManagedChannelImplTest { void resolved() { if (error != null) { - listener.onError(error); + syncContext.execute(() -> + listener.onResult2( + ResolutionResult.newBuilder() + .setAddressesOrError(StatusOr.fromStatus(error)).build())); return; } ResolutionResult.Builder builder = diff --git a/grpclb/src/test/java/io/grpc/grpclb/GrpclbNameResolverTest.java b/grpclb/src/test/java/io/grpc/grpclb/GrpclbNameResolverTest.java index c195a78e6f..1aa11ecf9a 100644 --- a/grpclb/src/test/java/io/grpc/grpclb/GrpclbNameResolverTest.java +++ b/grpclb/src/test/java/io/grpc/grpclb/GrpclbNameResolverTest.java @@ -96,7 +96,6 @@ public class GrpclbNameResolverTest { } @Captor private ArgumentCaptor resultCaptor; - @Captor private ArgumentCaptor errorCaptor; @Mock private ServiceConfigParser serviceConfigParser; @Mock private NameResolver.Listener2 mockListener; @@ -154,7 +153,7 @@ public class GrpclbNameResolverTest { verify(mockListener).onResult2(resultCaptor.capture()); ResolutionResult result = resultCaptor.getValue(); - assertThat(result.getAddresses()).isEmpty(); + assertThat(result.getAddressesOrError().getValue()).isEmpty(); assertThat(result.getAttributes()).isEqualTo(Attributes.EMPTY); assertThat(result.getServiceConfig()).isNull(); } @@ -196,7 +195,7 @@ public class GrpclbNameResolverTest { ResolutionResult result = resultCaptor.getValue(); InetSocketAddress resolvedBackendAddr = (InetSocketAddress) Iterables.getOnlyElement( - Iterables.getOnlyElement(result.getAddresses()).getAddresses()); + Iterables.getOnlyElement(result.getAddressesOrError().getValue()).getAddresses()); assertThat(resolvedBackendAddr.getAddress()).isEqualTo(backendAddr); EquivalentAddressGroup resolvedBalancerAddr = Iterables.getOnlyElement(result.getAttributes().get(GrpclbConstants.ATTR_LB_ADDRS)); @@ -227,7 +226,7 @@ public class GrpclbNameResolverTest { assertThat(fakeClock.runDueTasks()).isEqualTo(1); verify(mockListener).onResult2(resultCaptor.capture()); ResolutionResult result = resultCaptor.getValue(); - assertThat(result.getAddresses()) + assertThat(result.getAddressesOrError().getValue()) .containsExactly( new EquivalentAddressGroup(new InetSocketAddress(backendAddr, DEFAULT_PORT))); assertThat(result.getAttributes()).isEqualTo(Attributes.EMPTY); @@ -245,8 +244,8 @@ public class GrpclbNameResolverTest { resolver.start(mockListener); assertThat(fakeClock.runDueTasks()).isEqualTo(1); - verify(mockListener).onError(errorCaptor.capture()); - Status errorStatus = errorCaptor.getValue(); + verify(mockListener).onResult2(resultCaptor.capture()); + Status errorStatus = resultCaptor.getValue().getAddressesOrError().getStatus(); assertThat(errorStatus.getCode()).isEqualTo(Code.UNAVAILABLE); assertThat(errorStatus.getCause()).hasMessageThat().contains("no addr"); } @@ -274,7 +273,7 @@ public class GrpclbNameResolverTest { assertThat(fakeClock.runDueTasks()).isEqualTo(1); verify(mockListener).onResult2(resultCaptor.capture()); ResolutionResult result = resultCaptor.getValue(); - assertThat(result.getAddresses()).isEmpty(); + assertThat(result.getAddressesOrError().getValue()).isEmpty(); EquivalentAddressGroup resolvedBalancerAddr = Iterables.getOnlyElement(result.getAttributes().get(GrpclbConstants.ATTR_LB_ADDRS)); assertThat(resolvedBalancerAddr.getAttributes().get(GrpclbConstants.ATTR_LB_ADDR_AUTHORITY)) @@ -311,7 +310,7 @@ public class GrpclbNameResolverTest { InetSocketAddress resolvedBackendAddr = (InetSocketAddress) Iterables.getOnlyElement( - Iterables.getOnlyElement(result.getAddresses()).getAddresses()); + Iterables.getOnlyElement(result.getAddressesOrError().getValue()).getAddresses()); assertThat(resolvedBackendAddr.getAddress()).isEqualTo(backendAddr); assertThat(result.getAttributes().get(GrpclbConstants.ATTR_LB_ADDRS)).isNull(); verify(mockAddressResolver).resolveAddress(hostName); @@ -335,8 +334,8 @@ public class GrpclbNameResolverTest { resolver.start(mockListener); assertThat(fakeClock.runDueTasks()).isEqualTo(1); - verify(mockListener).onError(errorCaptor.capture()); - Status errorStatus = errorCaptor.getValue(); + verify(mockListener).onResult2(resultCaptor.capture()); + Status errorStatus = resultCaptor.getValue().getAddressesOrError().getStatus(); assertThat(errorStatus.getCode()).isEqualTo(Code.UNAVAILABLE); verify(mockAddressResolver).resolveAddress(hostName); verify(mockResourceResolver, never()).resolveTxt("_grpc_config." + hostName); diff --git a/netty/src/main/java/io/grpc/netty/UdsNameResolver.java b/netty/src/main/java/io/grpc/netty/UdsNameResolver.java index 8fa8ea0625..de14dc8b46 100644 --- a/netty/src/main/java/io/grpc/netty/UdsNameResolver.java +++ b/netty/src/main/java/io/grpc/netty/UdsNameResolver.java @@ -22,16 +22,16 @@ import static com.google.common.base.Preconditions.checkNotNull; import com.google.common.base.Preconditions; import io.grpc.EquivalentAddressGroup; import io.grpc.NameResolver; +import io.grpc.StatusOr; import io.netty.channel.unix.DomainSocketAddress; import java.util.ArrayList; -import java.util.Collections; import java.util.List; final class UdsNameResolver extends NameResolver { private NameResolver.Listener2 listener; private final String authority; - UdsNameResolver(String authority, String targetPath) { + UdsNameResolver(String authority, String targetPath, Args args) { checkArgument(authority == null, "non-null authority not supported"); this.authority = targetPath; } @@ -57,8 +57,8 @@ final class UdsNameResolver extends NameResolver { ResolutionResult.Builder resolutionResultBuilder = ResolutionResult.newBuilder(); List servers = new ArrayList<>(1); servers.add(new EquivalentAddressGroup(new DomainSocketAddress(authority))); - resolutionResultBuilder.setAddresses(Collections.unmodifiableList(servers)); - listener.onResult(resolutionResultBuilder.build()); + resolutionResultBuilder.setAddressesOrError(StatusOr.fromValue(servers)); + listener.onResult2(resolutionResultBuilder.build()); } @Override diff --git a/netty/src/main/java/io/grpc/netty/UdsNameResolverProvider.java b/netty/src/main/java/io/grpc/netty/UdsNameResolverProvider.java index 9f594193b4..fe6300057f 100644 --- a/netty/src/main/java/io/grpc/netty/UdsNameResolverProvider.java +++ b/netty/src/main/java/io/grpc/netty/UdsNameResolverProvider.java @@ -34,7 +34,7 @@ public final class UdsNameResolverProvider extends NameResolverProvider { @Override public UdsNameResolver newNameResolver(URI targetUri, NameResolver.Args args) { if (SCHEME.equals(targetUri.getScheme())) { - return new UdsNameResolver(targetUri.getAuthority(), getTargetPathFromUri(targetUri)); + return new UdsNameResolver(targetUri.getAuthority(), getTargetPathFromUri(targetUri), args); } else { return null; } diff --git a/netty/src/test/java/io/grpc/netty/UdsNameResolverProviderTest.java b/netty/src/test/java/io/grpc/netty/UdsNameResolverProviderTest.java index 6a329c8fc6..9dacf00cfa 100644 --- a/netty/src/test/java/io/grpc/netty/UdsNameResolverProviderTest.java +++ b/netty/src/test/java/io/grpc/netty/UdsNameResolverProviderTest.java @@ -18,10 +18,16 @@ package io.grpc.netty; import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.fail; +import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; +import io.grpc.ChannelLogger; import io.grpc.EquivalentAddressGroup; import io.grpc.NameResolver; +import io.grpc.NameResolver.ServiceConfigParser; +import io.grpc.SynchronizationContext; +import io.grpc.internal.FakeClock; +import io.grpc.internal.GrpcUtil; import io.netty.channel.unix.DomainSocketAddress; import java.net.SocketAddress; import java.net.URI; @@ -39,7 +45,7 @@ import org.mockito.junit.MockitoRule; /** Unit tests for {@link UdsNameResolverProvider}. */ @RunWith(JUnit4.class) public class UdsNameResolverProviderTest { - + private static final int DEFAULT_PORT = 887; @Rule public final MockitoRule mocks = MockitoJUnit.rule(); @@ -51,16 +57,29 @@ public class UdsNameResolverProviderTest { UdsNameResolverProvider udsNameResolverProvider = new UdsNameResolverProvider(); + private final SynchronizationContext syncContext = new SynchronizationContext( + (t, e) -> { + throw new AssertionError(e); + }); + private final FakeClock fakeExecutor = new FakeClock(); + private final NameResolver.Args args = NameResolver.Args.newBuilder() + .setDefaultPort(DEFAULT_PORT) + .setProxyDetector(GrpcUtil.DEFAULT_PROXY_DETECTOR) + .setSynchronizationContext(syncContext) + .setServiceConfigParser(mock(ServiceConfigParser.class)) + .setChannelLogger(mock(ChannelLogger.class)) + .setScheduledExecutorService(fakeExecutor.getScheduledExecutorService()) + .build(); @Test public void testUnixRelativePath() { UdsNameResolver udsNameResolver = - udsNameResolverProvider.newNameResolver(URI.create("unix:sock.sock"), null); + udsNameResolverProvider.newNameResolver(URI.create("unix:sock.sock"), args); assertThat(udsNameResolver).isNotNull(); udsNameResolver.start(mockListener); - verify(mockListener).onResult(resultCaptor.capture()); + verify(mockListener).onResult2(resultCaptor.capture()); NameResolver.ResolutionResult result = resultCaptor.getValue(); - List list = result.getAddresses(); + List list = result.getAddressesOrError().getValue(); assertThat(list).isNotNull(); assertThat(list).hasSize(1); EquivalentAddressGroup eag = list.get(0); @@ -75,12 +94,12 @@ public class UdsNameResolverProviderTest { @Test public void testUnixAbsolutePath() { UdsNameResolver udsNameResolver = - udsNameResolverProvider.newNameResolver(URI.create("unix:/sock.sock"), null); + udsNameResolverProvider.newNameResolver(URI.create("unix:/sock.sock"), args); assertThat(udsNameResolver).isNotNull(); udsNameResolver.start(mockListener); - verify(mockListener).onResult(resultCaptor.capture()); + verify(mockListener).onResult2(resultCaptor.capture()); NameResolver.ResolutionResult result = resultCaptor.getValue(); - List list = result.getAddresses(); + List list = result.getAddressesOrError().getValue(); assertThat(list).isNotNull(); assertThat(list).hasSize(1); EquivalentAddressGroup eag = list.get(0); @@ -95,12 +114,12 @@ public class UdsNameResolverProviderTest { @Test public void testUnixAbsoluteAlternatePath() { UdsNameResolver udsNameResolver = - udsNameResolverProvider.newNameResolver(URI.create("unix:///sock.sock"), null); + udsNameResolverProvider.newNameResolver(URI.create("unix:///sock.sock"), args); assertThat(udsNameResolver).isNotNull(); udsNameResolver.start(mockListener); - verify(mockListener).onResult(resultCaptor.capture()); + verify(mockListener).onResult2(resultCaptor.capture()); NameResolver.ResolutionResult result = resultCaptor.getValue(); - List list = result.getAddresses(); + List list = result.getAddressesOrError().getValue(); assertThat(list).isNotNull(); assertThat(list).hasSize(1); EquivalentAddressGroup eag = list.get(0); @@ -115,7 +134,7 @@ public class UdsNameResolverProviderTest { @Test public void testUnixPathWithAuthority() { try { - udsNameResolverProvider.newNameResolver(URI.create("unix://localhost/sock.sock"), null); + udsNameResolverProvider.newNameResolver(URI.create("unix://localhost/sock.sock"), args); fail("exception expected"); } catch (IllegalArgumentException e) { assertThat(e).hasMessageThat().isEqualTo("non-null authority not supported"); diff --git a/netty/src/test/java/io/grpc/netty/UdsNameResolverTest.java b/netty/src/test/java/io/grpc/netty/UdsNameResolverTest.java index 8eb010e23e..22790a41c7 100644 --- a/netty/src/test/java/io/grpc/netty/UdsNameResolverTest.java +++ b/netty/src/test/java/io/grpc/netty/UdsNameResolverTest.java @@ -18,10 +18,16 @@ package io.grpc.netty; import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.fail; +import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; +import io.grpc.ChannelLogger; import io.grpc.EquivalentAddressGroup; import io.grpc.NameResolver; +import io.grpc.NameResolver.ServiceConfigParser; +import io.grpc.SynchronizationContext; +import io.grpc.internal.FakeClock; +import io.grpc.internal.GrpcUtil; import io.netty.channel.unix.DomainSocketAddress; import java.net.SocketAddress; import java.util.List; @@ -41,7 +47,20 @@ public class UdsNameResolverTest { @Rule public final MockitoRule mocks = MockitoJUnit.rule(); - + private static final int DEFAULT_PORT = 887; + private final FakeClock fakeExecutor = new FakeClock(); + private final SynchronizationContext syncContext = new SynchronizationContext( + (t, e) -> { + throw new AssertionError(e); + }); + private final NameResolver.Args args = NameResolver.Args.newBuilder() + .setDefaultPort(DEFAULT_PORT) + .setProxyDetector(GrpcUtil.DEFAULT_PROXY_DETECTOR) + .setSynchronizationContext(syncContext) + .setServiceConfigParser(mock(ServiceConfigParser.class)) + .setChannelLogger(mock(ChannelLogger.class)) + .setScheduledExecutorService(fakeExecutor.getScheduledExecutorService()) + .build(); @Mock private NameResolver.Listener2 mockListener; @@ -52,11 +71,11 @@ public class UdsNameResolverTest { @Test public void testValidTargetPath() { - udsNameResolver = new UdsNameResolver(null, "sock.sock"); + udsNameResolver = new UdsNameResolver(null, "sock.sock", args); udsNameResolver.start(mockListener); - verify(mockListener).onResult(resultCaptor.capture()); + verify(mockListener).onResult2(resultCaptor.capture()); NameResolver.ResolutionResult result = resultCaptor.getValue(); - List list = result.getAddresses(); + List list = result.getAddressesOrError().getValue(); assertThat(list).isNotNull(); assertThat(list).hasSize(1); EquivalentAddressGroup eag = list.get(0); @@ -72,7 +91,7 @@ public class UdsNameResolverTest { @Test public void testNonNullAuthority() { try { - udsNameResolver = new UdsNameResolver("authority", "sock.sock"); + udsNameResolver = new UdsNameResolver("authority", "sock.sock", args); fail("exception expected"); } catch (IllegalArgumentException e) { assertThat(e).hasMessageThat().isEqualTo("non-null authority not supported"); diff --git a/testing/src/main/java/io/grpc/internal/testing/FakeNameResolverProvider.java b/testing/src/main/java/io/grpc/internal/testing/FakeNameResolverProvider.java index 4664dbcc43..c77f7f8945 100644 --- a/testing/src/main/java/io/grpc/internal/testing/FakeNameResolverProvider.java +++ b/testing/src/main/java/io/grpc/internal/testing/FakeNameResolverProvider.java @@ -21,6 +21,7 @@ import io.grpc.EquivalentAddressGroup; import io.grpc.NameResolver; import io.grpc.NameResolverProvider; import io.grpc.Status; +import io.grpc.StatusOr; import java.net.SocketAddress; import java.net.URI; import java.util.Collection; @@ -81,9 +82,10 @@ public final class FakeNameResolverProvider extends NameResolverProvider { if (shutdown) { listener.onError(Status.FAILED_PRECONDITION.withDescription("Resolver is shutdown")); } else { - listener.onResult( + listener.onResult2( ResolutionResult.newBuilder() - .setAddresses(ImmutableList.of(new EquivalentAddressGroup(address))) + .setAddressesOrError( + StatusOr.fromValue(ImmutableList.of(new EquivalentAddressGroup(address)))) .build()); } } diff --git a/xds/src/test/java/io/grpc/xds/ClusterResolverLoadBalancerTest.java b/xds/src/test/java/io/grpc/xds/ClusterResolverLoadBalancerTest.java index d3c6bc8f9a..0ecd77b12c 100644 --- a/xds/src/test/java/io/grpc/xds/ClusterResolverLoadBalancerTest.java +++ b/xds/src/test/java/io/grpc/xds/ClusterResolverLoadBalancerTest.java @@ -52,6 +52,7 @@ import io.grpc.NameResolverProvider; import io.grpc.NameResolverRegistry; import io.grpc.Status; import io.grpc.Status.Code; +import io.grpc.StatusOr; import io.grpc.SynchronizationContext; import io.grpc.internal.BackoffPolicy; import io.grpc.internal.FakeClock; @@ -1306,7 +1307,8 @@ public class ClusterResolverLoadBalancerTest { } private void deliverEndpointAddresses(List addresses) { - listener.onResult(ResolutionResult.newBuilder().setAddresses(addresses).build()); + listener.onResult(ResolutionResult.newBuilder() + .setAddressesOrError(StatusOr.fromValue(addresses)).build()); } private void deliverError(Status error) { diff --git a/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java b/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java index 24c2a43b83..76b92cd8c0 100644 --- a/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java +++ b/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java @@ -737,7 +737,7 @@ public class XdsNameResolverTest { ImmutableMap.of()))); verify(mockListener).onResult(resolutionResultCaptor.capture()); ResolutionResult result = resolutionResultCaptor.getValue(); - assertThat(result.getAddresses()).isEmpty(); + assertThat(result.getAddressesOrError().getValue()).isEmpty(); assertServiceConfigForLoadBalancingConfig( Collections.singletonList(cluster2), (Map) result.getServiceConfig().getConfig()); @@ -1071,7 +1071,7 @@ public class XdsNameResolverTest { ImmutableMap.of()))); verify(mockListener).onResult(resolutionResultCaptor.capture()); ResolutionResult result = resolutionResultCaptor.getValue(); - assertThat(result.getAddresses()).isEmpty(); + assertThat(result.getAddressesOrError().getValue()).isEmpty(); assertServiceConfigForLoadBalancingConfig( Arrays.asList(cluster1, cluster2), (Map) result.getServiceConfig().getConfig()); assertThat(result.getAttributes().get(InternalXdsAttributes.XDS_CLIENT_POOL)).isNotNull(); @@ -1100,7 +1100,7 @@ public class XdsNameResolverTest { ImmutableMap.of()))); verify(mockListener).onResult(resolutionResultCaptor.capture()); ResolutionResult result = resolutionResultCaptor.getValue(); - assertThat(result.getAddresses()).isEmpty(); + assertThat(result.getAddressesOrError().getValue()).isEmpty(); @SuppressWarnings("unchecked") Map resultServiceConfig = (Map) result.getServiceConfig().getConfig(); List> rawLbConfigs = @@ -1181,7 +1181,7 @@ public class XdsNameResolverTest { private void assertEmptyResolutionResult(String resource) { verify(mockListener).onResult(resolutionResultCaptor.capture()); ResolutionResult result = resolutionResultCaptor.getValue(); - assertThat(result.getAddresses()).isEmpty(); + assertThat(result.getAddressesOrError().getValue()).isEmpty(); assertThat((Map) result.getServiceConfig().getConfig()).isEmpty(); InternalConfigSelector configSelector = result.getAttributes().get(InternalConfigSelector.KEY); Result configResult = configSelector.selectConfig( @@ -1260,7 +1260,7 @@ public class XdsNameResolverTest { ImmutableMap.of()))); verify(mockListener).onResult(resolutionResultCaptor.capture()); ResolutionResult result = resolutionResultCaptor.getValue(); - assertThat(result.getAddresses()).isEmpty(); + assertThat(result.getAddressesOrError().getValue()).isEmpty(); assertServiceConfigForLoadBalancingConfig( Arrays.asList(cluster1, cluster2), (Map) result.getServiceConfig().getConfig()); assertThat(result.getAttributes().get(InternalXdsAttributes.XDS_CLIENT_POOL)).isNotNull(); diff --git a/xds/src/test/java/io/grpc/xds/XdsSecurityClientServerTest.java b/xds/src/test/java/io/grpc/xds/XdsSecurityClientServerTest.java index 2c349eec4a..c6b8e7515b 100644 --- a/xds/src/test/java/io/grpc/xds/XdsSecurityClientServerTest.java +++ b/xds/src/test/java/io/grpc/xds/XdsSecurityClientServerTest.java @@ -43,6 +43,7 @@ import io.grpc.NameResolverRegistry; import io.grpc.Server; import io.grpc.ServerCredentials; import io.grpc.Status; +import io.grpc.StatusOr; import io.grpc.StatusRuntimeException; import io.grpc.stub.StreamObserver; import io.grpc.testing.GrpcCleanupRule; @@ -520,7 +521,8 @@ public class XdsSecurityClientServerTest { } void resolved() { - ResolutionResult.Builder builder = ResolutionResult.newBuilder().setAddresses(servers); + ResolutionResult.Builder builder = ResolutionResult.newBuilder() + .setAddressesOrError(StatusOr.fromValue(servers)); listener.onResult(builder.build()); }