diff --git a/deadlines/src/main/java/com/palantir/deadlines/Deadlines.java b/deadlines/src/main/java/com/palantir/deadlines/Deadlines.java index 2bc5f7a..c31581f 100644 --- a/deadlines/src/main/java/com/palantir/deadlines/Deadlines.java +++ b/deadlines/src/main/java/com/palantir/deadlines/Deadlines.java @@ -56,6 +56,8 @@ private Deadlines() {} * * If no deadline state has been set for the current trace, return an empty Optional. * + * This method never throws. + * * @return the remaining deadline time for the current trace, or {@link Duration#ZERO} if the deadline * has expired, or {@link Optional#empty()} if no such deadline state exists. */ @@ -89,6 +91,9 @@ private static Optional getRemainingDeadlineInternal() { * * This function has no side-effects on the internal deadline state stored in a TraceLocal. * + * If at the time this method is called the selected deadline to encode has already expired, a + * {@link DeadlineExpiredException} will be thrown. + * * @param proposedDeadline a proposed value for the deadline; the actual value used will be the minimum of * this value and one already set via a previous call to {@link #parseFromRequest}, if it exists * @param request the request object to write the encoding to @@ -126,9 +131,16 @@ public static void encodeToRequest( * {@link #getRemainingDeadline()}} from threads participating in the current trace. The deadline value * is read from a {@link DeadlinesHttpHeaders#EXPECT_WITHIN} header on the request object. * + * The optional `internalDeadline` parameter specifices an alternative deadline to be used if it is lower + * than the one parsed from the request header. Callers can use this to bound the selected deadline based + * on specific internal logic that might take precedence over one provided in a request. + * * This function has side-effects on the internal deadline state stored in a TraceLocal; the state is * set (or overwritten) based on the value of the deadline parsed from request headers. * + * If at the time this method is called the selected deadline to store has already expired, a + * {@link DeadlineExpiredException} will be thrown. + * * @param internalDeadline if present, represents an alternative deadline that should be used if it is * lower than the one parsed from a request header * @param request the request object to read the deadline value from @@ -170,7 +182,10 @@ private static void checkExpiration(long deadline, boolean internal) { // expired Expired_Cause cause = internal ? Expired_Cause.INTERNAL : Expired_Cause.EXTERNAL; metrics.expired(cause).mark(); - // TODO(blaub): throw exception instead of return + if (internal) { + throw DeadlineExpiredException.internal(); + } + throw DeadlineExpiredException.external(); } } diff --git a/deadlines/src/test/java/com/palantir/deadlines/DeadlinesTest.java b/deadlines/src/test/java/com/palantir/deadlines/DeadlinesTest.java index 5626692..858967b 100644 --- a/deadlines/src/test/java/com/palantir/deadlines/DeadlinesTest.java +++ b/deadlines/src/test/java/com/palantir/deadlines/DeadlinesTest.java @@ -18,12 +18,15 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import com.codahale.metrics.Meter; import com.palantir.deadlines.DeadlineMetrics.Expired_Cause; import com.palantir.deadlines.Deadlines.RequestDecodingAdapter; import com.palantir.deadlines.Deadlines.RequestEncodingAdapter; +import com.palantir.tracing.CloseableSpan; import com.palantir.tracing.CloseableTracer; +import com.palantir.tracing.DetachedSpan; import com.palantir.tritium.metrics.registry.SharedTaggedMetricRegistries; import java.time.Duration; import java.util.HashMap; @@ -306,7 +309,9 @@ public void test_encode_to_request_expiration_external_deadline() { long originalInternalValue = internalMeter.getCount(); Map outbound = new HashMap<>(); - Deadlines.encodeToRequest(Duration.ofSeconds(10), outbound, DummyRequestEncoder.INSTANCE); + assertThatThrownBy(() -> + Deadlines.encodeToRequest(Duration.ofSeconds(10), outbound, DummyRequestEncoder.INSTANCE)) + .isInstanceOf(DeadlineExpiredException.External.class); assertThat(externalMeter.getCount()).isGreaterThan(originalExternalValue); assertThat(internalMeter.getCount()).isEqualTo(originalInternalValue); @@ -314,7 +319,7 @@ public void test_encode_to_request_expiration_external_deadline() { } @Test - public void test_encode_to_request_expiration_internal_deadline_2() { + public void test_encode_to_request_expiration_internal_deadline() { TestClock clock = new TestClock(); Deadlines.setClock(clock); try (CloseableTracer tracer = CloseableTracer.startSpan("test")) { @@ -335,13 +340,117 @@ public void test_encode_to_request_expiration_internal_deadline_2() { long originalInternalValue = internalMeter.getCount(); Map outbound = new HashMap<>(); - Deadlines.encodeToRequest(Duration.ofSeconds(10), outbound, DummyRequestEncoder.INSTANCE); + assertThatThrownBy(() -> + Deadlines.encodeToRequest(Duration.ofSeconds(10), outbound, DummyRequestEncoder.INSTANCE)) + .isInstanceOf(DeadlineExpiredException.Internal.class); assertThat(internalMeter.getCount()).isGreaterThan(originalInternalValue); assertThat(externalMeter.getCount()).isEqualTo(originalExternalValue); } } + @Test + public void test_rpc_expiration_1hop_external() { + TestClock clock = new TestClock(); + Deadlines.setClock(clock); + + Map request = new HashMap<>(); + Duration clientDeadline = Duration.ofSeconds(1); + Deadlines.encodeToRequest(clientDeadline, request, DummyRequestEncoder.INSTANCE); + + DetachedSpan span = DetachedSpan.start("test"); + try (CloseableSpan ignored = span.attach()) { + Deadlines.parseFromRequest(Optional.empty(), request, DummyRequestDecoder.INSTANCE); + clock.elapsed += Duration.ofSeconds(5).toNanos(); + // simulate making another outbound request, which should throw + Map serverRequest = new HashMap<>(); + assertThatThrownBy(() -> Deadlines.encodeToRequest( + Duration.ofSeconds(1), serverRequest, DummyRequestEncoder.INSTANCE)) + .isInstanceOf(DeadlineExpiredException.External.class); + } + } + + @Test + public void test_rpc_expiration_1hop_internal() { + TestClock clock = new TestClock(); + Deadlines.setClock(clock); + + Map request = new HashMap<>(); + Duration clientDeadline = Duration.ofSeconds(1); + Deadlines.encodeToRequest(clientDeadline, request, DummyRequestEncoder.INSTANCE); + + DetachedSpan span = DetachedSpan.start("test"); + try (CloseableSpan ignored = span.attach()) { + Deadlines.parseFromRequest( + Optional.of(Duration.ofMillis(500)), /* server has an internal deadline smaller than the client's */ + request, + DummyRequestDecoder.INSTANCE); + clock.elapsed += Duration.ofMillis(600).toNanos(); + + Map serverRequest = new HashMap<>(); + assertThatThrownBy(() -> Deadlines.encodeToRequest( + Duration.ofSeconds(1), serverRequest, DummyRequestEncoder.INSTANCE)) + .isInstanceOf(DeadlineExpiredException.Internal.class); + } + } + + @Test + public void test_rpc_expiration_2hop_external_then_internal() { + // client provides large initial deadline + // server1 has a self-imposed smaller deadline + // server1 sends request to server2 with this much smaller deadline + // server2 unable to meet server1 deadline + // server2 throws "external" expiration + // enough time elapses such that server1's internally imposed deadline has expired + // server1 throws "internal" expiration + + TestClock clock = new TestClock(); + Deadlines.setClock(clock); + + Map clientRequest = new HashMap<>(); + Duration clientDeadline = Duration.ofSeconds(1); + Deadlines.encodeToRequest(clientDeadline, clientRequest, DummyRequestEncoder.INSTANCE); + + DetachedSpan server1Span = DetachedSpan.start("server1"); + DetachedSpan server2Span = DetachedSpan.start("server2"); + + try (CloseableSpan ignored = server1Span.attach()) { + Deadlines.parseFromRequest( + Optional.of(Duration.ofMillis(200)), /* server1 has smaller internally imposed deadline */ + clientRequest, + DummyRequestDecoder.INSTANCE); + clock.elapsed += Duration.ofMillis(100).toNanos(); + Map server1Request = new HashMap<>(); + Deadlines.encodeToRequest( + Duration.ofSeconds(500), /* server1 should select smaller internal deadline */ + server1Request, + DummyRequestEncoder.INSTANCE); + + assertThat(server1Request) + .containsKey(DeadlinesHttpHeaders.EXPECT_WITHIN) + .containsValue("0.100"); + + try (CloseableSpan ignored2 = server2Span.attach()) { + Deadlines.parseFromRequest(Optional.empty(), server1Request, DummyRequestDecoder.INSTANCE); + clock.elapsed += Duration.ofMillis(1500).toNanos(); + + // server2 should throw an external error + Map server2Request = new HashMap<>(); + assertThatThrownBy(() -> { + Deadlines.encodeToRequest( + Duration.ofSeconds(100), server2Request, DummyRequestEncoder.INSTANCE); + }) + .isInstanceOf(DeadlineExpiredException.External.class); + } + + assertThatThrownBy(() -> { + Deadlines.encodeToRequest( + Duration.ofSeconds(100), server1Request, DummyRequestEncoder.INSTANCE); + }) + .isInstanceOf(DeadlineExpiredException.Internal.class); + } + } + private enum DummyRequestEncoder implements RequestEncodingAdapter> { INSTANCE;