Repository navigation
fix(throttle): close the request body on throttle errors and log at debug - #22
Merged
Merged
Conversation
…ebug - RoundTrip closes r.Body when the early context check, the limiter wait, or the post-wait context check fails. - The RoundTripper contract requires the transport to close the body on errors. http.Client does not close it after a transport error. - The throttle logs "tokens exhausted" and "wait complete" at Debug level. - The throttle reads the token count only when the logger has Debug enabled. - The throttle logs "wait complete" only after a successful wait.
- The throttle logs both debug lines with DebugContext, so the records use the context that Enabled checks. - The debug log test drops records that lack the request context, so it fails if a line logs without it. - A comment explains why RoundTrip discards the body close error. - The request body test spends tokens from a count field instead of a per-row branch. - The debug log test spaces tokens 400ms apart, so a slow runner is less likely to refill the bucket early. - Two test helper comments that restated the code are gone.
- The WithThrottleEvery change removed the exported throttle.Config type. Every client release from v0.0.2 to v0.0.7 exports it. - Config returns with its released fields, RPS and Burst, so code that names it compiles again. - A Deprecated notice says that nothing in the module reads Config, and it names NewRoundTripper and NewRoundTripperEvery instead.
… waits - The burst test checks that the first two requests succeed and that the third fails its token wait with ErrWaitingFailed. - All three burst requests share a one-minute deadline, so the limiter rejects any wait past the burst at once. - The cancel test no longer requires ErrWaitingFailed. A cancel that fires before the request reaches the limiter returns ErrContextEnded, which is also correct. - A doWithin helper fails a test when Do runs longer than a second, so a wait that ignores its context fails fast instead of hanging until the test timeout. - The burst test, the WithThrottleEvery cancel test, and the throttled retry cancel test send through doWithin.
adamwoolhether
marked this pull request as ready for review
October 6, 2026 05:11
adamwoolhether
added a commit
that referenced
this pull request
Oct 6, 2026
This PR removes the deprecated `throttle.Config` type that #22 restored. ## Changes - `client/throttle` no longer exports `Config`. - No exported API accepts `Config`, and nothing in the module reads it. In `client/v0.0.7`, only the unexported client options used it. - `client/v0.1.0` ships `Config` as deprecated. The next client release removes it. - **Breaking for code that names `throttle.Config`:** that code no longer compiles. Such code could only build a `Config` value, because no function takes one.
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
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.
The throttle round tripper now closes the request body when it fails a request. It also logs at Debug level instead of Info.
Changes
RoundTripclosesr.Bodywhen the early context check, the limiter wait, or the post-wait context check fails. Thehttp.RoundTrippercontract requires the transport to close the body on errors.http.Clientdoes not close it after a transport error.RoundTrippassesrto the next round tripper unchanged and leaves the body open.Tests
TestThrottleRoundTripper_ClosesBodyOnError: each of the three error returns closes the body once and sends nothing to the next round tripper.TestThrottleRoundTripper_PassesRequestOn: on success, the next round tripper receives the same request with the body open.TestClient_Retry_CanceledDuringThrottleWait: withWithThrottleEveryandWithRetry, a context cancelled during the wait returns an error that wrapscontext.Canceled, and the body closes once.TestThrottleRoundTripper_LogsAtDebug: a failed wait logs no "wait complete" line, a successful wait logs one, and every throttle line is at Debug level. Its handler drops records that lack the request context.TestThrottleRoundTripper_LogsRateandTestThrottleRoundTripper_LoggerTakesOneTokennow read a Debug-level handler.Follow-ups to #20
throttle.Configreturns as a deprecated type with its released fields,RPSandBurst. feat(client): add WithThrottleEvery for fractional throttle rates #20 removed it, but every client release from v0.0.2 to v0.0.7 exports it.TestClient_WithThrottleEvery_Burstchecks each request result. The first two requests succeed, and the third fails its token wait withErrWaitingFailed.TestClient_WithThrottleEvery_CanceledDuringWaitno longer requiresErrWaitingFailed, because a cancel that fires before the limiter wait returnsErrContextEnded.doWithintest helper fails a test whenDoruns longer than a second. The burst test and both throttle cancel tests send through it, so a wait that ignores its context fails fast instead of hanging until the test timeout.Closes #21