From 004ee10a73e840bbb6b068b2cb6ceac706ede8f4 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Mon, 4 Apr 2022 11:24:55 -0700 Subject: [PATCH] core: Vastly separate types of clock in FakeClock There was an attempt to use different epochs for the wall clock and the monotonic clock. However, 123456789 is actually less than a second. We want the gap between clocks to be at least a day. This issue was discovered in #8968. This separation found a bug in an RLS test where it was mixing epochs. However, it was only a problem in the test. The code under test is wrongly using wall clock for calculation durations, but that seems to be a wide-spread problem and will need to be handled separately. --- core/src/test/java/io/grpc/internal/FakeClock.java | 2 +- rls/src/test/java/io/grpc/rls/AdaptiveThrottlerTest.java | 9 ++++++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/core/src/test/java/io/grpc/internal/FakeClock.java b/core/src/test/java/io/grpc/internal/FakeClock.java index d708af5f25..58a33e8a23 100644 --- a/core/src/test/java/io/grpc/internal/FakeClock.java +++ b/core/src/test/java/io/grpc/internal/FakeClock.java @@ -357,7 +357,7 @@ public final class FakeClock { public long currentTimeMillis() { // Normally millis and nanos are of different epochs. Add an offset to simulate that. - return TimeUnit.NANOSECONDS.toMillis(currentTimeNanos + 123456789L); + return TimeUnit.NANOSECONDS.toMillis(currentTimeNanos + 1234567890123456789L); } /** diff --git a/rls/src/test/java/io/grpc/rls/AdaptiveThrottlerTest.java b/rls/src/test/java/io/grpc/rls/AdaptiveThrottlerTest.java index 8da8e9b021..54482e8de6 100644 --- a/rls/src/test/java/io/grpc/rls/AdaptiveThrottlerTest.java +++ b/rls/src/test/java/io/grpc/rls/AdaptiveThrottlerTest.java @@ -41,6 +41,8 @@ public class AdaptiveThrottlerTest { @Test public void shouldThrottle() { + long startTime = fakeClock.currentTimeMillis(); + // initial states assertThat(throttler.requestStat.get(fakeTimeProvider.currentTimeNanos())).isEqualTo(0L); assertThat(throttler.throttledStat.get(fakeTimeProvider.currentTimeNanos())).isEqualTo(0L); @@ -71,8 +73,8 @@ public class AdaptiveThrottlerTest { .isWithin(TOLERANCE) .of(1.0f / 3.0f); - // Skip half a second (half the duration). - fakeClock.forwardTime(500 - fakeClock.currentTimeMillis(), TimeUnit.MILLISECONDS); + // Skip to half second mark from the beginning (half the duration). + fakeClock.forwardTime(500 - (fakeClock.currentTimeMillis() - startTime), TimeUnit.MILLISECONDS); // Request 3, throttled by backend assertThat(throttler.shouldThrottle(0.4f)).isFalse(); @@ -96,7 +98,8 @@ public class AdaptiveThrottlerTest { .of(3.0f / 5.0f); // Skip to the point where only requests 3 and 4 are visible. - fakeClock.forwardTime(1250 - fakeClock.currentTimeMillis(), TimeUnit.MILLISECONDS); + fakeClock.forwardTime( + 1250 - (fakeClock.currentTimeMillis() - startTime), TimeUnit.MILLISECONDS); assertThat(throttler.requestStat.get(fakeTimeProvider.currentTimeNanos())).isEqualTo(2L); assertThat(throttler.throttledStat.get(fakeTimeProvider.currentTimeNanos())).isEqualTo(2L);