Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions client/src/main/java/org/asynchttpclient/Response.java
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,39 @@
*/
byte[] getResponseBodyAsBytes();

/**
* Returns the entire response body as a byte array whose storage the implementation may share with whatever
* else holds the body.
*
* <p>The returned array must be treated as read-only, and the response is not its only holder. Where it is a
* body part's own array, it is the array reachable from the part handed to
* {@link AsyncHandler#onBodyPartReceived}, the one each
* {@link org.asynchttpclient.handler.TransferListener} is given by a
* {@link org.asynchttpclient.handler.TransferCompletionHandler}, and the one
* {@link #getResponseBodyAsByteBuf()} wraps. Writing to it changes what all of those see, and a write
* through any of them changes what this returns.
*
* <p>Whether anything is shared at all is not something to rely on. It depends on how the body happened to
* arrive - how the origin chunked it, whether a proxy re-chunked it, whether it was compressed - and on the
* body parts the implementation was given, none of which is visible from here. The same body from the
* same server may be shared on one response and copied on the next. No array identity is guaranteed between
* calls either.
*
* <p>A caller that needs an array it may modify should copy what it receives. {@link
* #getResponseBodyAsBytes()} is the accessor to reach for first, but it is implemented by whoever implements
* this interface, so read its contract rather than assuming it hands over an array of its own.
*
* <p>Implementation note: the default implementation of this method returns
* {@link #getResponseBodyAsBytes()}. An implementation that leaves that default in place must not implement
* {@code getResponseBodyAsBytes()} in terms of this method, or the two call each other. Overriding both is
* fine.
*
* @return the entire response body, possibly sharing storage with the response
*/
default byte[] getResponseBodyAsBytesView() {
Comment thread
pavel-ptashyts marked this conversation as resolved.
return getResponseBodyAsBytes();
}

/**
* Return the entire response body as a ByteBuffer.
*
Expand Down Expand Up @@ -106,7 +139,7 @@
String getContentType();

/**
* @param name the header name

Check warning on line 142 in client/src/main/java/org/asynchttpclient/Response.java

View workflow job for this annotation

GitHub Actions / compile-and-check

[MissingSummary] A summary fragment is required; consider using the value of the @return block as a summary fragment instead.

Check warning on line 142 in client/src/main/java/org/asynchttpclient/Response.java

View workflow job for this annotation

GitHub Actions / test (ubuntu-latest, 25)

[MissingSummary] A summary fragment is required; consider using the value of the @return block as a summary fragment instead.

Check warning on line 142 in client/src/main/java/org/asynchttpclient/Response.java

View workflow job for this annotation

GitHub Actions / test (ubuntu-latest, 21)

[MissingSummary] A summary fragment is required; consider using the value of the @return block as a summary fragment instead.

Check warning on line 142 in client/src/main/java/org/asynchttpclient/Response.java

View workflow job for this annotation

GitHub Actions / test (ubuntu-latest, 17)

[MissingSummary] A summary fragment is required; consider using the value of the @return block as a summary fragment instead.

Check warning on line 142 in client/src/main/java/org/asynchttpclient/Response.java

View workflow job for this annotation

GitHub Actions / test (ubuntu-latest, 11)

[MissingSummary] A summary fragment is required; consider using the value of the @return block as a summary fragment instead.

Check warning on line 142 in client/src/main/java/org/asynchttpclient/Response.java

View workflow job for this annotation

GitHub Actions / test (macos-latest, 25)

[MissingSummary] A summary fragment is required; consider using the value of the @return block as a summary fragment instead.

Check warning on line 142 in client/src/main/java/org/asynchttpclient/Response.java

View workflow job for this annotation

GitHub Actions / test (macos-latest, 11)

[MissingSummary] A summary fragment is required; consider using the value of the @return block as a summary fragment instead.

Check warning on line 142 in client/src/main/java/org/asynchttpclient/Response.java

View workflow job for this annotation

GitHub Actions / test (macos-latest, 17)

[MissingSummary] A summary fragment is required; consider using the value of the @return block as a summary fragment instead.

Check warning on line 142 in client/src/main/java/org/asynchttpclient/Response.java

View workflow job for this annotation

GitHub Actions / test (macos-latest, 21)

[MissingSummary] A summary fragment is required; consider using the value of the @return block as a summary fragment instead.
* @return the first response header value
*/
String getHeader(CharSequence name);
Expand Down Expand Up @@ -137,7 +170,7 @@
String toString();

/**
* @return the list of {@link Cookie}.

Check warning on line 173 in client/src/main/java/org/asynchttpclient/Response.java

View workflow job for this annotation

GitHub Actions / compile-and-check

[MissingSummary] A summary fragment is required; consider using the value of the @return block as a summary fragment instead.

Check warning on line 173 in client/src/main/java/org/asynchttpclient/Response.java

View workflow job for this annotation

GitHub Actions / test (ubuntu-latest, 25)

[MissingSummary] A summary fragment is required; consider using the value of the @return block as a summary fragment instead.

Check warning on line 173 in client/src/main/java/org/asynchttpclient/Response.java

View workflow job for this annotation

GitHub Actions / test (ubuntu-latest, 21)

[MissingSummary] A summary fragment is required; consider using the value of the @return block as a summary fragment instead.

Check warning on line 173 in client/src/main/java/org/asynchttpclient/Response.java

View workflow job for this annotation

GitHub Actions / test (ubuntu-latest, 17)

[MissingSummary] A summary fragment is required; consider using the value of the @return block as a summary fragment instead.

Check warning on line 173 in client/src/main/java/org/asynchttpclient/Response.java

View workflow job for this annotation

GitHub Actions / test (ubuntu-latest, 11)

[MissingSummary] A summary fragment is required; consider using the value of the @return block as a summary fragment instead.

Check warning on line 173 in client/src/main/java/org/asynchttpclient/Response.java

View workflow job for this annotation

GitHub Actions / test (macos-latest, 25)

[MissingSummary] A summary fragment is required; consider using the value of the @return block as a summary fragment instead.

Check warning on line 173 in client/src/main/java/org/asynchttpclient/Response.java

View workflow job for this annotation

GitHub Actions / test (macos-latest, 11)

[MissingSummary] A summary fragment is required; consider using the value of the @return block as a summary fragment instead.

Check warning on line 173 in client/src/main/java/org/asynchttpclient/Response.java

View workflow job for this annotation

GitHub Actions / test (macos-latest, 17)

[MissingSummary] A summary fragment is required; consider using the value of the @return block as a summary fragment instead.

Check warning on line 173 in client/src/main/java/org/asynchttpclient/Response.java

View workflow job for this annotation

GitHub Actions / test (macos-latest, 21)

[MissingSummary] A summary fragment is required; consider using the value of the @return block as a summary fragment instead.
*/
List<Cookie> getCookies();

Expand Down Expand Up @@ -206,7 +239,7 @@
}

/**
* @param bodyPart a body part (possibly empty, but will be filtered out)

Check warning on line 242 in client/src/main/java/org/asynchttpclient/Response.java

View workflow job for this annotation

GitHub Actions / compile-and-check

[MissingSummary] A summary line is required on public/protected Javadocs.

Check warning on line 242 in client/src/main/java/org/asynchttpclient/Response.java

View workflow job for this annotation

GitHub Actions / test (ubuntu-latest, 25)

[MissingSummary] A summary line is required on public/protected Javadocs.

Check warning on line 242 in client/src/main/java/org/asynchttpclient/Response.java

View workflow job for this annotation

GitHub Actions / test (ubuntu-latest, 21)

[MissingSummary] A summary line is required on public/protected Javadocs.

Check warning on line 242 in client/src/main/java/org/asynchttpclient/Response.java

View workflow job for this annotation

GitHub Actions / test (ubuntu-latest, 17)

[MissingSummary] A summary line is required on public/protected Javadocs.

Check warning on line 242 in client/src/main/java/org/asynchttpclient/Response.java

View workflow job for this annotation

GitHub Actions / test (ubuntu-latest, 11)

[MissingSummary] A summary line is required on public/protected Javadocs.

Check warning on line 242 in client/src/main/java/org/asynchttpclient/Response.java

View workflow job for this annotation

GitHub Actions / test (macos-latest, 25)

[MissingSummary] A summary line is required on public/protected Javadocs.

Check warning on line 242 in client/src/main/java/org/asynchttpclient/Response.java

View workflow job for this annotation

GitHub Actions / test (macos-latest, 11)

[MissingSummary] A summary line is required on public/protected Javadocs.

Check warning on line 242 in client/src/main/java/org/asynchttpclient/Response.java

View workflow job for this annotation

GitHub Actions / test (macos-latest, 17)

[MissingSummary] A summary line is required on public/protected Javadocs.

Check warning on line 242 in client/src/main/java/org/asynchttpclient/Response.java

View workflow job for this annotation

GitHub Actions / test (macos-latest, 21)

[MissingSummary] A summary line is required on public/protected Javadocs.
*/
public void accumulate(HttpResponseBodyPart bodyPart) {
if (bodyPart.length() > 0) {
Expand Down
37 changes: 33 additions & 4 deletions client/src/main/java/org/asynchttpclient/netty/NettyResponse.java
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@
*/
public class NettyResponse implements Response {

private static final byte[] EMPTY_BODY = new byte[0];

private final List<HttpResponseBodyPart> bodyParts;
private final HttpHeaders headers;
private final HttpResponseStatus status;
Expand Down Expand Up @@ -193,6 +195,20 @@ public byte[] getResponseBodyAsBytes() {
return getResponseBodyAsByteBuffer().array();
}

/**
* Returns a lone body part's array; concatenates into one of its own when there are several, or an empty
* array when there are none. Which of those a given response takes is not a property of the body: see
* {@link Response#getResponseBodyAsBytesView()}, whose contract is deliberately weaker than this.
* <p>
* Whether a lone part hands over storage of its own is the part's business rather than this response's.
* {@link EagerResponseBodyPart} returns the array it holds; {@link LazyResponseBodyPart} copies out of its
* buffer on every call, so a response made of lazy parts never shares whatever this says.
*/
@Override
public byte[] getResponseBodyAsBytesView() {
Comment thread
pavel-ptashyts marked this conversation as resolved.
return sharedBodyBytes();
}
Comment thread
pavel-ptashyts marked this conversation as resolved.

@Override
public ByteBuffer getResponseBodyAsByteBuffer() {

Expand Down Expand Up @@ -225,12 +241,25 @@ public String getResponseBody() {
}

/**
Comment thread
pavel-ptashyts marked this conversation as resolved.
* The body as bytes, for callers that keep the array to themselves. A lone part's own array is returned
* rather than a copy of it, so a caller that let it out would let the part's buffer be mutated through it;
* {@link #getResponseBodyAsBytes()} is the copying variant for those. Several parts are concatenated
* because a multi-byte character can straddle a part boundary.
* The body as bytes, without a copy where there is one part to take it from. Several parts are concatenated
* because a multi-byte character can straddle a part boundary, which is why the string accessors cannot
* simply decode the first part.
* <p>
* The array does leave the client, through {@link #getResponseBodyAsBytesView()}, which is why that method
* documents it as read-only and names the other holders. {@link #getResponseBodyAsBytes()} stays the
* copying accessor for callers who want an array of their own.
* <p>
* Private, and called directly by the accessors below rather than through
* {@link #getResponseBodyAsBytesView()}, so that overriding the view does not silently change what this
* response's text says as well.
*/
private byte[] sharedBodyBytes() {
if (bodyParts.isEmpty()) {
// A HEAD, a 204 or a 304 otherwise walks the aggregating path to allocate an empty array and a
// buffer to wrap it, on every call. Nothing can be written through a zero-length array, so one
// shared instance serves every empty body.
return EMPTY_BODY;
}
return bodyParts.size() == 1 ? bodyParts.get(0).getBodyPartBytes() : getResponseBodyAsBytes();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,13 @@
import io.netty.handler.codec.http.HttpHeaders;
import io.netty.handler.codec.http.cookie.Cookie;
import org.asynchttpclient.HttpResponseBodyPart;
import org.asynchttpclient.Response;
import org.junit.jupiter.api.Test;

import java.io.IOException;
import java.io.OutputStream;
import java.lang.invoke.MethodHandles;
import java.lang.invoke.MethodType;
import java.nio.charset.StandardCharsets;
import java.text.SimpleDateFormat;
import java.util.Date;
Expand All @@ -32,9 +35,13 @@
import java.util.TimeZone;

import static io.netty.handler.codec.http.HttpHeaderNames.SET_COOKIE;
import static org.junit.jupiter.api.Assertions.assertArrayEquals;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotSame;
import static org.junit.jupiter.api.Assertions.assertSame;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

public class NettyAsyncResponseTest {

Expand Down Expand Up @@ -113,6 +120,8 @@ public void testGetResponseBodyDecodesOnePartAndSplitPartsIdentically() {

assertEquals(expected, single.getResponseBody(StandardCharsets.UTF_8));
assertEquals(expected, multiple.getResponseBody(StandardCharsets.UTF_8));
assertArrayEquals(utf8, single.getResponseBodyAsBytesView());
assertArrayEquals(utf8, multiple.getResponseBodyAsBytesView());
}

@Test
Expand All @@ -130,15 +139,64 @@ public void testGetResponseBodyReadsOnlyALazyPartsReadableRegion() throws IOExce
}

@Test
public void testGetResponseBodyAsBytesDoesNotShareTheBodyPartArray() {
public void testGetResponseBodyAsBytesViewSharesOneEagerPart() {
// NettyResponse's own behaviour, not the interface contract: Response#getResponseBodyAsBytesView
// guarantees no identity, deliberately, because whether a body arrives as one part is not up to it.
// What is worth pinning here is that when this response can share, it does, and does not copy instead.
List<HttpResponseBodyPart> bodyParts = new LinkedList<>();
bodyParts.add(new EagerResponseBodyPart(Unpooled.wrappedBuffer("Hello World".getBytes(StandardCharsets.UTF_8)), true));
NettyResponse response = new NettyResponse(new NettyResponseStatus(null, null, null), null, bodyParts);

byte[] view = response.getResponseBodyAsBytesView();
assertArrayEquals("Hello World".getBytes(StandardCharsets.UTF_8), view);
assertSame(bodyParts.get(0).getBodyPartBytes(), view);
Comment thread
pavel-ptashyts marked this conversation as resolved.
assertSame(view, response.getResponseBodyAsBytesView());
}

@Test
public void testGetResponseBodyAsBytesDoesNotShareTheBodyPartArray() {
byte[] expected = "Hello World".getBytes(StandardCharsets.UTF_8);
Comment thread
pavel-ptashyts marked this conversation as resolved.
List<HttpResponseBodyPart> bodyParts = new LinkedList<>();
// A clone into the part, so that expected stays an oracle: handing the part this very array would make
// it the part's own storage the moment EagerResponseBodyPart stopped copying, and a corrupt response
// would then satisfy both assertions below.
bodyParts.add(new EagerResponseBodyPart(Unpooled.wrappedBuffer(expected.clone()), true));
NettyResponse response = new NettyResponse(new NettyResponseStatus(null, null, null), null, bodyParts);

// getResponseBody may decode a lone part in place, but getResponseBodyAsBytes hands the array to the
// caller, so it must keep copying rather than expose the part's own array.
assertNotSame(response.getResponseBodyAsBytes(), response.getResponseBodyAsBytes());
assertNotSame(bodyParts.get(0).getBodyPartBytes(), response.getResponseBodyAsBytes());
byte[] firstCopy = response.getResponseBodyAsBytes();
byte[] secondCopy = response.getResponseBodyAsBytes();
assertArrayEquals(expected, firstCopy);
assertArrayEquals(expected, secondCopy);
assertNotSame(firstCopy, secondCopy);
assertNotSame(bodyParts.get(0).getBodyPartBytes(), firstCopy);
assertNotSame(bodyParts.get(0).getBodyPartBytes(), secondCopy);

firstCopy[0] = 'X';
assertArrayEquals(expected, response.getResponseBodyAsBytes());
assertArrayEquals(expected, response.getResponseBodyAsBytesView());
}

@Test
public void testGetResponseBodyAsBytesViewReturnsEmptyArray() {
NettyResponse response = new NettyResponse(new NettyResponseStatus(null, null, null), null, new LinkedList<>());

assertArrayEquals(new byte[0], response.getResponseBodyAsBytesView());
}

@Test
public void testGetResponseBodyAsBytesViewDefaultImplementationDelegates() throws Throwable {
byte[] expected = "Hello World".getBytes(StandardCharsets.UTF_8);
Response response = mock(Response.class);
when(response.getResponseBodyAsBytes()).thenReturn(expected);

byte[] actual = (byte[]) MethodHandles.privateLookupIn(Response.class, MethodHandles.lookup())
.findSpecial(Response.class, "getResponseBodyAsBytesView", MethodType.methodType(byte[].class), Response.class)
.bindTo(response)
.invokeExact();

assertSame(expected, actual);
}

@Test
Expand Down
Loading