-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Provide an opt-in zero-copy response body view #2322
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
| * by this response's other body accessors. | ||
| * | ||
| * <p>The implementation is not required to return shared storage. Depending on the response representation, this | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we say here that it also depends on |
||
| * 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. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| * | ||
| * @return the entire response body as a possibly shared byte array | ||
| */ | ||
| default byte[] getResponseBodyAsBytesView() { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add an |
||
| return getResponseBodyAsBytes(); | ||
| } | ||
|
|
||
| /** | ||
| * Return the entire response body as a ByteBuffer. | ||
| * | ||
|
|
@@ -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
|
||
| * @return the first response header value | ||
| */ | ||
| String getHeader(CharSequence name); | ||
|
|
@@ -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
|
||
| */ | ||
| List<Cookie> getCookies(); | ||
|
|
||
|
|
@@ -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
|
||
| */ | ||
| public void accumulate(HttpResponseBodyPart bodyPart) { | ||
| if (bodyPart.length() > 0) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -193,6 +193,11 @@ public byte[] getResponseBodyAsBytes() { | |
| return getResponseBodyAsByteBuffer().array(); | ||
| } | ||
|
|
||
| @Override | ||
| public byte[] getResponseBodyAsBytesView() { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This array is not only ours. |
||
| return bodyParts.size() == 1 ? bodyParts.get(0).getBodyPartBytes() : getResponseBodyAsBytes(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. |
||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
|
||
| @Override | ||
| public ByteBuffer getResponseBodyAsByteBuffer() { | ||
|
|
||
|
|
@@ -224,19 +229,9 @@ public String getResponseBody() { | |
| return getResponseBody(withDefault(extractContentTypeCharsetAttribute(getContentType()), UTF_8)); | ||
| } | ||
|
|
||
| /** | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This used to go through the private |
||
| } | ||
|
|
||
| @Override | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
|
@@ -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 { | ||
|
|
||
|
|
@@ -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() { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This state cannot happen on a real request. |
||
| byte[] backing = "XXXHello WorldYYY".getBytes(StandardCharsets.UTF_8); | ||
| ByteBuf direct = Unpooled.directBuffer(backing.length); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| 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); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| assertSame(view, response.getResponseBodyAsBytesView()); | ||
| } | ||
|
|
||
| @Test | ||
| public void testGetResponseBodyAsBytesDoesNotShareTheBodyPartArray() { | ||
| byte[] expected = "Hello World".getBytes(StandardCharsets.UTF_8); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| 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 | ||
|
|
||
There was a problem hiding this comment.
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, fromTransferListenerand fromgetResponseBodyAsByteBuf(). 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.