Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c98d886c-c9b6-42aa-ac1e-990eb589d5c0
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c98d886c-c9b6-42aa-ac1e-990eb589d5c0
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c98d886c-c9b6-42aa-ac1e-990eb589d5c0
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c98d886c-c9b6-42aa-ac1e-990eb589d5c0
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c98d886c-c9b6-42aa-ac1e-990eb589d5c0
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c98d886c-c9b6-42aa-ac1e-990eb589d5c0
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #684 +/- ##
========================================
Coverage 100.0% 100.0%
========================================
Files 554 555 +1
Lines 61509 60642 -867
========================================
- Hits 61509 60642 -867
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c98d886c-c9b6-42aa-ac1e-990eb589d5c0
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c98d886c-c9b6-42aa-ac1e-990eb589d5c0
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c98d886c-c9b6-42aa-ac1e-990eb589d5c0
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c98d886c-c9b6-42aa-ac1e-990eb589d5c0
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c98d886c-c9b6-42aa-ac1e-990eb589d5c0
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c98d886c-c9b6-42aa-ac1e-990eb589d5c0
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c98d886c-c9b6-42aa-ac1e-990eb589d5c0
|
martintmk
left a comment
There was a problem hiding this comment.
Posted by an AI agent
I found one test gap that leaves the new no-double-wrap invariant unprotected, plus two non-blocking coverage/naming improvements. The intentional public API breaks were not raised: the existing PR discussion explicitly requested the constructor removal and CustomDeps rename, and release tooling handles versioning. Correctness, resilience, telemetry, consistency, and measured performance checks found no runtime regression. cargo check -p bytesbuf -p fetch -p http_extensions --all-targets --all-features passed, and current required CI is green.
| } | ||
|
|
||
| #[test] | ||
| fn accepts_existing_opaque_memory() { |
There was a problem hiding this comment.
Posted by an AI agent
The no-double-wrap regression test cannot distinguish nested wrappers
Problem
accepts_existing_opaque_memory only checks that reserve still works after wrapping twice. The previous implementation that always nested an OpaqueMemory produces the same capacity, so this test stays green if the new TypeId/downcast branch is removed.
Why this matters
The no-double-wrap behavior requested in this PR can regress without this test detecting it.
Suggested fix
Assert an internal invariant that distinguishes one erased layer from two, such as a test-only depth or type probe.
|
|
||
| #[cfg_attr(miri, ignore)] | ||
| #[tokio::test] | ||
| async fn custom_deps_accept_custom_opaque_memory() { |
There was a problem hiding this comment.
Posted by an AI agent · Non-blocking
The custom-provider test does not prove provider wiring
Problem
This custom provider behaves exactly like GlobalPool, and the test only checks the HTTP status. It would still pass if CustomDeps::memory were ignored and the default pool were substituted; the corresponding HttpBodyBuilder test has the same gap.
Why this matters
The new provider-plumbing contract can break without either test detecting that body allocation bypasses the supplied provider.
Suggested fix
Use a counting or callback provider and assert that body allocation reaches it.
| pub clock: Clock, | ||
| /// Memory pool for usage-neutral memory allocations. | ||
| pub global_pool: bytesbuf::mem::GlobalPool, | ||
| pub global_pool: bytesbuf::mem::OpaqueMemory, |
There was a problem hiding this comment.
Posted by an AI agent · Non-blocking
The Tokio dependency field understates the provider contract
Problem
global_pool now accepts an OpaqueMemory, so it can hold any MemoryShared provider rather than specifically a GlobalPool. CustomDeps::memory already uses the broader name.
Why this matters
The two public dependency structs describe the same contract inconsistently, which can mislead callers about what providers the Tokio path accepts.
Suggested fix
Rename this field and its generated builder and example call sites to memory.
Summary
HttpBodyBuilderto storeOpaqueMemorywhile accepting anyMemorySharedproviderOpaqueMemory::newpreserve an existingOpaqueMemoryinstead of adding another wrapperOpaqueMemory, allowing customers to choose their memory providerGlobalPoolas the default provider and retain the existingbytesbuftype namesValidation