Home Original page

core: grpc-timeout should always be positive (#12201) · grpc/grpc-java@6dfa03c

File tree

7 files changed

lines changed

  • binder/src/main/java/io/grpc/binder/internal

    • main/java/io/grpc/internal

    • test/java/io/grpc/internal

  • inprocess/src/main/java/io/grpc/inprocess

7 files changed

lines changed

Original file line numberDiff line numberDiff line change

@@ -19,7 +19,6 @@

1919

import static com.google.common.base.Preconditions.checkNotNull;

2020

import static com.google.common.base.Preconditions.checkState;

2121

import static io.grpc.internal.GrpcUtil.TIMEOUT_KEY;

22-

import static java.lang.Math.max;

2322
2423

import android.os.Parcel;

2524

import com.google.errorprone.annotations.concurrent.GuardedBy;

@@ -397,8 +396,7 @@ protected int writeSuffix(Parcel parcel) throws IOException {

397396

@GuardedBy("this")

398397

void setDeadline(Deadline deadline) {

399398

headers.discardAll(TIMEOUT_KEY);

400-

long effectiveTimeoutNanos = max(0, deadline.timeRemaining(TimeUnit.NANOSECONDS));

401-

headers.put(TIMEOUT_KEY, effectiveTimeoutNanos);

399+

headers.put(TIMEOUT_KEY, deadline.timeRemaining(TimeUnit.NANOSECONDS));

402400

}

403401

}

404402
Original file line numberDiff line numberDiff line change

@@ -21,7 +21,6 @@

2121

import static io.grpc.internal.GrpcUtil.CONTENT_ENCODING_KEY;

2222

import static io.grpc.internal.GrpcUtil.MESSAGE_ENCODING_KEY;

2323

import static io.grpc.internal.GrpcUtil.TIMEOUT_KEY;

24-

import static java.lang.Math.max;

2524
2625

import com.google.common.annotations.VisibleForTesting;

2726

import com.google.common.base.Preconditions;

@@ -124,8 +123,7 @@ protected AbstractClientStream(

124123

@Override

125124

public void setDeadline(Deadline deadline) {

126125

headers.discardAll(TIMEOUT_KEY);

127-

long effectiveTimeout = max(0, deadline.timeRemaining(TimeUnit.NANOSECONDS));

128-

headers.put(TIMEOUT_KEY, effectiveTimeout);

126+

headers.put(TIMEOUT_KEY, deadline.timeRemaining(TimeUnit.NANOSECONDS));

129127

}

130128
131129

@Override

Original file line numberDiff line numberDiff line change

@@ -651,12 +651,14 @@ public Stopwatch get() {

651651

static class TimeoutMarshaller implements Metadata.AsciiMarshaller<Long> {

652652
653653

@Override

654-

public String toAsciiString(Long timeoutNanos) {

654+

public String toAsciiString(Long timeoutNanosObject) {

655655

long cutoff = 100000000;

656+

// Timeout checking is inherently racy. RPCs with timeouts in the past ideally don't even get

657+

// here, but if the timeout is expired assume that happened recently and adjust it to the

658+

// smallest allowed timeout

659+

long timeoutNanos = Math.max(1, timeoutNanosObject);

656660

TimeUnit unit = TimeUnit.NANOSECONDS;

657-

if (timeoutNanos < 0) {

658-

throw new IllegalArgumentException("Timeout too small");

659-

} else if (timeoutNanos < cutoff) {

661+

if (timeoutNanos < cutoff) {

660662

return timeoutNanos + "n";

661663

} else if (timeoutNanos < cutoff * 1000L) {

662664

return unit.toMicros(timeoutNanos) + "u";

Original file line numberDiff line numberDiff line change

@@ -465,6 +465,24 @@ allocator, new BaseTransportState(statsTraceCtx, transportTracer), sink, statsTr

465465

.isGreaterThan(TimeUnit.MILLISECONDS.toNanos(600));

466466

}

467467
468+

@Test

469+

public void setDeadline_thePastBecomesPositive() {

470+

AbstractClientStream.Sink sink = mock(AbstractClientStream.Sink.class);

471+

ClientStream stream = new BaseAbstractClientStream(

472+

allocator, new BaseTransportState(statsTraceCtx, transportTracer), sink, statsTraceCtx,

473+

transportTracer);

474+
475+

stream.setDeadline(Deadline.after(-1, TimeUnit.NANOSECONDS));

476+

stream.start(mockListener);

477+
478+

ArgumentCaptor<Metadata> headersCaptor = ArgumentCaptor.forClass(Metadata.class);

479+

verify(sink).writeHeaders(headersCaptor.capture(), ArgumentMatchers.<byte[]>any());

480+
481+

Metadata headers = headersCaptor.getValue();

482+

assertThat(headers.get(Metadata.Key.of("grpc-timeout", Metadata.ASCII_STRING_MARSHALLER)))

483+

.isEqualTo("1n");

484+

}

485+
468486

@Test

469487

public void appendTimeoutInsight() {

470488

InsightBuilder insight = new InsightBuilder();

Original file line numberDiff line numberDiff line change

@@ -98,8 +98,8 @@ public void timeoutTest() {

9898

GrpcUtil.TimeoutMarshaller marshaller =

9999

new GrpcUtil.TimeoutMarshaller();

100100

// nanos

101-

assertEquals("0n", marshaller.toAsciiString(0L));

102-

assertEquals(0L, (long) marshaller.parseAsciiString("0n"));

101+

assertEquals("1n", marshaller.toAsciiString(1L));

102+

assertEquals(1L, (long) marshaller.parseAsciiString("1n"));

103103
104104

assertEquals("99999999n", marshaller.toAsciiString(99999999L));

105105

assertEquals(99999999L, (long) marshaller.parseAsciiString("99999999n"));

Original file line numberDiff line numberDiff line change

@@ -53,6 +53,7 @@

5353

import io.grpc.Channel;

5454

import io.grpc.Compressor;

5555

import io.grpc.Context;

56+

import io.grpc.Deadline;

5657

import io.grpc.Grpc;

5758

import io.grpc.HandlerRegistry;

5859

import io.grpc.IntegerMarshaller;

@@ -1146,11 +1147,21 @@ public ServerCall.Listener<String> startCall(

11461147

@Test

11471148

public void testContextExpiredBeforeStreamCreate_StreamCancelNotCalledBeforeSetListener()

11481149

throws Exception {

1150+

builder.ticker = new Deadline.Ticker() {

1151+

private long time;

1152+
1153+

@Override

1154+

public long nanoTime() {

1155+

time += 1000;

1156+

return time;

1157+

}

1158+

};

1159+
11491160

AtomicBoolean contextCancelled = new AtomicBoolean(false);

11501161

AtomicReference<Context> context = new AtomicReference<>();

11511162

AtomicReference<ServerCall<String, Integer>> callReference = new AtomicReference<>();

11521163
1153-

testStreamClose_setup(callReference, context, contextCancelled, 0L);

1164+

testStreamClose_setup(callReference, context, contextCancelled, 1L);

11541165
11551166

// This assert that stream.setListener(jumpListener) is called before stream.cancel(), which

11561167

// prevents extremely short deadlines causing NPEs.

Original file line numberDiff line numberDiff line change

@@ -18,7 +18,6 @@

1818
1919

import static com.google.common.base.Preconditions.checkNotNull;

2020

import static io.grpc.internal.GrpcUtil.TIMEOUT_KEY;

21-

import static java.lang.Math.max;

2221
2322

import com.google.common.base.MoreObjects;

2423

import com.google.common.io.ByteStreams;

@@ -939,8 +938,7 @@ public void setMaxOutboundMessageSize(int maxSize) {}

939938

@Override

940939

public void setDeadline(Deadline deadline) {

941940

headers.discardAll(TIMEOUT_KEY);

942-

long effectiveTimeout = max(0, deadline.timeRemaining(TimeUnit.NANOSECONDS));

943-

headers.put(TIMEOUT_KEY, effectiveTimeout);

941+

headers.put(TIMEOUT_KEY, deadline.timeRemaining(TimeUnit.NANOSECONDS));

944942

}

945943
946944

@Override