Skip to content

Commit a57c14a

Browse files
authored
refactor: Stops exception allocation on channel shutdown
This fixes #11955. Stops exception allocation and its propagation on channel shutdown.
1 parent e80c197 commit a57c14a

File tree

16 files changed

+56
-66
lines changed

16 files changed

+56
-66
lines changed

binder/src/main/java/io/grpc/binder/internal/PingTracker.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ private final class Ping {
9999
private synchronized void fail(Status status) {
100100
if (!done) {
101101
done = true;
102-
executor.execute(() -> callback.onFailure(status.asException()));
102+
executor.execute(() -> callback.onFailure(status));
103103
}
104104
}
105105

binder/src/test/java/io/grpc/binder/internal/PingTrackerTest.java

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@ private static final class TestCallback implements ClientTransport.PingCallback
9696
private int numCallbacks;
9797
private boolean success;
9898
private boolean failure;
99-
private Throwable failureException;
99+
private Status failureStatus;
100100
private long roundtripTimeNanos;
101101

102102
@Override
@@ -107,10 +107,10 @@ public synchronized void onSuccess(long roundtripTimeNanos) {
107107
}
108108

109109
@Override
110-
public synchronized void onFailure(Throwable failureException) {
110+
public synchronized void onFailure(Status failureStatus) {
111111
numCallbacks += 1;
112112
failure = true;
113-
this.failureException = failureException;
113+
this.failureStatus = failureStatus;
114114
}
115115

116116
public void assertNotCalled() {
@@ -130,13 +130,13 @@ public void assertSuccess(long expectRoundTripTimeNanos) {
130130
public void assertFailure(Status status) {
131131
assertThat(numCallbacks).isEqualTo(1);
132132
assertThat(failure).isTrue();
133-
assertThat(((StatusException) failureException).getStatus()).isSameInstanceAs(status);
133+
assertThat(failureStatus).isSameInstanceAs(status);
134134
}
135135

136136
public void assertFailure(Status.Code statusCode) {
137137
assertThat(numCallbacks).isEqualTo(1);
138138
assertThat(failure).isTrue();
139-
assertThat(((StatusException) failureException).getStatus().getCode()).isEqualTo(statusCode);
139+
assertThat(failureStatus.getCode()).isEqualTo(statusCode);
140140
}
141141
}
142142
}

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import io.grpc.InternalInstrumented;
2323
import io.grpc.Metadata;
2424
import io.grpc.MethodDescriptor;
25+
import io.grpc.Status;
2526
import java.util.concurrent.Executor;
2627
import javax.annotation.concurrent.ThreadSafe;
2728

@@ -90,6 +91,6 @@ interface PingCallback {
9091
*
9192
* @param cause the cause of the ping failure
9293
*/
93-
void onFailure(Throwable cause);
94+
void onFailure(Status cause);
9495
}
9596
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ public ClientStream newStream(
5555
public void ping(final PingCallback callback, Executor executor) {
5656
executor.execute(new Runnable() {
5757
@Override public void run() {
58-
callback.onFailure(error.asException());
58+
callback.onFailure(error);
5959
}
6060
});
6161
}

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

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
import com.google.common.base.Stopwatch;
2020
import com.google.errorprone.annotations.concurrent.GuardedBy;
21+
import io.grpc.Status;
2122
import io.grpc.internal.ClientTransport.PingCallback;
2223
import java.util.LinkedHashMap;
2324
import java.util.Map;
@@ -62,7 +63,7 @@ public class Http2Ping {
6263
/**
6364
* If non-null, indicates the ping failed.
6465
*/
65-
@GuardedBy("this") private Throwable failureCause;
66+
@GuardedBy("this") private Status failureCause;
6667

6768
/**
6869
* The round-trip time for the ping, in nanoseconds. This value is only meaningful when
@@ -144,7 +145,7 @@ public boolean complete() {
144145
*
145146
* @param failureCause the cause of failure
146147
*/
147-
public void failed(Throwable failureCause) {
148+
public void failed(Status failureCause) {
148149
Map<ClientTransport.PingCallback, Executor> callbacks;
149150
synchronized (this) {
150151
if (completed) {
@@ -167,7 +168,7 @@ public void failed(Throwable failureCause) {
167168
* @param executor the executor used to invoke the callback
168169
* @param cause the cause of failure
169170
*/
170-
public static void notifyFailed(PingCallback callback, Executor executor, Throwable cause) {
171+
public static void notifyFailed(PingCallback callback, Executor executor, Status cause) {
171172
doExecute(executor, asRunnable(callback, cause));
172173
}
173174

@@ -203,7 +204,7 @@ public void run() {
203204
* failure.
204205
*/
205206
private static Runnable asRunnable(final ClientTransport.PingCallback callback,
206-
final Throwable failureCause) {
207+
final Status failureCause) {
207208
return new Runnable() {
208209
@Override
209210
public void run() {

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -275,7 +275,7 @@ public void ping() {
275275
public void onSuccess(long roundTripTimeNanos) {}
276276

277277
@Override
278-
public void onFailure(Throwable cause) {
278+
public void onFailure(Status cause) {
279279
transport.shutdownNow(Status.UNAVAILABLE.withDescription(
280280
"Keepalive failed. The connection is likely gone"));
281281
}

core/src/test/java/io/grpc/internal/KeepAliveManagerTest.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,7 @@ public void clientKeepAlivePinger_pingFailure() {
127127
verify(transport).ping(pingCallbackCaptor.capture(), isA(Executor.class));
128128
ClientTransport.PingCallback pingCallback = pingCallbackCaptor.getValue();
129129

130-
pingCallback.onFailure(new Throwable());
130+
pingCallback.onFailure(Status.UNAVAILABLE.withDescription("I must write descriptions"));
131131

132132
ArgumentCaptor<Status> statusCaptor = ArgumentCaptor.forClass(Status.class);
133133
verify(transport).shutdownNow(statusCaptor.capture());

core/src/testFixtures/java/io/grpc/internal/AbstractTransportTest.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -181,7 +181,7 @@ public void log(ChannelLogLevel level, String messageFormat, Object... args) {}
181181
protected ManagedClientTransport.Listener mockClientTransportListener
182182
= mock(ManagedClientTransport.Listener.class);
183183
protected MockServerListener serverListener = new MockServerListener();
184-
private ArgumentCaptor<Throwable> throwableCaptor = ArgumentCaptor.forClass(Throwable.class);
184+
private ArgumentCaptor<Status> statusCaptor = ArgumentCaptor.forClass(Status.class);
185185
protected final TestClientStreamTracer clientStreamTracer1 = new TestHeaderClientStreamTracer();
186186
private final TestClientStreamTracer clientStreamTracer2 = new TestHeaderClientStreamTracer();
187187
protected final ClientStreamTracer[] tracers = new ClientStreamTracer[] {
@@ -626,8 +626,8 @@ public void ping_afterTermination() throws Exception {
626626
// Transport doesn't support ping, so this neither passes nor fails.
627627
assumeTrue(false);
628628
}
629-
verify(mockPingCallback, timeout(TIMEOUT_MS)).onFailure(throwableCaptor.capture());
630-
Status status = Status.fromThrowable(throwableCaptor.getValue());
629+
verify(mockPingCallback, timeout(TIMEOUT_MS)).onFailure(statusCaptor.capture());
630+
Status status = statusCaptor.getValue();
631631
assertSame(shutdownReason, status);
632632
}
633633

inprocess/src/main/java/io/grpc/inprocess/InProcessTransport.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -246,7 +246,7 @@ public synchronized void ping(final PingCallback callback, Executor executor) {
246246
executor.execute(new Runnable() {
247247
@Override
248248
public void run() {
249-
callback.onFailure(shutdownStatus.asRuntimeException());
249+
callback.onFailure(shutdownStatus);
250250
}
251251
});
252252
} else {

netty/src/main/java/io/grpc/netty/ClientTransportLifecycleManager.java

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@ final class ClientTransportLifecycleManager {
3030
/** null iff !transportShutdown. */
3131
private Status shutdownStatus;
3232
/** null iff !transportShutdown. */
33-
private Throwable shutdownThrowable;
3433
private boolean transportTerminated;
3534

3635
public ClientTransportLifecycleManager(ManagedClientTransport.Listener listener) {
@@ -72,7 +71,6 @@ public boolean notifyShutdown(Status s) {
7271
return false;
7372
}
7473
shutdownStatus = s;
75-
shutdownThrowable = s.asException();
7674
return true;
7775
}
7876

@@ -97,7 +95,4 @@ public Status getShutdownStatus() {
9795
return shutdownStatus;
9896
}
9997

100-
public Throwable getShutdownThrowable() {
101-
return shutdownThrowable;
102-
}
10398
}

0 commit comments

Comments
 (0)