Skip to content

Commit ead532b

Browse files
committed
core: Improve DEADLINE_EXCEEDED message for CallCreds delays
DelayedStream is used both by DelayedClientTransport and CallCredentialsApplyingTransport, but it wasn't clear from the error which of the two was the cause of the delay. Now the two will have different messages. b/462499883
1 parent 738782f commit ead532b

File tree

5 files changed

+21
-9
lines changed

5 files changed

+21
-9
lines changed

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -363,6 +363,7 @@ private class PendingStream extends DelayedStream {
363363
private volatile Status lastPickStatus;
364364

365365
private PendingStream(PickSubchannelArgs args, ClientStreamTracer[] tracers) {
366+
super("connecting_and_lb");
366367
this.args = args;
367368
this.tracers = tracers;
368369
}

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

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
* necessary.
4343
*/
4444
class DelayedStream implements ClientStream {
45+
private final String bufferContext;
4546
/** {@code true} once realStream is valid and all pending calls have been drained. */
4647
private volatile boolean passThrough;
4748
/**
@@ -64,6 +65,14 @@ class DelayedStream implements ClientStream {
6465
// No need to synchronize; start() synchronization provides a happens-before
6566
private List<Runnable> preStartPendingCalls = new ArrayList<>();
6667

68+
/**
69+
* Create a delayed stream with debug context {@code bufferContext}. The context is what this
70+
* stream is delayed by (e.g., "connecting", "call_credentials").
71+
*/
72+
public DelayedStream(String bufferContext) {
73+
this.bufferContext = checkNotNull(bufferContext, "bufferContext");
74+
}
75+
6776
@Override
6877
public void setMaxInboundMessageSize(final int maxSize) {
6978
checkState(listener == null, "May only be called before start");
@@ -104,11 +113,13 @@ public void appendTimeoutInsight(InsightBuilder insight) {
104113
return;
105114
}
106115
if (realStream != null) {
107-
insight.appendKeyValue("buffered_nanos", streamSetTimeNanos - startTimeNanos);
116+
insight.appendKeyValue(
117+
bufferContext + "_delay", "" + (streamSetTimeNanos - startTimeNanos) + "ns");
108118
realStream.appendTimeoutInsight(insight);
109119
} else {
110-
insight.appendKeyValue("buffered_nanos", System.nanoTime() - startTimeNanos);
111-
insight.append("waiting_for_connection");
120+
insight.appendKeyValue(
121+
bufferContext + "_delay", "" + (System.nanoTime() - startTimeNanos) + "ns");
122+
insight.append("was_still_waiting");
112123
}
113124
}
114125
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,7 @@ ClientStream returnStream() {
120120
synchronized (lock) {
121121
if (returnedStream == null) {
122122
// apply() has not been called, needs to buffer the requests.
123-
delayedStream = new DelayedStream();
123+
delayedStream = new DelayedStream("call_credentials");
124124
return returnedStream = delayedStream;
125125
} else {
126126
return returnedStream;

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -742,7 +742,7 @@ public void pendingStream_appendTimeoutInsight_waitForReady() {
742742
InsightBuilder insight = new InsightBuilder();
743743
stream.appendTimeoutInsight(insight);
744744
assertThat(insight.toString())
745-
.matches("\\[wait_for_ready, buffered_nanos=[0-9]+\\, waiting_for_connection]");
745+
.matches("\\[wait_for_ready, connecting_and_lb_delay=[0-9]+ns\\, was_still_waiting]");
746746
}
747747

748748
@Test
@@ -759,7 +759,7 @@ public void pendingStream_appendTimeoutInsight_waitForReady_withLastPickFailure(
759759
assertThat(insight.toString())
760760
.matches("\\[wait_for_ready, "
761761
+ "Last Pick Failure=Status\\{code=PERMISSION_DENIED, description=null, cause=null\\},"
762-
+ " buffered_nanos=[0-9]+, waiting_for_connection]");
762+
+ " connecting_and_lb_delay=[0-9]+ns, was_still_waiting]");
763763
}
764764

765765
private static TransportProvider newTransportProvider(final ClientTransport transport) {

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ public class DelayedStreamTest {
7171
@Mock private ClientStreamListener listener;
7272
@Mock private ClientStream realStream;
7373
@Captor private ArgumentCaptor<ClientStreamListener> listenerCaptor;
74-
private DelayedStream stream = new DelayedStream();
74+
private DelayedStream stream = new DelayedStream("test_op");
7575

7676
@Test
7777
public void setStream_setAuthority() {
@@ -450,7 +450,7 @@ public void appendTimeoutInsight_realStreamNotSet() {
450450
InsightBuilder insight = new InsightBuilder();
451451
stream.start(listener);
452452
stream.appendTimeoutInsight(insight);
453-
assertThat(insight.toString()).matches("\\[buffered_nanos=[0-9]+\\, waiting_for_connection]");
453+
assertThat(insight.toString()).matches("\\[test_op_delay=[0-9]+ns\\, was_still_waiting]");
454454
}
455455

456456
@Test
@@ -469,7 +469,7 @@ public Void answer(InvocationOnMock in) {
469469
InsightBuilder insight = new InsightBuilder();
470470
stream.appendTimeoutInsight(insight);
471471
assertThat(insight.toString())
472-
.matches("\\[buffered_nanos=[0-9]+, remote_addr=127\\.0\\.0\\.1:443\\]");
472+
.matches("\\[test_op_delay=[0-9]+ns, remote_addr=127\\.0\\.0\\.1:443\\]");
473473
}
474474

475475
private void callMeMaybe(Runnable r) {

0 commit comments

Comments
 (0)