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
17 changes: 17 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,23 @@
*/
byte[] getResponseBodyAsBytes();

/**
* Returns the entire response body as a byte array that may share its storage with this response.
*
* <p>The returned array must be treated as read-only. Modifying it may change the content subsequently returned

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"this response's other body accessors" is too narrow. The array belongs to the HttpResponseBodyPart, not to the response, and it is reachable from the handler callback, from TransferListener and from getResponseBodyAsByteBuf(). It also goes the other way: bb.setByte(10, '9') on that ByteBuf changes what the caller's read-only view says. Worth spelling out who the other holders are.

* by this response's other body accessors.
*
* <p>The implementation is not required to return shared storage. Depending on the response representation, this

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we say here that it also depends on ResponseBodyPartFactory? EAGER shares, LAZY always copies through ByteBufUtil.getBytes(buf.duplicate()), and the caller cannot see which one the client was built with. Someone who develops an in-place transform against LAZY will ship it green and corrupt bodies on the default config.

* method may still return a copy. No array identity is guaranteed between calls.
*
* <p>Use {@link #getResponseBodyAsBytes()} when an independently owned, mutable array is required.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getResponseBodyAsBytes() never promised this. Its own javadoc just says it returns the body, and it is an interface method anyone can implement, so returning a cached array is legal there. I would not promise ownership here on behalf of a method that does not guarantee it.

*
* @return the entire response body as a possibly shared byte array
*/
default byte[] getResponseBodyAsBytesView() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add an @implSpec saying the default calls getResponseBodyAsBytes(). After reading the text above, the obvious thing for an implementor is getResponseBodyAsBytes() { return getResponseBodyAsBytesView().clone(); } and override nothing else, and that recurses until it blows the stack.

return getResponseBodyAsBytes();
}

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

/**
* @param name the header name

Check warning on line 126 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 126 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 126 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 126 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 126 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 126 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.

Check warning on line 126 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 126 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 126 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.
* @return the first response header value
*/
String getHeader(CharSequence name);
Expand Down Expand Up @@ -137,7 +154,7 @@
String toString();

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

Check warning on line 157 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 157 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 157 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 157 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 157 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 157 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.

Check warning on line 157 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 157 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 157 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.
*/
List<Cookie> getCookies();

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

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

Check warning on line 226 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 226 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 226 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 226 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 226 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 226 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.

Check warning on line 226 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 226 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 226 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.
*/
public void accumulate(HttpResponseBodyPart bodyPart) {
if (bodyPart.length() > 0) {
Expand Down
17 changes: 6 additions & 11 deletions client/src/main/java/org/asynchttpclient/netty/NettyResponse.java
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,11 @@ public byte[] getResponseBodyAsBytes() {
return getResponseBodyAsByteBuffer().array();
}

@Override
public byte[] getResponseBodyAsBytesView() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This array is not only ours. EagerResponseBodyPart returns its bytes field by identity, and that is the same array we already passed to the user's AsyncHandler.onBodyPartReceived, the same one TransferCompletionHandler gives to every TransferListener, and the one ResumableAsyncHandler wraps writable in a ByteBuffer. So the caller can corrupt those, and they can corrupt the caller's view after we returned it.

return bodyParts.size() == 1 ? bodyParts.get(0).getBodyPartBytes() : getResponseBodyAsBytes();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bodyParts.size() == 1 is not a property of the response, it is the number of non-empty chunks we happened to read. The same 48 byte body handed over in one chunk gives the caller our array, in two chunks gives them a copy. Any proxy that re-chunks flips it. gzip flips it the other way, because the decompressor output does not get split at httpClientCodecMaxChunkSize, so a 20000 byte body arrives as 4 parts uncompressed and 1 part compressed.

It is not even stable for a fixed server. On a fresh connection the split is at 2048 bytes, on a warm pooled one it is around 8192, so the caller gets a copy on request 1 and our array on request 2. A test that makes one client per test with a 4 KiB fixture never sees the sharing path.

}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: an empty body still goes down the aggregating path and allocates a byte[0] plus a ByteBuffer on every call. HEAD, 204 and 304 are common enough that a static final byte[] EMPTY short circuit is worth it, and it is the one case here that really is zero allocation.


@Override
public ByteBuffer getResponseBodyAsByteBuffer() {

Expand Down Expand Up @@ -224,19 +229,9 @@ public String getResponseBody() {
return getResponseBody(withDefault(extractContentTypeCharsetAttribute(getContentType()), UTF_8));
}

/**

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment was carrying two things. The reason multi part gets concatenated, a multi byte char can straddle a part boundary, which is the only place that is written down. And the invariant from #2303, that the array does not escape the method. #2303 landed a month ago and its tests are still in this file. If we are reversing it, the PR description should say why instead of just deleting the comment.

* 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.
*/
private byte[] sharedBodyBytes() {
return bodyParts.size() == 1 ? bodyParts.get(0).getBodyPartBytes() : getResponseBodyAsBytes();
}

@Override
public String getResponseBody(Charset charset) {
return new String(sharedBodyBytes(), charset);
return new String(getResponseBodyAsBytesView(), charset);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This used to go through the private sharedBodyBytes(). Now it is a virtual call to a public method on a non-final class, so any subclass that overrides the view silently changes getResponseBody(), getResponseBody(Charset) and toString() as well, and the String and the bytes of one response can disagree. getStatusCode, getHeaders and isRedirected right next to it are final. Can we keep a private helper here and leave the public method for callers?

}

@Override
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,32 +120,108 @@ 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
public void testGetResponseBodyReadsOnlyALazyPartsReadableRegion() throws IOException {
// A Lazy part's getBodyPartBytes returns just the readable region, not the whole backing array, so a
// single-part shortcut must go through it rather than reach for getBodyByteBuf().array().
byte[] backing = "XXXHello WorldYYY".getBytes(StandardCharsets.UTF_8);
ByteBuf slice = Unpooled.wrappedBuffer(backing).slice(3, 11);
int readerIndex = slice.readerIndex();
int writerIndex = slice.writerIndex();
int refCnt = slice.refCnt();
try {
List<HttpResponseBodyPart> bodyParts = new LinkedList<>();
bodyParts.add(new LazyResponseBodyPart(slice, true));
NettyResponse response = new NettyResponse(new NettyResponseStatus(null, null, null), null, bodyParts);

assertArrayEquals("Hello World".getBytes(StandardCharsets.UTF_8), response.getResponseBodyAsBytesView());
assertEquals("Hello World", response.getResponseBody(StandardCharsets.UTF_8));
assertEquals("Hello World",
new String(response.getResponseBodyAsStream().readAllBytes(), StandardCharsets.UTF_8));
assertEquals(readerIndex, slice.readerIndex());
assertEquals(writerIndex, slice.writerIndex());
assertEquals(refCnt, slice.refCnt());
} finally {
slice.release();
}
}

@Test
public void testGetResponseBodyAsBytesViewReadsDirectLazyPart() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This state cannot happen on a real request. AsyncHttpClientHandler.channelRead releases the message in a finally as soon as onBodyPartReceived returns and LazyResponseBodyPart never retains, so by the time anyone holds a Response the buffer is at refCnt 0 and this call throws IllegalReferenceCountException. With -Dio.netty.buffer.checkAccessible=false it does not throw, it returns recycled pool memory instead. The test keeps the buffer alive by hand, which makes LAZY look supported when it is not. Not something you introduced, but adding coverage here signs off on it.

byte[] backing = "XXXHello WorldYYY".getBytes(StandardCharsets.UTF_8);
ByteBuf direct = Unpooled.directBuffer(backing.length);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

direct is allocated, written and sliced before the try, so anything throwing in there leaks a direct buffer and the leak detector fails the build. Open the try right after the allocation.

direct.writeBytes(backing);
ByteBuf slice = direct.slice(3, 11);
int readerIndex = slice.readerIndex();
int writerIndex = slice.writerIndex();
int refCnt = slice.refCnt();
try {
List<HttpResponseBodyPart> bodyParts = new LinkedList<>();
bodyParts.add(new LazyResponseBodyPart(slice, true));
NettyResponse response = new NettyResponse(new NettyResponseStatus(null, null, null), null, bodyParts);

assertArrayEquals("Hello World".getBytes(StandardCharsets.UTF_8), response.getResponseBodyAsBytesView());
assertEquals(readerIndex, slice.readerIndex());
assertEquals(writerIndex, slice.writerIndex());
assertEquals(refCnt, slice.refCnt());
} finally {
direct.release();
}
}

@Test
public void testGetResponseBodyAsBytesViewSharesOneEagerPart() {
List<HttpResponseBodyPart> bodyParts = new LinkedList<>();
bodyParts.add(new LazyResponseBodyPart(Unpooled.wrappedBuffer(backing, 3, 11), true));
bodyParts.add(new EagerResponseBodyPart(Unpooled.wrappedBuffer("Hello World".getBytes(StandardCharsets.UTF_8)), true));
NettyResponse response = new NettyResponse(new NettyResponseStatus(null, null, null), null, bodyParts);

assertEquals("Hello World", response.getResponseBody(StandardCharsets.UTF_8));
assertEquals("Hello World",
new String(response.getResponseBodyAsStream().readAllBytes(), StandardCharsets.UTF_8));
byte[] view = response.getResponseBodyAsBytesView();
assertSame(bodyParts.get(0).getBodyPartBytes(), view);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The javadoc says no array identity is guaranteed between calls and that the implementation may return a copy, and then we assert both of those here. Swap the fixture to LazyResponseBodyPart and the same assertion fails. One of the two has to give, otherwise the next person cannot tell which one is the contract.

assertSame(view, response.getResponseBodyAsBytesView());
}

@Test
public void testGetResponseBodyAsBytesDoesNotShareTheBodyPartArray() {
byte[] expected = "Hello World".getBytes(StandardCharsets.UTF_8);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expected is both the array we wrap into the part and the oracle we compare against. It holds today only because EagerResponseBodyPart copies. Add the obvious hasArray() fast path there and expected becomes the part's live storage, so firstCopy[0] = 'X' mutates it too and both assertions pass on a corrupt response. Unpooled.wrappedBuffer(expected.clone()) keeps it honest.

List<HttpResponseBodyPart> bodyParts = new LinkedList<>();
bodyParts.add(new EagerResponseBodyPart(Unpooled.wrappedBuffer("Hello World".getBytes(StandardCharsets.UTF_8)), true));
bodyParts.add(new EagerResponseBodyPart(Unpooled.wrappedBuffer(expected), 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();
assertNotSame(firstCopy, secondCopy);
assertNotSame(bodyParts.get(0).getBodyPartBytes(), firstCopy);

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