perf: Schedule rate-limit notifications on shared executor (JAVA-653) - #5814
perf: Schedule rate-limit notifications on shared executor (JAVA-653)#5814runningcode wants to merge 2 commits into
Conversation
🚨 Detected changes in high risk code 🚨High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:
|
1 similar comment
🚨 Detected changes in high risk code 🚨High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:
|
📲 Install BuildsAndroid
|
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 05aa61d | 326.06 ms | 385.46 ms | 59.40 ms |
| bb0ff41 | 315.84 ms | 350.76 ms | 34.92 ms |
| 806307f | 357.85 ms | 424.64 ms | 66.79 ms |
| d501a7e | 307.33 ms | 341.94 ms | 34.61 ms |
| 0ee65e9 | 321.06 ms | 361.24 ms | 40.18 ms |
| ed33deb | 334.19 ms | 362.30 ms | 28.11 ms |
| 9fbb112 | 401.87 ms | 515.87 ms | 114.00 ms |
| b8bd880 | 314.56 ms | 336.50 ms | 21.94 ms |
| 5b1a06b | 315.40 ms | 353.33 ms | 37.94 ms |
| 6edfca2 | 316.43 ms | 398.90 ms | 82.46 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 05aa61d | 0 B | 0 B | 0 B |
| bb0ff41 | 0 B | 0 B | 0 B |
| 806307f | 1.58 MiB | 2.10 MiB | 533.42 KiB |
| d501a7e | 0 B | 0 B | 0 B |
| 0ee65e9 | 0 B | 0 B | 0 B |
| ed33deb | 1.58 MiB | 2.13 MiB | 559.52 KiB |
| 9fbb112 | 1.58 MiB | 2.11 MiB | 539.18 KiB |
| b8bd880 | 1.58 MiB | 2.29 MiB | 722.92 KiB |
| 5b1a06b | 0 B | 0 B | 0 B |
| 6edfca2 | 1.58 MiB | 2.13 MiB | 559.07 KiB |
Previous results on branch: no/perf/reuse-timer-executor
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 91c5190 | 392.32 ms | 460.78 ms | 68.46 ms |
| 50f90d3 | 319.27 ms | 373.26 ms | 53.99 ms |
| a70f7a7 | 324.94 ms | 379.86 ms | 54.92 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 91c5190 | 0 B | 0 B | 0 B |
| 50f90d3 | 0 B | 0 B | 0 B |
| a70f7a7 | 0 B | 0 B | 0 B |
81bc25c to
00ed5c3
Compare
🚨 Detected changes in high risk code 🚨High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:
|
00ed5c3 to
4f33cf7
Compare
🚨 Detected changes in high risk code 🚨High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:
|
4f33cf7 to
b040430
Compare
🚨 Detected changes in high risk code 🚨High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:
|
| fun getSUT(): RateLimiter { | ||
| val options = SentryOptions().apply { setLogger(NoOpLogger.getInstance()) } | ||
| // a real executor so scheduled rate-limit-lifted notifications actually run | ||
| options.setTimerExecutorService(SentryExecutorService(options)) |
There was a problem hiding this comment.
do we need to close it in teardown perhaps, so it doesn't leak across tests?
| timer = new Timer(true); | ||
| // notify observers again once the rate limit is lifted, using the shared timer executor | ||
| // instead of a dedicated Timer thread | ||
| try (final @NotNull ISentryLifecycleToken ignored = notifyFuturesLock.acquire()) { |
There was a problem hiding this comment.
l: unsure if we need to protect this with checking for instanceof NoOpSentryExecutorService to avoid allocating FutureTasks for nothing (isDone also returns false when no-op, so pruning wouldn't do anything), but we're not really doing that anywhere else I believe, so I'm fine with keeping it as-is.
There was a problem hiding this comment.
Yeah it is a good point but also quite an edge case. Since we aren't doing it anywhere else, I don't think we should do it here.
b040430 to
b202421
Compare
RateLimiter created a java.util.Timer whose thread stayed alive forever once the SDK got rate limited. Schedule the "rate limit lifted" observer notification on the shared timer executor instead, whose single worker thread is reused across all timeouts and self-terminates when idle. Pending notifications are cancelled on close(). Co-Authored-By: Claude Fable 5 <[email protected]>
b202421 to
472e0fb
Compare
🚨 Detected changes in high risk code 🚨High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 472e0fb. Configure here.
| notifyObserversFutures.add( | ||
| options | ||
| .getTimerExecutorService() | ||
| .schedule(this::notifyRateLimitObservers, delayMillis)); |
There was a problem hiding this comment.
Shutdown stalls on pending rate-limit tasks
Medium Severity
Rate-limit lift notifications are now scheduled on the shared timer executor, often with long delays. Scopes.close() shuts that executor down before RateLimiter.close() can cancel those futures, so awaitTermination waits out the full shutdown timeout whenever a limit is still pending. The old dedicated Timer was cancelled independently and did not block SDK shutdown.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 472e0fb. Configure here.


📜 Description
RateLimitercreated ajava.util.Timer(a dedicated thread) whose thread stayed alive for the rest of the process once the SDK got rate limited. The "rate limit lifted" observer notification now runs on the shared timer executor (SentryOptions#getTimerExecutorService), already used for transaction timeouts, whose single worker thread is reused and self-terminates when idle.getCurrentTimeMillis()call the already-computedretryAfterMillisis passed through as the delay.close(), preserving the oldTimer#cancel()semantics.💡 Motivation and Context
Part of reducing the number of threads created by the SDK: JAVA-653.
Once an app got rate limited, this timer thread lived forever. The shared timer executor's worker is reused and idles out.
💚 How did you test it?
Existing
RateLimiterTest, adapted from the Timer-mock verification to the executor/future model, plusAsyncHttpTransportTest.📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps
Related PRs in this effort: LifecycleWatcher (#5819), performance collector (#5816), HostnameCache (#5817), batch processors (#5818).
🤖 Generated with Claude Code