diff --git a/CHANGELOG.md b/CHANGELOG.md index c75336703af..2b95d303f2d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## Unreleased +### Behavioral Changes + +- Measure HTTP rate-limit backoff on a monotonic clock instead of the wall clock, so that a device time change no longer lifts or extends an active rate limit ([#6030](https://github.com/getsentry/sentry-java/pull/6030)) + ### Fixes - Update `SentryTraced` so that it now honors `options.setIgnoredSpanOrigins` ([#6058](https://github.com/getsentry/sentry-java/pull/6058)) @@ -19,6 +23,7 @@ - Add an internal `MonotonicTicker` abstraction with `Deadline` and `Stopwatch` primitives ([#6028](https://github.com/getsentry/sentry-java/pull/6028)) - Add internal `Timestamp`, `EpochClock` and `AnchoredClock`, so related instants project from one wall-clock reading instead of each reading the clock ([#6045](https://github.com/getsentry/sentry-java/pull/6045)) +- Deprecate `RateLimiter(ICurrentDateProvider, SentryOptions)` in favor of `RateLimiter(SentryOptions)`, whose backoff is measured on a monotonic ticker ([#6030](https://github.com/getsentry/sentry-java/pull/6030)) ### Dependencies diff --git a/sentry-apache-http-client-5/src/main/java/io/sentry/transport/apache/ApacheHttpClientTransportFactory.java b/sentry-apache-http-client-5/src/main/java/io/sentry/transport/apache/ApacheHttpClientTransportFactory.java index e6687af845b..99423ba673d 100644 --- a/sentry-apache-http-client-5/src/main/java/io/sentry/transport/apache/ApacheHttpClientTransportFactory.java +++ b/sentry-apache-http-client-5/src/main/java/io/sentry/transport/apache/ApacheHttpClientTransportFactory.java @@ -62,7 +62,7 @@ public ApacheHttpClientTransportFactory(final @NotNull TimeValue connectionTimeT .setResponseTimeout(options.getReadTimeoutMillis(), TimeUnit.MILLISECONDS) .build()) .build(); - final RateLimiter rateLimiter = new RateLimiter(options); + final RateLimiter rateLimiter = RateLimiter.create(options.getMonotonicTicker(), options); return new ApacheHttpClientTransport(options, requestDetails, httpclient, rateLimiter); } diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index e6b83beb214..24635fc5ddd 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -3654,7 +3654,7 @@ public final class io/sentry/SentryOpenTelemetryMode : java/lang/Enum { public static fun values ()[Lio/sentry/SentryOpenTelemetryMode; } -public class io/sentry/SentryOptions { +public class io/sentry/SentryOptions : io/sentry/transport/RateLimiterConfig { public static final field DEFAULT_PROPAGATION_TARGETS Ljava/lang/String; public static final field MAX_EVENT_SIZE_BYTES J protected final field lock Lio/sentry/util/AutoClosableReentrantLock; @@ -7706,6 +7706,7 @@ public final class io/sentry/transport/RateLimiter : java/io/Closeable { public fun (Lio/sentry/transport/ICurrentDateProvider;Lio/sentry/SentryOptions;)V public fun addRateLimitObserver (Lio/sentry/transport/RateLimiter$IRateLimitObserver;)V public fun close ()V + public static fun create (Lio/sentry/time/MonotonicTicker;Lio/sentry/transport/RateLimiterConfig;)Lio/sentry/transport/RateLimiter; public fun filter (Lio/sentry/SentryEnvelope;Lio/sentry/Hint;)Lio/sentry/SentryEnvelope; public fun isActiveForCategory (Lio/sentry/DataCategory;)Z public fun isAnyRateLimitActive ()Z @@ -7717,6 +7718,12 @@ public abstract interface class io/sentry/transport/RateLimiter$IRateLimitObserv public abstract fun onRateLimitChanged (Lio/sentry/transport/RateLimiter;)V } +public abstract interface class io/sentry/transport/RateLimiterConfig { + public abstract fun getClientReportRecorder ()Lio/sentry/clientreport/IClientReportRecorder; + public abstract fun getLogger ()Lio/sentry/ILogger; + public abstract fun getTimerExecutorService ()Lio/sentry/ISentryExecutorService; +} + public final class io/sentry/transport/ReusableCountLatch { public fun ()V public fun (I)V diff --git a/sentry/src/main/java/io/sentry/AsyncHttpTransportFactory.java b/sentry/src/main/java/io/sentry/AsyncHttpTransportFactory.java index efbc6be19e9..02a47e79315 100644 --- a/sentry/src/main/java/io/sentry/AsyncHttpTransportFactory.java +++ b/sentry/src/main/java/io/sentry/AsyncHttpTransportFactory.java @@ -18,6 +18,9 @@ public final class AsyncHttpTransportFactory implements ITransportFactory { Objects.requireNonNull(requestDetails, "requestDetails is required"); return new AsyncHttpTransport( - options, new RateLimiter(options), options.getTransportGate(), requestDetails); + options, + RateLimiter.create(options.getMonotonicTicker(), options), + options.getTransportGate(), + requestDetails); } } diff --git a/sentry/src/main/java/io/sentry/SentryOptions.java b/sentry/src/main/java/io/sentry/SentryOptions.java index 1a1fbf738c9..280bc01174e 100644 --- a/sentry/src/main/java/io/sentry/SentryOptions.java +++ b/sentry/src/main/java/io/sentry/SentryOptions.java @@ -29,6 +29,7 @@ import io.sentry.transport.ITransportGate; import io.sentry.transport.NoOpEnvelopeCache; import io.sentry.transport.NoOpTransportGate; +import io.sentry.transport.RateLimiterConfig; import io.sentry.util.AutoClosableReentrantLock; import io.sentry.util.LazyEvaluator; import io.sentry.util.LoadClass; @@ -58,7 +59,7 @@ /** Sentry SDK options */ @Open -public class SentryOptions { +public class SentryOptions implements RateLimiterConfig { @ApiStatus.Internal public static final @NotNull String DEFAULT_PROPAGATION_TARGETS = ".*"; @@ -847,6 +848,7 @@ public void setDebug(final boolean debug) { * * @return the logger */ + @Override public @NotNull ILogger getLogger() { return logger; } @@ -1610,6 +1612,7 @@ public void setExecutorService(final @NotNull ISentryExecutorService executorSer * @return the timer executor service */ @ApiStatus.Internal + @Override @NotNull public ISentryExecutorService getTimerExecutorService() { return timerExecutorService; @@ -2601,6 +2604,7 @@ public void setInstrumenter(final @NotNull Instrumenter instrumenter) { * @return a client report recorder or NoOp */ @ApiStatus.Internal + @Override public @NotNull IClientReportRecorder getClientReportRecorder() { return clientReportRecorder; } diff --git a/sentry/src/main/java/io/sentry/time/Deadline.java b/sentry/src/main/java/io/sentry/time/Deadline.java index 036469383c4..b6cc63e8768 100644 --- a/sentry/src/main/java/io/sentry/time/Deadline.java +++ b/sentry/src/main/java/io/sentry/time/Deadline.java @@ -24,15 +24,18 @@ private Deadline(final @NotNull MonotonicTicker ticker, final long deadlineNanos } /** - * A deadline {@code amount} of {@code unit} from now. + * A deadline {@code amount} of {@code unit} from now, or one that has already {@link #passed} if + * {@code amount} is negative. * - * @throws IllegalArgumentException if {@code amount} is negative. A deadline that starts out in - * the past is a sign error at the call site; {@link #passed} says it deliberately. + *

Negative amounts are tolerated because callers pass durations parsed from server headers, + * where a bogus value must degrade to "no wait" rather than throw out of response handling. + * Clamping rather than adding a negative offset also keeps the tick arithmetic away from + * wrapping. */ public static @NotNull Deadline after( final @NotNull MonotonicTicker ticker, final long amount, final @NotNull TimeUnit unit) { if (amount < 0) { - throw new IllegalArgumentException("Deadline amount must not be negative, but was " + amount); + return passed(ticker); } return new Deadline(ticker, ticker.tickNanos() + unit.toNanos(amount)); } diff --git a/sentry/src/main/java/io/sentry/transport/RateLimiter.java b/sentry/src/main/java/io/sentry/transport/RateLimiter.java index dfbc4cb2622..9b15cb6fefe 100644 --- a/sentry/src/main/java/io/sentry/transport/RateLimiter.java +++ b/sentry/src/main/java/io/sentry/transport/RateLimiter.java @@ -14,6 +14,8 @@ import io.sentry.hints.DiskFlushNotification; import io.sentry.hints.Retryable; import io.sentry.hints.SubmissionResult; +import io.sentry.time.Deadline; +import io.sentry.time.MonotonicTicker; import io.sentry.util.AutoClosableReentrantLock; import io.sentry.util.HintUtils; import io.sentry.util.StringUtils; @@ -22,7 +24,6 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; -import java.util.Date; import java.util.Iterator; import java.util.List; import java.util.Map; @@ -30,32 +31,65 @@ import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.Future; import java.util.concurrent.RejectedExecutionException; +import java.util.concurrent.TimeUnit; +import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; /** Controls retry limits on different category types sent to Sentry. */ public final class RateLimiter implements Closeable { - private static final int HTTP_RETRY_AFTER_DEFAULT_DELAY_MILLIS = 60000; + private static final long HTTP_RETRY_AFTER_DEFAULT_DELAY_MILLIS = 60_000; - private final @NotNull ICurrentDateProvider currentDateProvider; - private final @NotNull SentryOptions options; - private final @NotNull Map sentryRetryAfterLimit = + private final @NotNull MonotonicTicker ticker; + private final @NotNull RateLimiterConfig config; + private final @NotNull Map sentryRetryAfterLimit = new ConcurrentHashMap<>(); private final @NotNull List rateLimitObservers = new CopyOnWriteArrayList<>(); private final @NotNull List> notifyObserversFutures = new ArrayList<>(); private final @NotNull AutoClosableReentrantLock notifyFuturesLock = new AutoClosableReentrantLock(); - public RateLimiter( - final @NotNull ICurrentDateProvider currentDateProvider, - final @NotNull SentryOptions options) { - this.currentDateProvider = currentDateProvider; - this.options = options; + private RateLimiter( + final @NotNull MonotonicTicker ticker, final @NotNull RateLimiterConfig config) { + this.ticker = ticker; + this.config = config; + } + + /** + * Names the collaborators a rate limiter actually reads, rather than handing over the whole + * options object. Internal only for as long as {@link MonotonicTicker} is. + */ + @ApiStatus.Internal + public static @NotNull RateLimiter create( + final @NotNull MonotonicTicker ticker, final @NotNull RateLimiterConfig config) { + return new RateLimiter(ticker, config); } + /** + * The supported way to build a rate limiter from outside the SDK, kept non-deprecated only + * because {@link #create(MonotonicTicker, RateLimiterConfig)} cannot be called without the + * internal {@link MonotonicTicker}. Deprecate this once that type is public API. + */ public RateLimiter(final @NotNull SentryOptions options) { - this(CurrentDateProvider.getInstance(), options); + this(options.getMonotonicTicker(), options); + } + + /** + * @deprecated the date provider is only used to measure a backoff, which is now measured on + * {@link SentryOptions#getMonotonicTicker()}. Use {@link #RateLimiter(SentryOptions)}, or + * {@link #create(MonotonicTicker, RateLimiterConfig)} to supply a ticker of your own. + */ + @Deprecated + public RateLimiter( + final @NotNull ICurrentDateProvider currentDateProvider, + final @NotNull SentryOptions options) { + // ICurrentDateProvider is itself a `long ()` interface, so an unadorned lambda matches this + // constructor as readily as the intended one. The cast is what makes the call non-recursive. + this( + (MonotonicTicker) + () -> TimeUnit.MILLISECONDS.toNanos(currentDateProvider.getCurrentTimeMillis()), + options); } public @Nullable SentryEnvelope filter( @@ -70,14 +104,14 @@ public RateLimiter(final @NotNull SentryOptions options) { } dropItems.add(item); - options + config .getClientReportRecorder() .recordLostEnvelopeItem(DiscardReason.RATELIMIT_BACKOFF, item); } } if (dropItems != null) { - options + config .getLogger() .log( SentryLevel.WARNING, @@ -94,7 +128,7 @@ public RateLimiter(final @NotNull SentryOptions options) { // no reason to continue if (toSend.isEmpty()) { - options + config .getLogger() .log(SentryLevel.WARNING, "Envelope discarded due all items rate limited."); @@ -107,16 +141,11 @@ public RateLimiter(final @NotNull SentryOptions options) { return envelope; } - @SuppressWarnings({"JdkObsolete", "JavaUtilDate"}) public boolean isActiveForCategory(final @NotNull DataCategory dataCategory) { - final Date currentDate = new Date(currentDateProvider.getCurrentTimeMillis()); - // check all categories - final Date dateAllCategories = sentryRetryAfterLimit.get(DataCategory.All); - if (dateAllCategories != null) { - if (!currentDate.after(dateAllCategories)) { - return true; - } + final @Nullable Deadline allCategories = sentryRetryAfterLimit.get(DataCategory.All); + if (allCategories != null && !allCategories.hasPassed()) { + return true; } // Unknown should not be rate limited @@ -125,24 +154,15 @@ public boolean isActiveForCategory(final @NotNull DataCategory dataCategory) { } // check for specific dataCategory - final Date dateCategory = sentryRetryAfterLimit.get(dataCategory); - if (dateCategory != null) { - return !currentDate.after(dateCategory); - } - - return false; + final @Nullable Deadline categoryLimit = sentryRetryAfterLimit.get(dataCategory); + return categoryLimit != null && !categoryLimit.hasPassed(); } @SuppressWarnings({"JdkObsolete", "JavaUtilDate"}) public boolean isAnyRateLimitActive() { - final Date currentDate = new Date(currentDateProvider.getCurrentTimeMillis()); - - for (DataCategory dataCategory : sentryRetryAfterLimit.keySet()) { - final Date dateCategory = sentryRetryAfterLimit.get(dataCategory); - if (dateCategory != null) { - if (!currentDate.after(dateCategory)) { - return true; - } + for (final @NotNull Deadline limit : sentryRetryAfterLimit.values()) { + if (!limit.hasPassed()) { + return true; } } @@ -163,7 +183,7 @@ private void markHintWhenSendingFailed(final @NotNull Hint hint, final boolean r DiskFlushNotification.class, (diskFlushNotification) -> { diskFlushNotification.markFlushed(); - options.getLogger().log(SentryLevel.DEBUG, "Disk flush envelope fired due to rate limit"); + config.getLogger().log(SentryLevel.DEBUG, "Disk flush envelope fired due to rate limit"); }); } @@ -251,15 +271,11 @@ public void updateRetryAfterLimits( if (rateLimit.length > 0) { final String retryAfter = rateLimit[0]; - long retryAfterMillis = parseRetryAfterOrDefault(retryAfter); + final @NotNull Deadline deadline = parseRetryAfterOrDefault(retryAfter); if (rateLimit.length > 1) { final String allCategories = rateLimit[1]; - // we dont care if Date is UTC as we just add the relative seconds - final Date date = - new Date(currentDateProvider.getCurrentTimeMillis() + retryAfterMillis); - if (allCategories != null && !allCategories.isEmpty()) { final String[] categories = allCategories.split(";", -1); @@ -270,48 +286,43 @@ public void updateRetryAfterLimits( if (catItemCapitalized != null) { dataCategory = DataCategory.valueOf(catItemCapitalized); } else { - options.getLogger().log(ERROR, "Couldn't capitalize: %s", catItem); + config.getLogger().log(ERROR, "Couldn't capitalize: %s", catItem); } } catch (IllegalArgumentException e) { - options.getLogger().log(INFO, e, "Unknown category: %s", catItem); + config.getLogger().log(INFO, e, "Unknown category: %s", catItem); } // we dont apply rate limiting for unknown categories if (DataCategory.Unknown.equals(dataCategory)) { continue; } - applyRetryAfterOnlyIfLonger(dataCategory, date, retryAfterMillis); + applyRetryAfterOnlyIfLonger(dataCategory, deadline); } } else { // if categories are empty, we should apply to "all" categories. - applyRetryAfterOnlyIfLonger(DataCategory.All, date, retryAfterMillis); + applyRetryAfterOnlyIfLonger(DataCategory.All, deadline); } } } } } else if (errorCode == 429) { - final long retryAfterMillis = parseRetryAfterOrDefault(retryAfterHeader); - // we dont care if Date is UTC as we just add the relative seconds - final Date date = new Date(currentDateProvider.getCurrentTimeMillis() + retryAfterMillis); - applyRetryAfterOnlyIfLonger(DataCategory.All, date, retryAfterMillis); + applyRetryAfterOnlyIfLonger(DataCategory.All, parseRetryAfterOrDefault(retryAfterHeader)); } } /** - * apply new timestamp for rate limiting only if its longer than the previous one + * apply the new deadline for rate limiting only if it is longer than the previous one * * @param dataCategory the DataCategory - * @param date the Date to be applied - * @param delayMillis the millis until the rate limit is lifted + * @param deadline when the rate limit is lifted */ - @SuppressWarnings({"JdkObsolete", "JavaUtilDate"}) private void applyRetryAfterOnlyIfLonger( - final @NotNull DataCategory dataCategory, final @NotNull Date date, final long delayMillis) { - final Date oldDate = sentryRetryAfterLimit.get(dataCategory); + final @NotNull DataCategory dataCategory, final @NotNull Deadline deadline) { + final @Nullable Deadline oldLimit = sentryRetryAfterLimit.get(dataCategory); - // only overwrite its previous date if the limit is even longer - if (oldDate == null || date.after(oldDate)) { - sentryRetryAfterLimit.put(dataCategory, date); + // only overwrite the previous deadline if the limit is even longer + if (oldLimit == null || deadline.isAfter(oldLimit)) { + sentryRetryAfterLimit.put(dataCategory, deadline); notifyRateLimitObservers(); @@ -326,11 +337,12 @@ private void applyRetryAfterOnlyIfLonger( } try { notifyObserversFutures.add( - options + config .getTimerExecutorService() - .schedule(this::notifyRateLimitObservers, delayMillis)); + .schedule( + this::notifyRateLimitObservers, deadline.remaining(TimeUnit.MILLISECONDS))); } catch (RejectedExecutionException e) { - options + config .getLogger() .log(SentryLevel.WARNING, "Failed to schedule rate limit lifted notification.", e); } @@ -344,7 +356,7 @@ private void applyRetryAfterOnlyIfLonger( * @param retryAfterHeader the header * @return the millis in seconds or the default seconds value */ - private long parseRetryAfterOrDefault(final @Nullable String retryAfterHeader) { + private @NotNull Deadline parseRetryAfterOrDefault(final @Nullable String retryAfterHeader) { long retryAfterMillis = HTTP_RETRY_AFTER_DEFAULT_DELAY_MILLIS; if (retryAfterHeader != null) { try { @@ -354,7 +366,7 @@ private long parseRetryAfterOrDefault(final @Nullable String retryAfterHeader) { // let's use the default then } } - return retryAfterMillis; + return Deadline.after(ticker, retryAfterMillis, TimeUnit.MILLISECONDS); } private void notifyRateLimitObservers() { diff --git a/sentry/src/main/java/io/sentry/transport/RateLimiterConfig.java b/sentry/src/main/java/io/sentry/transport/RateLimiterConfig.java new file mode 100644 index 00000000000..789228bcbe5 --- /dev/null +++ b/sentry/src/main/java/io/sentry/transport/RateLimiterConfig.java @@ -0,0 +1,29 @@ +package io.sentry.transport; + +import io.sentry.ILogger; +import io.sentry.ISentryExecutorService; +import io.sentry.clientreport.IClientReportRecorder; +import org.jetbrains.annotations.ApiStatus; +import org.jetbrains.annotations.NotNull; + +/** + * The configuration {@link RateLimiter} reads. Declared next to its consumer rather than alongside + * the implementation, so that the collaborators a rate limiter actually touches are three lines to + * read instead of three hundred, and a test can supply them without building a {@link + * io.sentry.SentryOptions}. + * + *

Implementations are expected to delegate to live configuration rather than snapshot it, so + * that a logger or executor replaced after {@code Sentry.init} is still picked up. + */ +@ApiStatus.Internal +public interface RateLimiterConfig { + + @NotNull + ILogger getLogger(); + + @NotNull + IClientReportRecorder getClientReportRecorder(); + + @NotNull + ISentryExecutorService getTimerExecutorService(); +} diff --git a/sentry/src/test/java/io/sentry/time/DeadlineTest.kt b/sentry/src/test/java/io/sentry/time/DeadlineTest.kt index 2cf4804f39f..fc7241e7cd6 100644 --- a/sentry/src/test/java/io/sentry/time/DeadlineTest.kt +++ b/sentry/src/test/java/io/sentry/time/DeadlineTest.kt @@ -65,10 +65,22 @@ class DeadlineTest { } @Test - fun `after rejects a negative amount`() { - assertFailsWith { - Deadline.after(TestMonotonicTicker(), -1, SECONDS) - } + fun `after treats a negative amount as already passed`() { + val ticker = TestMonotonicTicker() + + val deadline = Deadline.after(ticker, -1, SECONDS) + + assertTrue(deadline.hasPassed()) + assertEquals(0, deadline.remaining(MILLISECONDS)) + } + + @Test + fun `a negative amount does not outlast a standing deadline`() { + // A bogus Retry-After must not be able to shorten a rate limit that is already in force. + val ticker = TestMonotonicTicker() + val standing = Deadline.after(ticker, 60, SECONDS) + + assertFalse(Deadline.after(ticker, -1, SECONDS).isAfter(standing)) } @Test diff --git a/sentry/src/test/java/io/sentry/transport/RateLimiterTest.kt b/sentry/src/test/java/io/sentry/transport/RateLimiterTest.kt index 36927df97dd..00ba9cd9872 100644 --- a/sentry/src/test/java/io/sentry/transport/RateLimiterTest.kt +++ b/sentry/src/test/java/io/sentry/transport/RateLimiterTest.kt @@ -3,11 +3,13 @@ package io.sentry.transport import io.sentry.Attachment import io.sentry.CheckIn import io.sentry.CheckInStatus +import io.sentry.DataCategory import io.sentry.DataCategory.Replay import io.sentry.EnvelopeReader import io.sentry.Hint import io.sentry.ILogger import io.sentry.IScopes +import io.sentry.ISentryExecutorService import io.sentry.ISerializer import io.sentry.JsonSerializer import io.sentry.NoOpLogger @@ -18,13 +20,11 @@ import io.sentry.SentryEnvelope import io.sentry.SentryEnvelopeHeader import io.sentry.SentryEnvelopeItem import io.sentry.SentryEvent -import io.sentry.SentryExecutorService import io.sentry.SentryLogEvent import io.sentry.SentryLogEvents import io.sentry.SentryLogLevel import io.sentry.SentryLongDate import io.sentry.SentryOptions -import io.sentry.SentryOptionsManipulator import io.sentry.SentryReplayEvent import io.sentry.SentryTracer import io.sentry.Session @@ -37,20 +37,21 @@ import io.sentry.protocol.Feedback import io.sentry.protocol.SentryId import io.sentry.protocol.SentryTransaction import io.sentry.protocol.User +import io.sentry.test.DeferredExecutorService import io.sentry.test.getProperty +import io.sentry.time.TestMonotonicTicker import io.sentry.util.HintUtils import java.io.File import java.util.UUID import java.util.concurrent.Future -import java.util.concurrent.atomic.AtomicBoolean -import kotlin.test.AfterTest +import java.util.concurrent.TimeUnit.MILLISECONDS +import java.util.concurrent.TimeUnit.SECONDS import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFalse import kotlin.test.assertNotNull import kotlin.test.assertNull import kotlin.test.assertTrue -import org.awaitility.kotlin.await import org.mockito.kotlin.eq import org.mockito.kotlin.mock import org.mockito.kotlin.same @@ -61,36 +62,31 @@ import org.mockito.kotlin.whenever class RateLimiterTest { private class Fixture { - val currentDateProvider = mock() + val ticker = TestMonotonicTicker() val clientReportRecorder = mock() val serializer = mock() - var executorService: SentryExecutorService? = null + val executorService = DeferredExecutorService() - fun getSUT(): RateLimiter { - val options = SentryOptions().apply { setLogger(NoOpLogger.getInstance()) } - // a real executor so scheduled rate-limit-lifted notifications actually run - val timerExecutorService = SentryExecutorService(options) - executorService = timerExecutorService - options.setTimerExecutorService(timerExecutorService) + private val config = + object : RateLimiterConfig { + override fun getLogger(): ILogger = NoOpLogger.getInstance() - SentryOptionsManipulator.setClientReportRecorder(options, clientReportRecorder) + // qualified because an unqualified `clientReportRecorder` would resolve to this object's + // own synthetic property for the getter being declared, and recurse + override fun getClientReportRecorder(): IClientReportRecorder = + this@Fixture.clientReportRecorder - return RateLimiter(currentDateProvider, options) - } + override fun getTimerExecutorService(): ISentryExecutorService = executorService + } + + fun getSUT(): RateLimiter = RateLimiter.create(ticker, config) } private val fixture = Fixture() - @AfterTest - fun `tear down`() { - // the executor's core thread never times out, so it would stay parked for the whole test JVM - fixture.executorService?.close(0) - } - @Test fun `uses X-Sentry-Rate-Limit and allows sending if time has passed`() { val rateLimiter = fixture.getSUT() - whenever(fixture.currentDateProvider.currentTimeMillis).thenReturn(0, 0, 1001) val eventItem = SentryEnvelopeItem.fromEvent(fixture.serializer, SentryEvent()) val envelope = SentryEnvelope(SentryEnvelopeHeader(), arrayListOf(eventItem)) @@ -100,6 +96,9 @@ class RateLimiterTest { 1, ) + // the shortest limit in the header has now lapsed + fixture.ticker.advance(1001, MILLISECONDS) + val result = rateLimiter.filter(envelope, Hint()) assertNotNull(result) assertEquals(1, result.items.count()) @@ -108,7 +107,6 @@ class RateLimiterTest { @Test fun `parse X-Sentry-Rate-Limit and set its values and retry after should be true`() { val rateLimiter = fixture.getSUT() - whenever(fixture.currentDateProvider.currentTimeMillis).thenReturn(0) val scopes: IScopes = mock() whenever(scopes.options).thenReturn(SentryOptions()) val eventItem = SentryEnvelopeItem.fromEvent(fixture.serializer, SentryEvent()) @@ -129,7 +127,6 @@ class RateLimiterTest { @Test fun `parse X-Sentry-Rate-Limit and set its values and retry after should be false`() { val rateLimiter = fixture.getSUT() - whenever(fixture.currentDateProvider.currentTimeMillis).thenReturn(0, 0, 1001) val scopes: IScopes = mock() whenever(scopes.options).thenReturn(SentryOptions()) val eventItem = SentryEnvelopeItem.fromEvent(fixture.serializer, SentryEvent()) @@ -143,6 +140,9 @@ class RateLimiterTest { 1, ) + // the shortest limit in the header has now lapsed + fixture.ticker.advance(1001, MILLISECONDS) + val result = rateLimiter.filter(envelope, Hint()) assertNotNull(result) assertEquals(2, result.items.count()) @@ -151,7 +151,6 @@ class RateLimiterTest { @Test fun `When X-Sentry-Rate-Limit categories are empty, applies to all the categories`() { val rateLimiter = fixture.getSUT() - whenever(fixture.currentDateProvider.currentTimeMillis).thenReturn(0) val eventItem = SentryEnvelopeItem.fromEvent(fixture.serializer, SentryEvent()) val envelope = SentryEnvelope(SentryEnvelopeHeader(), arrayListOf(eventItem)) @@ -164,12 +163,14 @@ class RateLimiterTest { @Test fun `When all categories is set but expired, applies only for specific category`() { val rateLimiter = fixture.getSUT() - whenever(fixture.currentDateProvider.currentTimeMillis).thenReturn(0, 0, 1001) val eventItem = SentryEnvelopeItem.fromEvent(fixture.serializer, SentryEvent()) val envelope = SentryEnvelope(SentryEnvelopeHeader(), arrayListOf(eventItem)) rateLimiter.updateRetryAfterLimits("1::key, 60:default;error;security:organization", null, 1) + // the shortest limit in the header has now lapsed + fixture.ticker.advance(1001, MILLISECONDS) + val result = rateLimiter.filter(envelope, Hint()) assertNull(result) } @@ -177,12 +178,14 @@ class RateLimiterTest { @Test fun `When category has shorter rate limiting, do not apply new timestamp`() { val rateLimiter = fixture.getSUT() - whenever(fixture.currentDateProvider.currentTimeMillis).thenReturn(0, 0, 1001) val eventItem = SentryEnvelopeItem.fromEvent(fixture.serializer, SentryEvent()) val envelope = SentryEnvelope(SentryEnvelopeHeader(), arrayListOf(eventItem)) rateLimiter.updateRetryAfterLimits("60:error:key, 1:error:organization", null, 1) + // the shortest limit in the header has now lapsed + fixture.ticker.advance(1001, MILLISECONDS) + val result = rateLimiter.filter(envelope, Hint()) assertNull(result) } @@ -190,12 +193,14 @@ class RateLimiterTest { @Test fun `When category has longer rate limiting, apply new timestamp`() { val rateLimiter = fixture.getSUT() - whenever(fixture.currentDateProvider.currentTimeMillis).thenReturn(0, 0, 1001) val eventItem = SentryEnvelopeItem.fromEvent(fixture.serializer, SentryEvent()) val envelope = SentryEnvelope(SentryEnvelopeHeader(), arrayListOf(eventItem)) rateLimiter.updateRetryAfterLimits("1:error:key, 5:error:organization", null, 1) + // the shortest limit in the header has now lapsed + fixture.ticker.advance(1001, MILLISECONDS) + val result = rateLimiter.filter(envelope, Hint()) assertNull(result) } @@ -203,16 +208,52 @@ class RateLimiterTest { @Test fun `When both retry headers are not present, default delay is set`() { val rateLimiter = fixture.getSUT() - whenever(fixture.currentDateProvider.currentTimeMillis).thenReturn(0, 0, 1001) val eventItem = SentryEnvelopeItem.fromEvent(fixture.serializer, SentryEvent()) val envelope = SentryEnvelope(SentryEnvelopeHeader(), arrayListOf(eventItem)) rateLimiter.updateRetryAfterLimits(null, null, 429) + // a second in, the 60s default delay is still running + fixture.ticker.advance(1001, MILLISECONDS) + val result = rateLimiter.filter(envelope, Hint()) assertNull(result) } + @Test + fun `When X-Sentry-Rate-Limit delay is negative, nothing is rate limited`() { + val rateLimiter = fixture.getSUT() + val eventItem = SentryEnvelopeItem.fromEvent(fixture.serializer, SentryEvent()) + val envelope = SentryEnvelope(SentryEnvelopeHeader(), arrayListOf(eventItem)) + + rateLimiter.updateRetryAfterLimits("-1:error:key", null, 1) + + assertNotNull(rateLimiter.filter(envelope, Hint())) + } + + @Test + fun `When Retry-After is negative, nothing is rate limited`() { + val rateLimiter = fixture.getSUT() + val eventItem = SentryEnvelopeItem.fromEvent(fixture.serializer, SentryEvent()) + val envelope = SentryEnvelope(SentryEnvelopeHeader(), arrayListOf(eventItem)) + + rateLimiter.updateRetryAfterLimits(null, "-1", 429) + + assertNotNull(rateLimiter.filter(envelope, Hint())) + } + + @Test + fun `A negative delay does not lift a standing rate limit`() { + val rateLimiter = fixture.getSUT() + val eventItem = SentryEnvelopeItem.fromEvent(fixture.serializer, SentryEvent()) + val envelope = SentryEnvelope(SentryEnvelopeHeader(), arrayListOf(eventItem)) + + rateLimiter.updateRetryAfterLimits("60:error:key", null, 1) + rateLimiter.updateRetryAfterLimits("-1:error:key", null, 1) + + assertNull(rateLimiter.filter(envelope, Hint())) + } + @Test fun `records dropped items as lost`() { val rateLimiter = fixture.getSUT() @@ -372,10 +413,22 @@ class RateLimiterTest { verifyNoMoreInteractions(fixture.clientReportRecorder) } + @Test + fun `a limit lapses exactly at its deadline, not a millisecond later`() { + val rateLimiter = fixture.getSUT() + + rateLimiter.updateRetryAfterLimits("1:error:key", null, 1) + + fixture.ticker.advance(999, MILLISECONDS) + assertTrue(rateLimiter.isActiveForCategory(DataCategory.Error)) + + fixture.ticker.advance(1, MILLISECONDS) + assertFalse(rateLimiter.isActiveForCategory(DataCategory.Error)) + } + @Test fun `any limit can be checked`() { val rateLimiter = fixture.getSUT() - whenever(fixture.currentDateProvider.currentTimeMillis).thenReturn(0) val eventItem = SentryEnvelopeItem.fromEvent(fixture.serializer, SentryEvent()) val envelope = SentryEnvelope(SentryEnvelopeHeader(), arrayListOf(eventItem)) @@ -393,7 +446,6 @@ class RateLimiterTest { @Test fun `on rate limit DiskFlushNotification is marked as flushed`() { val rateLimiter = fixture.getSUT() - whenever(fixture.currentDateProvider.currentTimeMillis).thenReturn(0) val sentryEvent = SentryEvent() val eventItem = SentryEnvelopeItem.fromEvent(fixture.serializer, sentryEvent) val envelope = SentryEnvelope(SentryEnvelopeHeader(sentryEvent.eventId), arrayListOf(eventItem)) @@ -668,14 +720,15 @@ class RateLimiterTest { @Test fun `apply rate limits schedules a task to notify observers of lifted limits`() { val rateLimiter = fixture.getSUT() - whenever(fixture.currentDateProvider.currentTimeMillis).thenReturn(0, 1, 2001) - - val applied = AtomicBoolean(true) - rateLimiter.addRateLimitObserver { applied.set(rateLimiter.isActiveForCategory(Replay)) } + var applied = true + rateLimiter.addRateLimitObserver { applied = rateLimiter.isActiveForCategory(Replay) } rateLimiter.updateRetryAfterLimits("1:replay:key", null, 1) - await.untilFalse(applied) - assertFalse(applied.get()) + // the notification was scheduled for when the limit lapses + fixture.ticker.advance(2, SECONDS) + fixture.executorService.runAll() + + assertFalse(applied) } @Test