Skip to content

Commit 73a9ed8

Browse files
authored
Fix response body accessors for bodyless and rebuilt responses (#2329)
Motivation: `ResponseBuilder#build()` hands out the list it accumulates into, and `AsyncCompletionHandler` resets that builder on every `onStatusReceived`. A handler reused for a second request empties the first response and refills it with the second body. `NettyResponse#getResponseBodyAsByteBuf()` sizes a composite to the part count, and `CompositeByteBuf` rejects zero components, so a bodyless response throws instead of returning an empty buffer. The body view added in #2322 names three of the four holders of its array, and its `NettyResponse` override replaced the inherited description, dropping the read-only warning from the only published implementation. Modification: Copy the parts in `build()`, floor the composite at one component, name `getBodyByteBuffer()` as the fourth holder, and restore the inherited text with `{@inheritdoc}`. Result: A response keeps its own body when its builder is reused, a bodyless response yields an empty buffer, and the view names every holder it has.
1 parent c2da749 commit 73a9ed8

4 files changed

Lines changed: 134 additions & 15 deletions

File tree

‎client/src/main/java/org/asynchttpclient/Response.java‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -63,15 +63,19 @@ public interface Response {
6363
* body part's own array, it is the array reachable from the part handed to
6464
* {@link AsyncHandler#onBodyPartReceived}, the one each
6565
* {@link org.asynchttpclient.handler.TransferListener} is given by a
66-
* {@link org.asynchttpclient.handler.TransferCompletionHandler}, and the one
66+
* {@link org.asynchttpclient.handler.TransferCompletionHandler}, the one
67+
* {@link HttpResponseBodyPart#getBodyByteBuffer()} wraps for a
68+
* {@link org.asynchttpclient.handler.resumable.ResumableListener}, and the one
6769
* {@link #getResponseBodyAsByteBuf()} wraps. Writing to it changes what all of those see, and a write
6870
* through any of them changes what this returns.
6971
*
7072
* <p>Whether anything is shared at all is not something to rely on. It depends on how the body happened to
71-
* arrive - how the origin chunked it, whether a proxy re-chunked it, whether it was compressed - and on the
72-
* body parts the implementation was given, none of which is visible from here. The same body from the
73-
* same server may be shared on one response and copied on the next. No array identity is guaranteed between
74-
* calls either.
73+
* arrive - how the origin chunked it, whether a proxy re-chunked it, whether it was compressed - on how the
74+
* client happened to read it off the wire, which varies with connection age and client configuration, and on
75+
* the body parts the implementation was given. The same body from the same server may be shared on one
76+
* response and copied on the next. Which of the two a caller gets is the peer's choice rather than the
77+
* caller's, so code that writes to the array can behave one way against a friendly server and another way
78+
* against a hostile one. No array identity is guaranteed between calls either.
7579
*
7680
* <p>A caller that needs an array it may modify should copy what it receives. {@link
7781
* #getResponseBodyAsBytes()} is the accessor to reach for first, but it is implemented by whoever implements
@@ -253,7 +257,9 @@ public void accumulate(HttpResponseBodyPart bodyPart) {
253257
* @return a {@link Response} instance
254258
*/
255259
public @Nullable Response build() {
256-
return status == null ? null : new NettyResponse(status, headers, bodyParts);
260+
// A copy: reset() clears this list, and a builder may build more than once, so a response that held
261+
// the builder's own list would lose its body or take on the next response's.
262+
return status == null ? null : new NettyResponse(status, headers, new ArrayList<>(bodyParts));
257263
}
258264

259265
/**

‎client/src/main/java/org/asynchttpclient/netty/NettyResponse.java‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -196,6 +196,8 @@ public byte[] getResponseBodyAsBytes() {
196196
}
197197

198198
/**
199+
* {@inheritDoc}
200+
* <p>
199201
* Returns a lone body part's array; concatenates into one of its own when there are several, or an empty
200202
* array when there are none. Which of those a given response takes is not a property of the body: see
201203
* {@link Response#getResponseBodyAsBytesView()}, whose contract is deliberately weaker than this.
@@ -228,7 +230,9 @@ public ByteBuffer getResponseBodyAsByteBuffer() {
228230

229231
@Override
230232
public ByteBuf getResponseBodyAsByteBuf() {
231-
CompositeByteBuf compositeByteBuf = ByteBufAllocator.DEFAULT.compositeBuffer(bodyParts.size());
233+
// At least one component: CompositeByteBuf rejects a maxNumComponents of 0, so a bodyless response
234+
// would otherwise throw here rather than return an empty buffer.
235+
CompositeByteBuf compositeByteBuf = ByteBufAllocator.DEFAULT.compositeBuffer(Math.max(1, bodyParts.size()));
232236
for (HttpResponseBodyPart part : bodyParts) {
233237
compositeByteBuf.addComponent(true, part.getBodyByteBuf());
234238
}
@@ -249,15 +253,15 @@ public String getResponseBody() {
249253
* documents it as read-only and names the other holders. {@link #getResponseBodyAsBytes()} stays the
250254
* copying accessor for callers who want an array of their own.
251255
* <p>
252-
* Private, and called directly by the accessors below rather than through
256+
* Private, and called directly by the string accessors rather than through
253257
* {@link #getResponseBodyAsBytesView()}, so that overriding the view does not silently change what this
254258
* response's text says as well.
255259
*/
256260
private byte[] sharedBodyBytes() {
257261
if (bodyParts.isEmpty()) {
258-
// A HEAD, a 204 or a 304 otherwise walks the aggregating path to allocate an empty array and a
259-
// buffer to wrap it, on every call. Nothing can be written through a zero-length array, so one
260-
// shared instance serves every empty body.
262+
// A response with no body - a HEAD, a 204, a 304, a discarded CONNECT failure, one aborted from a
263+
// handler - otherwise walks the aggregating path to allocate an empty array and a buffer to wrap it,
264+
// on every call. Nothing can be written through a zero-length array, so one instance serves them all.
261265
return EMPTY_BODY;
262266
}
263267
return bodyParts.size() == 1 ? bodyParts.get(0).getBodyPartBytes() : getResponseBodyAsBytes();
Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
/*
2+
* Copyright (c) 2026 AsyncHttpClient Project. All rights reserved.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
package org.asynchttpclient;
17+
18+
import io.netty.buffer.Unpooled;
19+
import org.asynchttpclient.netty.EagerResponseBodyPart;
20+
import org.asynchttpclient.netty.NettyResponseStatus;
21+
import org.junit.jupiter.api.Test;
22+
23+
import java.nio.charset.StandardCharsets;
24+
25+
import static org.junit.jupiter.api.Assertions.assertEquals;
26+
import static org.junit.jupiter.api.Assertions.assertTrue;
27+
28+
public class ResponseBuilderTest {
29+
30+
private static void accumulateBody(Response.ResponseBuilder builder, String body) {
31+
builder.accumulate(new NettyResponseStatus(null, null, null));
32+
builder.accumulate(new EagerResponseBodyPart(Unpooled.wrappedBuffer(body.getBytes(StandardCharsets.UTF_8)), true));
33+
}
34+
35+
@Test
36+
public void testAResponseKeepsItsBodyWhenTheBuilderIsReused() {
37+
// AsyncCompletionHandler resets its builder on every onStatusReceived, so a handler instance reused for
38+
// a second request refills the same list. A response holding that list would take on the second body.
39+
Response.ResponseBuilder builder = new Response.ResponseBuilder();
40+
accumulateBody(builder, "first");
41+
Response first = builder.build();
42+
43+
builder.reset();
44+
accumulateBody(builder, "second");
45+
Response second = builder.build();
46+
47+
assertEquals("first", first.getResponseBody(StandardCharsets.UTF_8));
48+
assertEquals("second", second.getResponseBody(StandardCharsets.UTF_8));
49+
}
50+
51+
@Test
52+
public void testAResponseKeepsItsBodyWhenTheBuilderAccumulatesAgain() {
53+
// Without a reset in between, a further part must not appear in a response that was already built.
54+
Response.ResponseBuilder builder = new Response.ResponseBuilder();
55+
accumulateBody(builder, "first");
56+
Response first = builder.build();
57+
58+
builder.accumulate(new EagerResponseBodyPart(Unpooled.wrappedBuffer(" second".getBytes(StandardCharsets.UTF_8)), true));
59+
60+
assertEquals("first", first.getResponseBody(StandardCharsets.UTF_8));
61+
assertEquals("first second", builder.build().getResponseBody(StandardCharsets.UTF_8));
62+
}
63+
64+
@Test
65+
public void testAResponseKeepsItsBodyWhenTheBuilderIsResetAndNotRebuilt() {
66+
Response.ResponseBuilder builder = new Response.ResponseBuilder();
67+
accumulateBody(builder, "first");
68+
Response first = builder.build();
69+
70+
builder.reset();
71+
72+
assertTrue(first.hasResponseBody());
73+
assertEquals("first", first.getResponseBody(StandardCharsets.UTF_8));
74+
}
75+
}

‎client/src/test/java/org/asynchttpclient/netty/NettyAsyncResponseTest.java‎

Lines changed: 38 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -109,13 +109,15 @@ public void testGetResponseBodyDecodesOnePartAndSplitPartsIdentically() {
109109
// 0xC3 0xA9 encodes U+00E9; split between its two bytes so neither half decodes on its own
110110
int split = 4;
111111

112+
// Clones onto the wire, so that utf8 stays an oracle rather than becoming a part's own storage.
112113
List<HttpResponseBodyPart> onePart = new LinkedList<>();
113-
onePart.add(new EagerResponseBodyPart(Unpooled.wrappedBuffer(utf8), true));
114+
onePart.add(new EagerResponseBodyPart(Unpooled.wrappedBuffer(utf8.clone()), true));
114115
NettyResponse single = new NettyResponse(new NettyResponseStatus(null, null, null), null, onePart);
115116

117+
byte[] wire = utf8.clone();
116118
List<HttpResponseBodyPart> splitParts = new LinkedList<>();
117-
splitParts.add(new EagerResponseBodyPart(Unpooled.wrappedBuffer(utf8, 0, split), false));
118-
splitParts.add(new EagerResponseBodyPart(Unpooled.wrappedBuffer(utf8, split, utf8.length - split), true));
119+
splitParts.add(new EagerResponseBodyPart(Unpooled.wrappedBuffer(wire, 0, split), false));
120+
splitParts.add(new EagerResponseBodyPart(Unpooled.wrappedBuffer(wire, split, wire.length - split), true));
119121
NettyResponse multiple = new NettyResponse(new NettyResponseStatus(null, null, null), null, splitParts);
120122

121123
assertEquals(expected, single.getResponseBody(StandardCharsets.UTF_8));
@@ -182,7 +184,39 @@ public void testGetResponseBodyAsBytesDoesNotShareTheBodyPartArray() {
182184
public void testGetResponseBodyAsBytesViewReturnsEmptyArray() {
183185
NettyResponse response = new NettyResponse(new NettyResponseStatus(null, null, null), null, new LinkedList<>());
184186

185-
assertArrayEquals(new byte[0], response.getResponseBodyAsBytesView());
187+
byte[] view = response.getResponseBodyAsBytesView();
188+
assertArrayEquals(new byte[0], view);
189+
// One shared instance rather than an allocation per call, which is the whole point of the branch.
190+
assertSame(view, response.getResponseBodyAsBytesView());
191+
}
192+
193+
@Test
194+
public void testBodylessResponseHasAnEmptyByteBuf() {
195+
// A CompositeByteBuf rejects a maxNumComponents of 0, so a HEAD, a 204 or a 304 threw from here.
196+
NettyResponse response = new NettyResponse(new NettyResponseStatus(null, null, null), null, new LinkedList<>());
197+
198+
ByteBuf body = response.getResponseBodyAsByteBuf();
199+
try {
200+
assertEquals(0, body.readableBytes());
201+
} finally {
202+
body.release();
203+
}
204+
}
205+
206+
@Test
207+
public void testOverridingTheViewLeavesTheResponseTextAlone() {
208+
// The string accessors go through a private helper rather than the overridable view, so a subclass that
209+
// hardens the view cannot silently change what this response's text says.
210+
List<HttpResponseBodyPart> bodyParts = new LinkedList<>();
211+
bodyParts.add(new EagerResponseBodyPart(Unpooled.wrappedBuffer("Hello World".getBytes(StandardCharsets.UTF_8)), true));
212+
NettyResponse response = new NettyResponse(new NettyResponseStatus(null, null, null), null, bodyParts) {
213+
@Override
214+
public byte[] getResponseBodyAsBytesView() {
215+
return "Goodbye".getBytes(StandardCharsets.UTF_8);
216+
}
217+
};
218+
219+
assertEquals("Hello World", response.getResponseBody(StandardCharsets.UTF_8));
186220
}
187221

188222
@Test

0 commit comments

Comments
 (0)