Handle zero size hints in Http1OutputProducer - #69005
Open
DeagleGross wants to merge 2 commits into
Open
Conversation
DeagleGross
requested review from
BrennanConroy and
SamMonoRT
as code owners
September 2, 2026 17:34
DeagleGross
added
the
area-networking
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The new test disposes the producer inside a using var scope, causing a double-dispose that should be avoided (use Stop() or otherwise ensure a single dispose).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
 |
src/Servers/Kestrel/Core/test/Http1/Http1OutputProducerTests.cs — output is declared with using var, but the test also calls Dispose() explicitly. This results… |
What changed in this PR
This PR fixes Http1OutputProducer’s handling of sizeHint: 0 by ensuring GetMemory(0)/GetSpan(0) never indirectly rent empty buffers from MemoryPool<byte>, aligning behavior with the IBufferWriter<byte> contract and Kestrel’s segment sizing.
Changes:
- Normalize
sizeHint == 0to Kestrel’s minimum segment size before renting in pre-start buffering (AddSegment) and completed-output scratch memory (GetFakeMemory). - Add a regression test ensuring
GetMemory(0)/GetSpan(0)return non-empty buffers for both pre-start and completed output paths. - Extend the test helper to allow injecting a custom
MemoryPool<byte>.
| File | Description |
|---|---|
| src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs | Normalizes zero size hints to a minimum segment size before renting buffers. |
| src/Servers/Kestrel/Core/test/Http1/Http1OutputProducerTests.cs | Adds a regression test and enables injecting a custom MemoryPool<byte> into the producer. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
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.
IBufferWriter<byte>.GetMemory(0)andGetSpan(0)must return non-empty buffers, butHttp1OutputProducerpassed zero directly toMemoryPool<byte>.Rent().MemoryPool<byte>.Rent(0)only guarantees memory with at least zero elements, andMemoryPool<byte>.Sharedreturns empty memory for that request. This conflicts with the strongerIBufferWriter<byte>contract, which requiresGetMemory(0)andGetSpan(0)to return non-empty buffers.This change normalizes zero size hints to Kestrel's minimum segment size before renting memory in the pre-start buffering and completed-output scratch-memory paths. Using the existing minimum segment size keeps allocation behavior consistent with Kestrel's pipe configuration and avoids inefficient tiny buffers.
Fixes #58644