From 7f6453a7d04ca5acd29cb6a4766de3efef4997eb Mon Sep 17 00:00:00 2001 From: reggiemcdonald Date: Thu, 23 Apr 2020 01:06:40 -0700 Subject: [PATCH 1/6] inprocess,internal: add ability to pass status cause to client --- .../inprocess/InProcessChannelBuilder.java | 20 +++++++-- .../io/grpc/inprocess/InProcessTransport.java | 37 +++++++++++++--- .../java/io/grpc/internal/ServerImpl.java | 17 ++++--- .../InProcessTransportTestWithCause.java | 44 +++++++++++++++++++ .../grpc/internal/AbstractTransportTest.java | 30 ++++++------- .../java/io/grpc/internal/ServerImplTest.java | 5 ++- 6 files changed, 116 insertions(+), 37 deletions(-) create mode 100644 core/src/test/java/io/grpc/inprocess/InProcessTransportTestWithCause.java diff --git a/core/src/main/java/io/grpc/inprocess/InProcessChannelBuilder.java b/core/src/main/java/io/grpc/inprocess/InProcessChannelBuilder.java index 792257758b0..08693e9e3d4 100644 --- a/core/src/main/java/io/grpc/inprocess/InProcessChannelBuilder.java +++ b/core/src/main/java/io/grpc/inprocess/InProcessChannelBuilder.java @@ -70,6 +70,7 @@ public static InProcessChannelBuilder forAddress(String name, int port) { private final String name; private ScheduledExecutorService scheduledExecutorService; private int maxInboundMetadataSize = Integer.MAX_VALUE; + private boolean transportIncludeStatusCause = false; private InProcessChannelBuilder(String name) { super(new InProcessSocketAddress(name), "localhost"); @@ -157,11 +158,22 @@ public InProcessChannelBuilder maxInboundMetadataSize(int bytes) { return this; } + /** + * Sets whether to override the default behaviour of InProcessTransport to include + * the cause of the status. + * @param enable whether to include cause in status + * @return this + */ + public InProcessChannelBuilder transportIncludeStatusCause(boolean enable) { + this.transportIncludeStatusCause = enable; + return this; + } + @Override @Internal protected ClientTransportFactory buildTransportFactory() { return new InProcessClientTransportFactory( - name, scheduledExecutorService, maxInboundMetadataSize); + name, scheduledExecutorService, maxInboundMetadataSize, transportIncludeStatusCause); } /** @@ -173,16 +185,18 @@ static final class InProcessClientTransportFactory implements ClientTransportFac private final boolean useSharedTimer; private final int maxInboundMetadataSize; private boolean closed; + private boolean includeCauseWithStatus; private InProcessClientTransportFactory( String name, @Nullable ScheduledExecutorService scheduledExecutorService, - int maxInboundMetadataSize) { + int maxInboundMetadataSize, boolean includeCauseWithStatus) { this.name = name; useSharedTimer = scheduledExecutorService == null; timerService = useSharedTimer ? SharedResourceHolder.get(GrpcUtil.TIMER_SERVICE) : scheduledExecutorService; this.maxInboundMetadataSize = maxInboundMetadataSize; + this.includeCauseWithStatus = includeCauseWithStatus; } @Override @@ -194,7 +208,7 @@ public ConnectionClientTransport newClientTransport( // TODO(carl-mastrangelo): Pass channelLogger in. return new InProcessTransport( name, maxInboundMetadataSize, options.getAuthority(), options.getUserAgent(), - options.getEagAttributes()); + options.getEagAttributes(), includeCauseWithStatus); } @Override diff --git a/core/src/main/java/io/grpc/inprocess/InProcessTransport.java b/core/src/main/java/io/grpc/inprocess/InProcessTransport.java index 448d6913066..2db3ebc4634 100644 --- a/core/src/main/java/io/grpc/inprocess/InProcessTransport.java +++ b/core/src/main/java/io/grpc/inprocess/InProcessTransport.java @@ -83,6 +83,7 @@ final class InProcessTransport implements ServerTransport, ConnectionClientTrans private final String userAgent; private final Optional optionalServerListener; private int serverMaxInboundMetadataSize; + private boolean includeCauseWithStatus; private ObjectPool serverSchedulerPool; private ScheduledExecutorService serverScheduler; private ServerTransportListener serverTransportListener; @@ -115,7 +116,8 @@ protected void handleNotInUse() { }; private InProcessTransport(String name, int maxInboundMetadataSize, String authority, - String userAgent, Attributes eagAttrs, Optional optionalServerListener) { + String userAgent, Attributes eagAttrs, + Optional optionalServerListener, boolean includeCauseWithStatus) { this.name = name; this.clientMaxInboundMetadataSize = maxInboundMetadataSize; this.authority = authority; @@ -129,13 +131,21 @@ private InProcessTransport(String name, int maxInboundMetadataSize, String autho .build(); this.optionalServerListener = optionalServerListener; logId = InternalLogId.allocate(getClass(), name); + this.includeCauseWithStatus = includeCauseWithStatus; } public InProcessTransport( String name, int maxInboundMetadataSize, String authority, String userAgent, Attributes eagAttrs) { this(name, maxInboundMetadataSize, authority, userAgent, eagAttrs, - Optional.absent()); + Optional.absent(), false); + } + + public InProcessTransport( + String name, int maxInboundMetadataSize, String authority, String userAgent, + Attributes eagAttrs, boolean includeCauseWithStatus) { + this(name, maxInboundMetadataSize, authority, userAgent, eagAttrs, + Optional.absent(), includeCauseWithStatus); } InProcessTransport( @@ -143,12 +153,17 @@ public InProcessTransport( Attributes eagAttrs, ObjectPool serverSchedulerPool, List serverStreamTracerFactories, ServerListener serverListener) { - this(name, maxInboundMetadataSize, authority, userAgent, eagAttrs, Optional.of(serverListener)); + this(name, maxInboundMetadataSize, authority, userAgent, eagAttrs, + Optional.of(serverListener), false); this.serverMaxInboundMetadataSize = maxInboundMetadataSize; this.serverSchedulerPool = serverSchedulerPool; this.serverStreamTracerFactories = serverStreamTracerFactories; } + public void includeStatusWithCause(boolean enable) { + this.includeCauseWithStatus = enable; + } + @CheckReturnValue @Override public synchronized Runnable start(ManagedClientTransport.Listener listener) { @@ -323,6 +338,10 @@ public ListenableFuture getStats() { return ret; } + public boolean getIncludeCauseWithStatus() { + return includeCauseWithStatus; + } + private synchronized void notifyShutdown(Status s) { if (shutdown) { return; @@ -564,7 +583,7 @@ public void close(Status status, Metadata trailers) { /** clientStream.serverClosed() must be called before this method */ private void notifyClientClose(Status status, Metadata trailers) { - Status clientStatus = stripCause(status); + Status clientStatus = stripCause(status, includeCauseWithStatus); synchronized (this) { if (closed) { return; @@ -744,7 +763,7 @@ public synchronized boolean isReady() { // Must be thread-safe for shutdownNow() @Override public void cancel(Status reason) { - Status serverStatus = stripCause(reason); + Status serverStatus = stripCause(reason, includeCauseWithStatus); if (!internalCancel(serverStatus, serverStatus)) { return; } @@ -849,13 +868,17 @@ public void appendTimeoutInsight(InsightBuilder insight) { *

This is, so that the InProcess transport behaves in the same way as the other transports, * when exchanging statuses between client and server and vice versa. */ - private static Status stripCause(Status status) { + private static Status stripCause(Status status, boolean includeCauseWithStatus) { if (status == null) { return null; } - return Status + Status clientStatus = Status .fromCodeValue(status.getCode().value()) .withDescription(status.getDescription()); + if (includeCauseWithStatus) { + clientStatus = clientStatus.withCause(status.getCause()); + } + return clientStatus; } private static class SingleMessageProducer implements StreamListener.MessageProducer { diff --git a/core/src/main/java/io/grpc/internal/ServerImpl.java b/core/src/main/java/io/grpc/internal/ServerImpl.java index 6e9cb9bf5ec..9984f2f282e 100644 --- a/core/src/main/java/io/grpc/internal/ServerImpl.java +++ b/core/src/main/java/io/grpc/internal/ServerImpl.java @@ -161,7 +161,6 @@ public final class ServerImpl extends io.grpc.Server implements InternalInstrume this.channelz = builder.channelz; this.serverCallTracer = builder.callTracerFactory.create(); this.ticker = checkNotNull(builder.ticker, "ticker"); - channelz.addServer(this); } @@ -759,9 +758,9 @@ void setListener(ServerStreamListener listener) { /** * Like {@link ServerCall#close(Status, Metadata)}, but thread-safe for internal use. */ - private void internalClose() { + private void internalClose(Throwable t) { // TODO(ejona86): this is not thread-safe :) - stream.close(Status.UNKNOWN, new Metadata()); + stream.close(Status.UNKNOWN.withCause(t), new Metadata()); } @Override @@ -782,10 +781,10 @@ public void runInContext() { try { getListener().messagesAvailable(producer); } catch (RuntimeException e) { - internalClose(); + internalClose(e); throw e; } catch (Error e) { - internalClose(); + internalClose(e); throw e; } finally { PerfMark.stopTask("ServerCallListener(app).messagesAvailable", tag); @@ -817,10 +816,10 @@ public void runInContext() { try { getListener().halfClosed(); } catch (RuntimeException e) { - internalClose(); + internalClose(e); throw e; } catch (Error e) { - internalClose(); + internalClose(e); throw e; } finally { PerfMark.stopTask("ServerCallListener(app).halfClosed", tag); @@ -891,10 +890,10 @@ public void runInContext() { try { getListener().onReady(); } catch (RuntimeException e) { - internalClose(); + internalClose(e); throw e; } catch (Error e) { - internalClose(); + internalClose(e); throw e; } finally { PerfMark.stopTask("ServerCallListener(app).onReady", tag); diff --git a/core/src/test/java/io/grpc/inprocess/InProcessTransportTestWithCause.java b/core/src/test/java/io/grpc/inprocess/InProcessTransportTestWithCause.java new file mode 100644 index 00000000000..08359613419 --- /dev/null +++ b/core/src/test/java/io/grpc/inprocess/InProcessTransportTestWithCause.java @@ -0,0 +1,44 @@ +/* + * Copyright 2020 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.inprocess; + +import static org.junit.Assert.assertEquals; + +import io.grpc.Status; +import io.grpc.internal.InternalServer; +import io.grpc.internal.ManagedClientTransport; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class InProcessTransportTestWithCause extends InProcessTransportTest { + + @Override + protected ManagedClientTransport newClientTransport(InternalServer server) { + InProcessTransport transport = (InProcessTransport) super.newClientTransport(server); + transport.includeStatusWithCause(true); + return transport; + } + + @Override + protected void checkClientStatus(Status expectedStatus, Status clientStreamStatus) { + assertEquals(expectedStatus.getCode(), clientStreamStatus.getCode()); + assertEquals(expectedStatus.getDescription(), clientStreamStatus.getDescription()); + // Transport has been configured to pass the cause + assertEquals(expectedStatus.getCause(), clientStreamStatus.getCause()); + } +} diff --git a/core/src/test/java/io/grpc/internal/AbstractTransportTest.java b/core/src/test/java/io/grpc/internal/AbstractTransportTest.java index adbefbedef4..509825010c6 100644 --- a/core/src/test/java/io/grpc/internal/AbstractTransportTest.java +++ b/core/src/test/java/io/grpc/internal/AbstractTransportTest.java @@ -1058,9 +1058,7 @@ public void earlyServerClose_withServerHeaders() throws Exception { Metadata clientStreamTrailers = clientStreamListener.trailers.get(TIMEOUT_MS, TimeUnit.MILLISECONDS); assertNotNull(clientStreamTrailers); - assertEquals(status.getCode(), clientStreamStatus.getCode()); - assertEquals("Hello. Goodbye.", clientStreamStatus.getDescription()); - assertNull(clientStreamStatus.getCause()); + checkClientStatus(status, clientStreamStatus); assertTrue(clientStreamTracer1.getOutboundHeaders()); assertTrue(clientStreamTracer1.getInboundHeaders()); assertSame(clientStreamTrailers, clientStreamTracer1.getInboundTrailers()); @@ -1097,10 +1095,7 @@ public void earlyServerClose_noServerHeaders() throws Exception { Status clientStreamStatus = clientStreamListener.status.get(TIMEOUT_MS, TimeUnit.MILLISECONDS); Metadata clientStreamTrailers = clientStreamListener.trailers.get(TIMEOUT_MS, TimeUnit.MILLISECONDS); - assertEquals(status.getCode(), clientStreamStatus.getCode()); - assertEquals("Hellogoodbye", clientStreamStatus.getDescription()); - // Cause should not be transmitted to the client. - assertNull(clientStreamStatus.getCause()); + checkClientStatus(status, clientStreamStatus); assertEquals( Lists.newArrayList(trailers.getAll(asciiKey)), Lists.newArrayList(clientStreamTrailers.getAll(asciiKey))); @@ -1138,9 +1133,7 @@ public void earlyServerClose_serverFailure() throws Exception { Metadata clientStreamTrailers = clientStreamListener.trailers.get(TIMEOUT_MS, TimeUnit.MILLISECONDS); assertNotNull(clientStreamTrailers); - assertEquals(status.getCode(), clientStreamStatus.getCode()); - assertEquals(status.getDescription(), clientStreamStatus.getDescription()); - assertNull(clientStreamStatus.getCause()); + checkClientStatus(status, clientStreamStatus); assertTrue(clientStreamTracer1.getOutboundHeaders()); assertSame(clientStreamTrailers, clientStreamTracer1.getInboundTrailers()); assertSame(clientStreamStatus, clientStreamTracer1.getStatus()); @@ -1188,9 +1181,7 @@ public void closed( Metadata clientStreamTrailers = clientStreamListener.trailers.get(TIMEOUT_MS, TimeUnit.MILLISECONDS); assertNotNull(clientStreamTrailers); - assertEquals(status.getCode(), clientStreamStatus.getCode()); - assertEquals(status.getDescription(), clientStreamStatus.getDescription()); - assertNull(clientStreamStatus.getCause()); + checkClientStatus(status, clientStreamStatus); assertTrue(clientStreamTracer1.getOutboundHeaders()); assertSame(clientStreamTrailers, clientStreamTracer1.getInboundTrailers()); assertSame(clientStreamStatus, clientStreamTracer1.getStatus()); @@ -1219,9 +1210,6 @@ public void clientCancel() throws Exception { assertNotNull(clientStreamListener.trailers.get(TIMEOUT_MS, TimeUnit.MILLISECONDS)); Status serverStatus = serverStreamListener.status.get(TIMEOUT_MS, TimeUnit.MILLISECONDS); assertNotEquals(Status.Code.OK, serverStatus.getCode()); - // Cause should not be transmitted between client and server - assertNull(serverStatus.getCause()); - clientStream.cancel(status); assertTrue(clientStreamTracer1.getOutboundHeaders()); assertNull(clientStreamTracer1.getInboundTrailers()); @@ -2072,6 +2060,16 @@ private static void assertStatusEquals(Status expected, Status actual) { } } + /** + * Verifies that the client status is as expected. By default, the code and description should + * be present, and the cause should be stripped away. + */ + protected void checkClientStatus(Status expectedStatus, Status clientStreamStatus) { + assertEquals(expectedStatus.getCode(), clientStreamStatus.getCode()); + assertEquals(expectedStatus.getDescription(), clientStreamStatus.getDescription()); + assertNull(clientStreamStatus.getCause()); + } + private static boolean waitForFuture(Future future, long timeout, TimeUnit unit) throws InterruptedException { try { diff --git a/core/src/test/java/io/grpc/internal/ServerImplTest.java b/core/src/test/java/io/grpc/internal/ServerImplTest.java index 2fe2692bcdb..51191e1debf 100644 --- a/core/src/test/java/io/grpc/internal/ServerImplTest.java +++ b/core/src/test/java/io/grpc/internal/ServerImplTest.java @@ -1440,8 +1440,9 @@ private void verifyExecutorsReturned() { private void ensureServerStateNotLeaked() { verify(stream).close(statusCaptor.capture(), metadataCaptor.capture()); - assertEquals(Status.UNKNOWN, statusCaptor.getValue()); - assertNull(statusCaptor.getValue().getCause()); + assertEquals(Status.UNKNOWN.getCode(), statusCaptor.getValue().getCode()); + // Used in InProcessTransport when set to include the cause with the status + assertNotNull(statusCaptor.getValue().getCause()); assertTrue(metadataCaptor.getValue().keys().isEmpty()); } From c9270e24ea716af9f4c5995674cbd4db6066ce1b Mon Sep 17 00:00:00 2001 From: reggiemcdonald Date: Sun, 26 Apr 2020 00:11:59 -0700 Subject: [PATCH 2/6] inprocess: improve javadocs and method name for InProcessChannelBuilder.propagateCauseWithStatus Addresses comment in PR to change method name tranportIncludeStatusCause to something more descriptive: propagateCauseWithStatus. Updates javadoc to better describe what the propagateCauseWithStatus setting does - including the default behaviour of the method. --- .../io/grpc/inprocess/InProcessChannelBuilder.java | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/io/grpc/inprocess/InProcessChannelBuilder.java b/core/src/main/java/io/grpc/inprocess/InProcessChannelBuilder.java index 08693e9e3d4..ba666115117 100644 --- a/core/src/main/java/io/grpc/inprocess/InProcessChannelBuilder.java +++ b/core/src/main/java/io/grpc/inprocess/InProcessChannelBuilder.java @@ -159,12 +159,20 @@ public InProcessChannelBuilder maxInboundMetadataSize(int bytes) { } /** - * Sets whether to override the default behaviour of InProcessTransport to include - * the cause of the status. + * Sets whether to include the cause with the status that is propagated + * forward from the InProcessTransport. This was added to make debugging failing + * tests easier by showing the cause of the status. + * + *

By default, this is set to false. + * A default value of false maintains consistency with other transports which strip causal + * information from the status to avoid leaking information to untrusted clients, and + * to avoid sharing language-specific information with the client. + * For the in-process implementation, this is not a concern. + * * @param enable whether to include cause in status * @return this */ - public InProcessChannelBuilder transportIncludeStatusCause(boolean enable) { + public InProcessChannelBuilder propagateCauseWithStatus(boolean enable) { this.transportIncludeStatusCause = enable; return this; } From e85eac9c20848cb43d5dd3b7b0641522e592e68a Mon Sep 17 00:00:00 2001 From: reggiemcdonald Date: Sun, 26 Apr 2020 02:10:57 -0700 Subject: [PATCH 3/6] inprocess: remove unused method Addressing PR feedback, this commit removes the getter from InProcessTransport that is not used in the codebase. --- core/src/main/java/io/grpc/inprocess/InProcessTransport.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/core/src/main/java/io/grpc/inprocess/InProcessTransport.java b/core/src/main/java/io/grpc/inprocess/InProcessTransport.java index 2db3ebc4634..9420e392aa4 100644 --- a/core/src/main/java/io/grpc/inprocess/InProcessTransport.java +++ b/core/src/main/java/io/grpc/inprocess/InProcessTransport.java @@ -338,10 +338,6 @@ public ListenableFuture getStats() { return ret; } - public boolean getIncludeCauseWithStatus() { - return includeCauseWithStatus; - } - private synchronized void notifyShutdown(Status s) { if (shutdown) { return; From 2efad75c7b0895419d8465a1a410acb8c0b28569 Mon Sep 17 00:00:00 2001 From: reggiemcdonald Date: Sun, 26 Apr 2020 14:22:26 -0700 Subject: [PATCH 4/6] inprocess, internal: creates single test to check for propagating cause with status To address a PR comment, this commit makes the includeCauseWithStatus field of the InProcessTransport final, and adds a single test case to verify that the cause is propagated properly when configured. The field server of AbstractTransportTest had to be made protected so that it could be nullified in the test case for the after each hook. --- .../io/grpc/inprocess/InProcessTransport.java | 13 +--- .../inprocess/InProcessTransportTest.java | 59 ++++++++++++++++++- .../InProcessTransportTestWithCause.java | 44 -------------- .../grpc/internal/AbstractTransportTest.java | 4 +- 4 files changed, 61 insertions(+), 59 deletions(-) delete mode 100644 core/src/test/java/io/grpc/inprocess/InProcessTransportTestWithCause.java diff --git a/core/src/main/java/io/grpc/inprocess/InProcessTransport.java b/core/src/main/java/io/grpc/inprocess/InProcessTransport.java index 9420e392aa4..998dbfc0872 100644 --- a/core/src/main/java/io/grpc/inprocess/InProcessTransport.java +++ b/core/src/main/java/io/grpc/inprocess/InProcessTransport.java @@ -83,7 +83,7 @@ final class InProcessTransport implements ServerTransport, ConnectionClientTrans private final String userAgent; private final Optional optionalServerListener; private int serverMaxInboundMetadataSize; - private boolean includeCauseWithStatus; + private final boolean includeCauseWithStatus; private ObjectPool serverSchedulerPool; private ScheduledExecutorService serverScheduler; private ServerTransportListener serverTransportListener; @@ -134,13 +134,6 @@ private InProcessTransport(String name, int maxInboundMetadataSize, String autho this.includeCauseWithStatus = includeCauseWithStatus; } - public InProcessTransport( - String name, int maxInboundMetadataSize, String authority, String userAgent, - Attributes eagAttrs) { - this(name, maxInboundMetadataSize, authority, userAgent, eagAttrs, - Optional.absent(), false); - } - public InProcessTransport( String name, int maxInboundMetadataSize, String authority, String userAgent, Attributes eagAttrs, boolean includeCauseWithStatus) { @@ -160,10 +153,6 @@ public InProcessTransport( this.serverStreamTracerFactories = serverStreamTracerFactories; } - public void includeStatusWithCause(boolean enable) { - this.includeCauseWithStatus = enable; - } - @CheckReturnValue @Override public synchronized Runnable start(ManagedClientTransport.Listener listener) { diff --git a/core/src/test/java/io/grpc/inprocess/InProcessTransportTest.java b/core/src/test/java/io/grpc/inprocess/InProcessTransportTest.java index 3bd46cd384d..f7e325ad5a9 100644 --- a/core/src/test/java/io/grpc/inprocess/InProcessTransportTest.java +++ b/core/src/test/java/io/grpc/inprocess/InProcessTransportTest.java @@ -16,14 +16,30 @@ package io.grpc.inprocess; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.fail; + import com.google.common.collect.ImmutableList; +import io.grpc.CallOptions; +import io.grpc.ManagedChannel; +import io.grpc.Metadata; +import io.grpc.Server; +import io.grpc.ServerCall; +import io.grpc.ServerCallHandler; +import io.grpc.ServerServiceDefinition; import io.grpc.ServerStreamTracer; +import io.grpc.Status; +import io.grpc.StatusRuntimeException; import io.grpc.internal.AbstractTransportTest; import io.grpc.internal.GrpcUtil; import io.grpc.internal.InternalServer; import io.grpc.internal.ManagedClientTransport; +import io.grpc.stub.ClientCalls; +import io.grpc.testing.GrpcCleanupRule; +import io.grpc.testing.TestMethodDescriptors; import java.util.List; import org.junit.Ignore; +import org.junit.Rule; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -35,6 +51,9 @@ public class InProcessTransportTest extends AbstractTransportTest { private static final String AUTHORITY = "a-testing-authority"; private static final String USER_AGENT = "a-testing-user-agent"; + @Rule + public final GrpcCleanupRule grpcCleanupRule = new GrpcCleanupRule(); + @Override protected List newServer( List streamTracerFactories) { @@ -59,7 +78,7 @@ protected String testAuthority(InternalServer server) { protected ManagedClientTransport newClientTransport(InternalServer server) { return new InProcessTransport( TRANSPORT_NAME, GrpcUtil.DEFAULT_MAX_HEADER_LIST_SIZE, testAuthority(server), USER_AGENT, - eagAttrs()); + eagAttrs(), false); } @Override @@ -75,4 +94,42 @@ protected boolean sizesReported() { public void socketStats() throws Exception { // test does not apply to in-process } + + @Test + public void causeShouldBePropagatedWithStatus() throws Exception { + server = null; + String failingServerName = "server_foo"; + String serviceFoo = "service_foo"; + final Status s = Status.INTERNAL.withCause(new Throwable("failing server exception")); + ServerServiceDefinition definition = ServerServiceDefinition.builder(serviceFoo) + .addMethod(TestMethodDescriptors.voidMethod(), new ServerCallHandler() { + @Override + public ServerCall.Listener startCall( + ServerCall call, Metadata headers) { + call.close(s, new Metadata()); + return new ServerCall.Listener() {}; + } + }) + .build(); + Server failingServer = InProcessServerBuilder + .forName(failingServerName) + .addService(definition) + .directExecutor() + .build() + .start(); + grpcCleanupRule.register(failingServer); + ManagedChannel channel = InProcessChannelBuilder + .forName(failingServerName) + .propagateCauseWithStatus(true) + .build(); + grpcCleanupRule.register(channel); + try { + ClientCalls.blockingUnaryCall(channel, TestMethodDescriptors.voidMethod(), + CallOptions.DEFAULT, null); + fail("exception should have been thrown"); + } catch (StatusRuntimeException e) { + // When propagateCauseWithStatus is true, the cause should be sent forward + assertEquals(s.getCause(), e.getCause()); + } + } } diff --git a/core/src/test/java/io/grpc/inprocess/InProcessTransportTestWithCause.java b/core/src/test/java/io/grpc/inprocess/InProcessTransportTestWithCause.java deleted file mode 100644 index 08359613419..00000000000 --- a/core/src/test/java/io/grpc/inprocess/InProcessTransportTestWithCause.java +++ /dev/null @@ -1,44 +0,0 @@ -/* - * Copyright 2020 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.inprocess; - -import static org.junit.Assert.assertEquals; - -import io.grpc.Status; -import io.grpc.internal.InternalServer; -import io.grpc.internal.ManagedClientTransport; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; - -@RunWith(JUnit4.class) -public class InProcessTransportTestWithCause extends InProcessTransportTest { - - @Override - protected ManagedClientTransport newClientTransport(InternalServer server) { - InProcessTransport transport = (InProcessTransport) super.newClientTransport(server); - transport.includeStatusWithCause(true); - return transport; - } - - @Override - protected void checkClientStatus(Status expectedStatus, Status clientStreamStatus) { - assertEquals(expectedStatus.getCode(), clientStreamStatus.getCode()); - assertEquals(expectedStatus.getDescription(), clientStreamStatus.getDescription()); - // Transport has been configured to pass the cause - assertEquals(expectedStatus.getCause(), clientStreamStatus.getCause()); - } -} diff --git a/core/src/test/java/io/grpc/internal/AbstractTransportTest.java b/core/src/test/java/io/grpc/internal/AbstractTransportTest.java index 509825010c6..e87829960e5 100644 --- a/core/src/test/java/io/grpc/internal/AbstractTransportTest.java +++ b/core/src/test/java/io/grpc/internal/AbstractTransportTest.java @@ -157,7 +157,7 @@ public void log(ChannelLogLevel level, String messageFormat, Object... args) {} * {@code serverListener}, otherwise tearDown() can't wait for shutdown which can put following * tests in an indeterminate state. */ - private InternalServer server; + protected InternalServer server; private ServerTransport serverTransport; private ManagedClientTransport client; private MethodDescriptor methodDescriptor = @@ -2064,7 +2064,7 @@ private static void assertStatusEquals(Status expected, Status actual) { * Verifies that the client status is as expected. By default, the code and description should * be present, and the cause should be stripped away. */ - protected void checkClientStatus(Status expectedStatus, Status clientStreamStatus) { + private void checkClientStatus(Status expectedStatus, Status clientStreamStatus) { assertEquals(expectedStatus.getCode(), clientStreamStatus.getCode()); assertEquals(expectedStatus.getDescription(), clientStreamStatus.getDescription()); assertNull(clientStreamStatus.getCause()); From 4ac7bebf950c95ec37f927cf2b1bbfc59a26d2d8 Mon Sep 17 00:00:00 2001 From: reggiemcdonald Date: Tue, 5 May 2020 14:16:59 -0600 Subject: [PATCH 5/6] internal: Add back assertnull assertion for server status This commit addresses PR feedback by adding back a null assertion. Default behaviour for transports is to strip cause from status. So in AbstractTransportTest.clientCancel, server status should have a null cause. --- core/src/test/java/io/grpc/internal/AbstractTransportTest.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/core/src/test/java/io/grpc/internal/AbstractTransportTest.java b/core/src/test/java/io/grpc/internal/AbstractTransportTest.java index e87829960e5..43f6273cdeb 100644 --- a/core/src/test/java/io/grpc/internal/AbstractTransportTest.java +++ b/core/src/test/java/io/grpc/internal/AbstractTransportTest.java @@ -1210,6 +1210,9 @@ public void clientCancel() throws Exception { assertNotNull(clientStreamListener.trailers.get(TIMEOUT_MS, TimeUnit.MILLISECONDS)); Status serverStatus = serverStreamListener.status.get(TIMEOUT_MS, TimeUnit.MILLISECONDS); assertNotEquals(Status.Code.OK, serverStatus.getCode()); + // Cause should not be transmitted between client and server by default + assertNull(serverStatus.getCause()); + clientStream.cancel(status); assertTrue(clientStreamTracer1.getOutboundHeaders()); assertNull(clientStreamTracer1.getInboundTrailers()); From d27e0756800079ba0325472ddedf2134f6e66b68 Mon Sep 17 00:00:00 2001 From: reggiemcdonald Date: Tue, 5 May 2020 18:31:55 -0600 Subject: [PATCH 6/6] inprocess: rename InProccessTransport.stripCause to InProcessTransport.cleanStatus and update javadoc This commit addresses PR feedback by renaming InProcessTransport.stripCause to InProcessTransport.cleanStatus and updating the javadoc, to better reflect the fact that the status can now optionally include cause. --- .../io/grpc/inprocess/InProcessTransport.java | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/core/src/main/java/io/grpc/inprocess/InProcessTransport.java b/core/src/main/java/io/grpc/inprocess/InProcessTransport.java index 998dbfc0872..3461eeebc0f 100644 --- a/core/src/main/java/io/grpc/inprocess/InProcessTransport.java +++ b/core/src/main/java/io/grpc/inprocess/InProcessTransport.java @@ -568,7 +568,7 @@ public void close(Status status, Metadata trailers) { /** clientStream.serverClosed() must be called before this method */ private void notifyClientClose(Status status, Metadata trailers) { - Status clientStatus = stripCause(status, includeCauseWithStatus); + Status clientStatus = cleanStatus(status, includeCauseWithStatus); synchronized (this) { if (closed) { return; @@ -748,7 +748,7 @@ public synchronized boolean isReady() { // Must be thread-safe for shutdownNow() @Override public void cancel(Status reason) { - Status serverStatus = stripCause(reason, includeCauseWithStatus); + Status serverStatus = cleanStatus(reason, includeCauseWithStatus); if (!internalCancel(serverStatus, serverStatus)) { return; } @@ -847,13 +847,15 @@ public void appendTimeoutInsight(InsightBuilder insight) { } /** - * Returns a new status with the same code and description, but stripped of any other information - * (i.e. cause). + * Returns a new status with the same code and description. + * If includeCauseWithStatus is true, cause is also included. * - *

This is, so that the InProcess transport behaves in the same way as the other transports, - * when exchanging statuses between client and server and vice versa. + *

For InProcess transport to behave in the same way as the other transports, + * when exchanging statuses between client and server and vice versa, + * the cause should be excluded from the status. + * For easier debugging, the status may be optionally included. */ - private static Status stripCause(Status status, boolean includeCauseWithStatus) { + private static Status cleanStatus(Status status, boolean includeCauseWithStatus) { if (status == null) { return null; }