Fix shutdown leaving a non-daemon thread alive, and bound rate limiting at 30 minutes - #540
Merged
Merged
Conversation
… min shutdownAndWait force-stopped only the looper. The network executor was given shutdown() and a 75s wait, and if it had not terminated by then the method returned — no shutdownNow(), no interrupt, no log, no exception. shutdown() then logged "Analytics client shut down in %s ms" and returned as though it had drained. ExecutorService.shutdown() does not interrupt running tasks, the task can be a whole rate-limit budget deep in a sleep, and the thread factory in Platform.java never calls setDaemon — so a live thread kept the JVM alive after shutdown() reported success. The retry sleeps have always handled InterruptedException correctly, clearing rate-limit state and returning; nothing was ever sending the interrupt. The same gap applied in the catch block, where an interrupted shutdown also only forced the looper. The log line there also printed TERMINATION_TIMEOUT_S regardless of which executor it waited on, so the network case reported 1 second after waiting 75. Separately, maxRateLimitDuration drops from 12 hours to 5 minutes and Retry-After from 300s to 60s. The 12 hour value assumed a retry count would stop us reaching it, but Retry-After retries are deliberately uncounted, so it was the only limit. At 300s a single sleep consumed a 5 minute budget, so the cap has to sit well under it. NETWORK_TERMINATION_TIMEOUT_S can stay at 75s: its comment claimed headroom over a 60s cap, which is now true. The test asserting verifyNoMoreInteractions(networkExecutor) encoded the old behaviour; it is replaced by one that drives awaitTermination to false and expects shutdownNow(). That test reports "Wanted but not invoked" against the previous code. 173 tests pass under devbox (JDK 11).
Less conversational, and no references to other SDKs — someone reading this library's notes has no context for them. Unlike the other clients, the 12 hour and 300 second values here did ship, in 3.5.5, so these entries do describe a change from one released state to another and say what to pass to restore the previous limit.
Making shutdown actually stop the network executor turned two latent paths live, and both lose data without telling anyone. A batch interrupted while waiting out a Retry-After or a backoff returned straight out of the retry loop. Every other exit from that loop — success, retries exhausted, duration exceeded, non-retryable status — reports the batch through Callback. These two did not, and could not be reached before, because nothing interrupted that thread. Both now report an IOException. shutdownNow() hands back the tasks that were submitted and never ran. The code logged how many there were. For the looper that is nothing, since it only ever holds one task, but for the network executor those are real batches, and previously they always eventually ran. They now report failure too. Both tests fail against the code without these changes, reporting "Wanted but not invoked" for the callback. One limitation this does not address, worth stating plainly: the interrupt only helps a thread parked in a sleep. OkHttp's socket reads are governed by SO_TIMEOUT rather than interruption, so a thread inside the HTTP call itself is unaffected and shutdown still waits for the client's own timeouts — 15s each by default, but unbounded if a caller supplies an OkHttpClient without them. 173 tests pass under devbox JDK 11.
Every other client clamps this; java was the one I missed. It tested the budget and then slept the full Retry-After regardless, so an episode could run past its limit by up to one wait. Against twelve hours that was 0.14% and not worth noticing. Against five minutes it is 20% — a 300 second budget running to 360 — which is the overshoot this whole change exists to close. setRateLimitStateAndCheckDuration returned a boolean, which meant the caller had no way to know how much budget was left without reading the clock a second time. It now returns the remaining milliseconds, so one reading serves both the test and the wait, the same discipline applied to ruby and go. The boundary moves from "elapsed > budget" to "elapsed >= budget", matching the other clients. 176 tests pass under devbox JDK 11.
Applying the team convention to my own work from today. The comments explaining these changes had accumulated into potted histories: why a value had been twelve hours, what a test used to assert, which path used to be unreachable. Six months from now none of that resolves to anything — the diff and the commit messages hold it, and the comment should say why the code is the way it is. What stayed is what a maintainer would undo without it: that Kernel#sleep raises on a negative interval, that Thread#wakeup only interrupts a sleep already in progress, that OkHttp's reads are governed by SO_TIMEOUT so an interrupt does not reach them, and that inverting one assertion would make the duration budget unreachable again. Comments only, no behaviour change.
Capping at 60s meant waiting less than the server asked for, which does not make the next attempt more likely to succeed — it just sends more requests at something already rate-limiting us. Against a Retry-After of 180s inside a 5 minute budget it turns 3 requests into 6; against 300s it turns 2 into 6. The cap is a guard against an absurd header, not a second budget. How long we keep trying is max_rate_limit_duration's job, and the clamp to the remaining budget already stops a single wait running past it, so the cap now rarely binds at all. It also bought nothing for the client this was partly aimed at: with no background thread, a shorter cap turns one long wait into several short ones for the same total blocking time and more requests. Tests that pinned 60 are updated, and each SDK gains one asserting that a Retry-After inside the cap is used as given rather than shortened.
The budget and the Retry-After cap were both 300s, and at parity the rate-limit path degenerates. A response with no usable Retry-After waits the cap by default, the elapsed check runs before the wait, so that one wait spends the whole budget and the batch is dropped having been tried once. A legitimate Retry-After of 300 does the same. The cap also stops binding: whatever is left of the budget is always the smaller term, so the cap can never be the value that clamps. Thirty minutes restores the relationship the two knobs are meant to have — the cap bounds one wait, the budget bounds the episode — and leaves room for several attempts. It costs nothing in normal operation, since the budget only binds when the server has been rate-limiting us for a long time, and in that case keeping the data is the point.
MichaelGHSeg
changed the title
The notification added for batches discarded at shutdown could never fire. Batches reached the network executor via submit(), which wraps a Runnable in a FutureTask and queues the wrapper, so shutdownNow() handed back FutureTasks and the `instanceof BatchUploadTask` test was never true. The log line counted them and nothing else happened -- exactly the behaviour the changelog says was fixed. execute() queues the task itself; its Future was discarded anyway, and it throws the same RejectedExecutionException already handled at the call site. The test could not have caught this: it stubs a mock executor to return a bare BatchUploadTask, which no real executor does. There is now a test driving a real ThreadPoolExecutor, which is what makes the wrapping visible. It documents the JDK contract rather than guarding our call site; the guard is the set of verifications that now assert execute(), which fail if the call reverts. Two related gaps closed: The interrupted-shutdown path called shutdownNow() without notifying anything, so an interrupt during the network executor's 75 second wait still discarded queued batches silently. Both paths now share one helper. setRateLimitStateAndRemaining took a second clock reading, so the budget test and the clamp did not share one -- the thing its own javadoc said they did. setRateLimitState now returns the reading it used. Immaterial in production at millisecond granularity, but a test running with a 1ms budget is flaky as a result, and ruby has the same defect where it turns CI red. Left alone and documented instead: a ForkJoinPool returns an empty list from shutdownNow() whatever is queued, and a ScheduledThreadPoolExecutor wraps even execute(), so a caller supplying either through Builder#networkExecutor still gets no callbacks. Noted on the helper. 177 tests pass, spotless clean.
Four review points. execute() removed the FutureTask that used to absorb anything thrown out of the upload task. Callback and Log are caller-supplied and are invoked from inside the retry loop outside any try, so a callback that throws would kill and replace the pool's worker and reach the application's uncaught-exception handler -- from a library that previously could not raise one. The loop now runs inside a try/catch(Throwable) that logs, restoring the property the old code had by accident. A test drives a throwing Callback on a real thread with a recording handler and fails without the guard. The dropped-batch notification on the interrupted-shutdown path ran after the interrupt flag was restored, so every callback it invoked started with the flag set and any interruptible call inside one -- a queue put, an await, a Future.get -- threw immediately. It now reports first and restores the flag afterwards. A Retry-After that will not fit the remaining budget ends the episode instead of being shortened, for the reason raised in review: resuming inside the window the server named sends a request it has already declined to serve, and the budget is spent by then so it would be the final attempt regardless. That also settles a test that was measurably flaky. retryAfterCappedAtMaxRateLimitedSeconds runs with a 1ms budget, where whether the second attempt happened at all depended on whether the millisecond ticked between two clock reads. It is now one attempt, deterministically. CHANGELOG additions for two things it did not mention: the at-least-once consequence of reporting a batch that may in fact have been delivered, and that the budget is measured against the system clock. 178 tests pass, spotless clean.
didiergarcia
approved these changes
Sep 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes shutdown leaving a non-daemon thread alive, and bounds the rate-limit retry path.
Shutdown
shutdown()stops the network executor rather than leaving it running. It was asked to stop, given up to 75 seconds, and thenshutdown()returned regardless. SinceExecutorService.shutdown()does not interrupt running tasks, a thread waiting out aRetry-Afterkept running, and as these are non-daemon threads the JVM would not exit — whileshutdown()reported success.execute()rather thansubmit().submit()wraps the task in aFutureTaskand queues the wrapper, soshutdownNow()returnedFutureTasks and the batches inside them could not be identified, let alone reported.Callback, on both the normal and the interrupted path. Note this is a failure notification for a batch that may in fact have been delivered: one interrupted after its request went out is reported as failed because the client cannot know. Delivery has always been at-least-once; this makes the uncertainty visible.Rate limiting
maxRateLimitDurationis 30 minutes, was 12 hours. This is the one SDK where the 12 hour default shipped — 3.5.5 — so it is the one where a customer loses a behaviour they had.Analytics.Builder.maxRateLimitDurationrestores it.Retry-Afterthat will not fit in what is left of the budget ends the episode rather than being shortened.setRateLimitStateAndRemainingtakes one clock reading for the budget test and the wait.Notes for review
BatchUploadTask.run()now runs insidetry/catch(Throwable).execute()removed theFutureTaskthat used to absorb anything thrown, andCallback/Logare caller-supplied and invoked outside any try, so a callback that throws would otherwise kill and replace the pool's worker and reach the application's uncaught-exception handler — from a library that previously could not raise one.SO_TIMEOUT, so a thread inside the HTTP call is bounded by the client's own timeouts, and by nothing if anOkHttpClientwithout them is supplied. Separately, a caller supplying aForkJoinPoolorScheduledThreadPoolExecutorviaBuilder.networkExecutorstill gets no shutdown callbacks — the former returns nothing fromshutdownNow(), the latter wraps tasks regardless. Both are inCHANGELOG.md.maxRateLimitDurationis measured against the system clock, so a large adjustment mid-episode shortens or extends it.