Provide an opt-in zero-copy response body view - #2322
Conversation
Byte-array consumers currently pay for an aggregate copy even when a response has a single body part. Add an explicit read-only view accessor so callers can opt into sharing while the existing accessor retains its defensive-copy contract. Reuse the view for string decoding and cover eager, lazy, multipart, and third-party Response implementations. Refs AsyncHttpClient#2321 Codex on behalf of Pavel Ptashyts Co-Authored-By: OpenAI Codex <noreply@openai.com>
Mockito 4 cannot invoke interface default methods through CALLS_REAL_METHODS on JDK 21 and newer. Invoke the Response default method explicitly so delegation remains covered on every supported JDK. Refs AsyncHttpClient#2321 Codex on behalf of Pavel Ptashyts Co-Authored-By: OpenAI Codex <noreply@openai.com>
Review feedback on the response body view. The contract was written as though the sharing were the response's to give. It is not. Whether a lone body part exists to share from is a fact about how the body arrived - how the origin chunked it, whether a proxy re-chunked it, whether it was compressed - so the same body from the same server can be shared on one response and copied on the next, and the caller sees no difference. And where an array is shared it is not the response's alone: it is reachable from the part handed to onBodyPartReceived, it is what a TransferCompletionHandler gives each TransferListener, and it is what getResponseBodyAsByteBuf wraps. A write through any of those changes what this returns, and a write through this changes what they see. None of that can be designed away while the method returns a byte array, so the javadoc says it, and says the identity between calls is not guaranteed either. It no longer promises anything on behalf of getResponseBodyAsBytes, which is an interface method whose own contract guarantees no independent array. It records that leaving the default in place while implementing getResponseBodyAsBytes in terms of this method makes the two call each other, which is the first thing an implementor would otherwise write. getResponseBody(Charset) goes back through the private helper rather than the new public method, so overriding the view cannot change what a response's text says as well; the helper keeps the note about multi-byte characters straddling a part boundary, which is the only place that reason is written down, and now says that the array does leave the client rather than claiming it does not. An empty body returns a shared empty array rather than walking the aggregating path to allocate one and a buffer to wrap it. Nothing can be written through a zero-length array. The tests asserted identity where they meant content, so an implementation that shared a corrupted array satisfied them, and one took its expectation from the array it had handed to the part, which would have stopped being an oracle the moment the part stopped copying. The lazy coverage this branch added is dropped: a Response built from lazy parts holds buffers at refCnt 0 on a real request, since channelRead releases in a finally and LazyResponseBodyPart never retains, and a test that keeps one alive by hand signs off on a mode that does not work. That branch is still covered through getResponseBody(Charset), which shares the helper. Claude Code on behalf of Pavel Ptashyts Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Round 1 addressed. Two of the comments are about the contract rather than the code, and they are right that neither can be designed away while the method returns a That leaves an accessor whose honest description is "will not copy when it does not have to, and will not tell you when that is". If that is not worth an interface method, I would rather you said so than have me argue it - it closes cleanly, and The rest:
|
Summary
Response#getResponseBodyAsBytesView()as a compatible default methodwith an explicit possibly-shared, read-only contract
aggregate copy without exposing
ByteBufstoragegetResponseBodyAsBytes(),getResponseBodyAsByteBuffer(), andgetResponseBodyAsStream()centralized
Closes #2321.
Compatibility and safety
The default implementation delegates to
getResponseBodyAsBytes(), so existingthird-party
Responseimplementations keep their current behavior. Netty usesthe optimized path only for exactly one body part. Empty and multipart bodies
continue through the existing aggregation path.
Tests cover eager identity and repeated access, defensive-copy isolation,
heap and direct lazy slices, reader/writer indices, reference counts, empty
bodies, multipart ordering and split UTF-8 characters, stream isolation, and
default-method delegation.
Verification
mvnw.cmd -B -ntp -pl client -Dtest=NettyAsyncResponseTest testmvnw.cmd clean verifyAllocation benchmark
JMH 1.37, Corretto 11.0.32.1, one fork, three 500 ms warmups, five
500 ms measurements,
gcprofiler. Values are allocated bytes per operation,rounded to whole bytes; zero means below the profiler's resolution.
getResponseBodyAsBytes()getResponseBodyAsBytesView()The eager single-part view measured about 2.3 ns/op regardless of body size,
versus 322 ns/op for 4 KiB and 11.0 us/op for 128 KiB defensive copies. Lazy
single-part allocation is halved as expected. Multipart bodies retain the
payload-sized aggregation allocation.
Codex on behalf of Pavel Ptashyts