Store-gateway: drain chunk range reader before close to allow connection reuse - #16338
Store-gateway: drain chunk range reader before close to allow connection reuse#16338omkar619-dev wants to merge 3 commits into
Conversation
Signed commits reportAll 3 commits between |
Signed commits report1 of 1 commit between
This repository requires all commits to be signed. See GitHub docs on commit signature verification. |
|
|
|
|
The chunks range reader is fetched using estimated chunk lengths, so the requested range frequently extends past the end of the last chunk actually read. net/http cannot return a connection to the idle pool when the response body has unread bytes, so every chunk range fetch discarded its connection and dialed a new one. Drain the remaining bytes before closing so the connection can be reused. Signed-off-by: Omkar Shendge <omkarshendge619@gmail.com>
Signed-off-by: Omkar Shendge <omkarshendge619@gmail.com>
8cad8fe to
c3f9747
Compare
aknuds1
left a comment
aknuds1
left a comment
There was a problem hiding this comment.
I think we shouldn't be draining the reader in error cases.
| // Drain any unread bytes of the fetched range before closing it, so the underlying HTTP | ||
| // connection can be returned to the idle pool and reused. The range is fetched using | ||
| // estimated chunk lengths, so it frequently extends past the last chunk actually read; | ||
| // closing a response body with unread bytes causes net/http to discard the connection. | ||
| defer func() { | ||
| _, _ = io.Copy(io.Discard, bucketReader) | ||
| runutil.CloseWithLogOnErr(r.block.logger, bucketReader, "readChunkRange close range reader") | ||
| }() |
There was a problem hiding this comment.
Please undo this change, and only add the following before returning nil at the end of this function, so the reader is only drained in the success case:
// Chunk lengths are estimated, so drain any overfetched tail to make the connection reusable.
_, _ = io.Copy(io.Discard, reader)I would also suggest adding a corresponding regression test.
There was a problem hiding this comment.
Good point actually — the deferred version ran on every error return too, which is wasted work when we're
aborting anyway. Reverted the defer and moved the drain to just before the success return nil,
using reader as you suggested so the buffered bytes are accounted for.
Also added TestBucketChunkReader_loadChunks_drainsOverfetchedTail. It over-estimates the chunk
length so the fetched range includes a tail, then asserts every served byte was consumed. Worth
noting the tail has to exceed EstimatedMaxChunkSize — with a smaller one, bufio read-ahead
consumes it incidentally and the test passes even without the fix. Verified against unpatched code,
where it fails with 16000 of 64106 bytes consumed, i.e. exactly one buffer fill.
Address review feedback: draining in a defer also ran on the error returns, which is wasted work when the request is being aborted anyway. Drain immediately before the success return instead. Add TestBucketChunkReader_loadChunks_drainsOverfetchedTail, which over-estimates the chunk length so the fetched range includes a tail larger than the reader's bufio buffer, then asserts the whole range is consumed. Without the drain it consumes only 16000 of 64106 bytes, which is exactly one EstimatedMaxChunkSize buffer fill. Signed-off-by: Omkar Shendge <omkarshendge619@gmail.com>
|
please rebase this PR on latest main, otherwise it can't merge — it's blocked on required checks that will never report. context: #16335 changed the unit test CI matrix and the required checks got renamed ( |
3 participants
What this PR does
Drains the chunks range reader before closing it in
bucketChunkReader.loadChunks, so the underlying HTTP connection is returned to the idle pool instead of being discarded.The range is fetched using estimated chunk lengths, so it frequently extends past the end of the last chunk actually read.
net/httpcannot reuse a connection whose response body has unread bytes, so every chunk range fetch was costing a fresh TCP (and TLS) connection.Reproduced in isolation — 100 identical 1 MiB ranged GETs against MinIO, TCP dials counted by hooking
Transport.DialContext:There is no size threshold: 8 KiB through 4 MiB ranges all produce 100 dials when the body is abandoned. Repro: https://gist.github.com/omkar619-dev/23460d8af1d421479be9804c857ca327
Same class of fix as #16303 (Memcached connection reuse).
Which issue(s) this PR fixes
Fixes #13841
Approach confirmed by @narqo in #13841 (comment) ("simply draining the connection in-place will be enough").
Notes for reviewers
fetchChunkRemainderuses the same non-draining close but reads an exact length viaio.ReadFull, so it likely consumes fully. Left unchanged to keep this minimal — happy to include it if you'd prefer.SkipTo, so only the segment past the last chunk remains, on the order ofEstimatedMaxChunkSize.Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]