-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Reuse byte buffers in BufferingResponseListener #12691
base: jetty-12.0.x
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 | ||||
---|---|---|---|---|---|---|
|
@@ -24,7 +24,10 @@ | |||||
import org.eclipse.jetty.http.HttpFields; | ||||||
import org.eclipse.jetty.http.HttpHeader; | ||||||
import org.eclipse.jetty.http.HttpMethod; | ||||||
import org.eclipse.jetty.util.BufferUtil; | ||||||
import org.eclipse.jetty.io.ByteBufferAccumulator; | ||||||
import org.eclipse.jetty.io.ByteBufferPool; | ||||||
|
||||||
import static java.util.Objects.requireNonNull; | ||||||
|
||||||
/** | ||||||
* <p>Implementation of {@link Listener} that buffers the content up to a maximum length | ||||||
|
@@ -36,7 +39,8 @@ | |||||
public abstract class BufferingResponseListener implements Listener | ||||||
{ | ||||||
private final int maxLength; | ||||||
private ByteBuffer buffer; | ||||||
private final ByteBufferAccumulator buffer; | ||||||
private byte[] result; | ||||||
private String mediaType; | ||||||
private String encoding; | ||||||
|
||||||
|
@@ -55,6 +59,18 @@ public BufferingResponseListener() | |||||
*/ | ||||||
public BufferingResponseListener(int maxLength) | ||||||
{ | ||||||
this(ByteBufferPool.NON_POOLING, maxLength); | ||||||
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.
Suggested change
See below |
||||||
} | ||||||
|
||||||
/** | ||||||
* Creates an instance with the given and byte buffer pool and maximum length. | ||||||
* | ||||||
* @param maxLength the maximum length of the content | ||||||
* @param byteBufferPool the byte buffer pool to use | ||||||
*/ | ||||||
public BufferingResponseListener(ByteBufferPool byteBufferPool, int maxLength) | ||||||
{ | ||||||
this.buffer = new ByteBufferAccumulator(requireNonNull(byteBufferPool, "byteBufferPool is null"), true); | ||||||
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.
Suggested change
|
||||||
if (maxLength < 0) | ||||||
throw new IllegalArgumentException("Invalid max length " + maxLength); | ||||||
this.maxLength = maxLength; | ||||||
|
@@ -109,16 +125,7 @@ public void onContent(Response response, ByteBuffer content) | |||||
int length = content.remaining(); | ||||||
if (length == 0) | ||||||
return; | ||||||
if (length > BufferUtil.space(buffer)) | ||||||
{ | ||||||
int remaining = buffer == null ? 0 : buffer.remaining(); | ||||||
if (remaining + length > maxLength) | ||||||
response.abort(new IllegalArgumentException("Buffering capacity " + maxLength + " exceeded")); | ||||||
int requiredCapacity = buffer == null ? length : buffer.capacity() + length; | ||||||
int newCapacity = Math.min(Integer.highestOneBit(requiredCapacity) << 1, maxLength); | ||||||
buffer = BufferUtil.ensureCapacity(buffer, newCapacity); | ||||||
} | ||||||
BufferUtil.append(buffer, content); | ||||||
buffer.copyBuffer(content); | ||||||
} | ||||||
|
||||||
@Override | ||||||
|
@@ -140,9 +147,7 @@ public String getEncoding() | |||||
*/ | ||||||
public byte[] getContent() | ||||||
{ | ||||||
if (buffer == null) | ||||||
return new byte[0]; | ||||||
return BufferUtil.toArray(buffer); | ||||||
return toByteArray(); | ||||||
} | ||||||
|
||||||
/** | ||||||
|
@@ -165,9 +170,7 @@ public String getContentAsString() | |||||
*/ | ||||||
public String getContentAsString(String encoding) | ||||||
{ | ||||||
if (buffer == null) | ||||||
return null; | ||||||
return BufferUtil.toString(buffer, Charset.forName(encoding)); | ||||||
return getContentAsString(Charset.forName(encoding)); | ||||||
} | ||||||
|
||||||
/** | ||||||
|
@@ -177,18 +180,31 @@ public String getContentAsString(String encoding) | |||||
*/ | ||||||
public String getContentAsString(Charset encoding) | ||||||
{ | ||||||
if (buffer == null) | ||||||
if (buffer.getLength() == 0) | ||||||
return null; | ||||||
return BufferUtil.toString(buffer, encoding); | ||||||
return new String(toByteArray(), encoding); | ||||||
} | ||||||
|
||||||
/** | ||||||
* @return Content as InputStream | ||||||
*/ | ||||||
public InputStream getContentAsInputStream() | ||||||
{ | ||||||
if (buffer == null) | ||||||
return new ByteArrayInputStream(new byte[0]); | ||||||
return new ByteArrayInputStream(buffer.array(), buffer.arrayOffset(), buffer.remaining()); | ||||||
return new ByteArrayInputStream(toByteArray()); | ||||||
} | ||||||
|
||||||
private byte[] toByteArray() | ||||||
{ | ||||||
synchronized (buffer) | ||||||
{ | ||||||
try (ByteBufferAccumulator buffer = this.buffer) | ||||||
{ | ||||||
if (buffer.getLength() == 0) | ||||||
result = new byte[0]; | ||||||
else | ||||||
result = buffer.toByteArray(); | ||||||
} | ||||||
} | ||||||
return result; | ||||||
Comment on lines
+193
to
+208
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. ideally we would not have to copy all the retained buffers to a single buffer just to stream as a InputStream. We should have a mechanism to do this without this extra copy. |
||||||
} | ||||||
} |
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 class is deprecated in 12.0. In 12.1 it should probably be a
org.eclipse.jetty.io.RetainableByteBuffer.DynamicCapacity
Perhaps it is best to rebase this PR to 12.1 and use that class. 12.1 will soon be the main branch.