Issue #5048 - Review temporary buffer usage #5091
Conversation
- avoid readOnly buffers if possible for access to the array - other cleanups to related code Signed-off-by: Lachlan Roberts <[email protected]>
…plementations Signed-off-by: Lachlan Roberts <[email protected]>
| return equalsIgnoreCase(_encoding, ccf._encoding) && equalsIgnoreCase(_extension, ccf._extension); | ||
| } | ||
|
|
||
| private static boolean equalsIgnoreCase(String a, String b) |
There was a problem hiding this comment.
Isn't there a StringUtil.equalsIgnoreCase method? If not, this method should go there.
| @@ -156,7 +151,26 @@ public ByteBuffer getIndirectBuffer() | |||
| return null; | |||
| try | |||
| { | |||
| return BufferUtil.toBuffer(_resource, false); | |||
| ByteBuffer byteBuffer = ByteBufferPoolUtil.resourceToBuffer(_resource, false, _bufferPool); | |||
| _allocatedBuffers.add(byteBuffer); | |||
There was a problem hiding this comment.
What is the lifecycle of a ResourceHttpContent? Can it be put into a cache? If so, could getIndirectBuffer be called again and again, and the list of allocated buffers get longer and longer with release never called or called in the distant future? Is release always called?
I'd be perhaps more included to have a ByteBuffer getIndirectBuffer(ByteBufferPool pool) method that the caller can pass in their pool and then be responsible for doing the release themselves.
| { | ||
| try (InputStream is = resource.getInputStream()) | ||
| { | ||
| BufferUtil.readFrom(is, ilen, buffer); |
There was a problem hiding this comment.
It is the temporary buffers like the one used inside readFrom that this issue was initially raised about.
There was a problem hiding this comment.
Also I think we can use our context to use bigger temp buffers inside readFrom
| @@ -674,10 +674,10 @@ protected boolean sendData(HttpServletRequest request, | |||
| content.getResource().writeTo(out, 0, content_length); | |||
| } | |||
| // else if we can't do a bypass write because of wrapping | |||
| else if (written || !(out instanceof HttpOutput)) | |||
| else if (written) | |||
There was a problem hiding this comment.
why was the instanceof test removed? The response object may be wrapped!
Signed-off-by: Lachlan Roberts <[email protected]>
| @@ -237,7 +255,7 @@ private HttpContent load(String pathInContext, Resource resource, int maxBufferS | |||
| { | |||
| String compressedPathInContext = pathInContext + format._extension; | |||
| CachedHttpContent compressedContent = _cache.get(compressedPathInContext); | |||
| if (compressedContent == null || compressedContent.isValid()) | |||
| if (compressedContent == null || !compressedContent.isValid()) | |||
There was a problem hiding this comment.
We only want to get the resource from the factory again if it is null or not valid.
The code doesn't make much sense the way it was, I think it was a mistake and someone forgot the !.
Implementations of
HttpContentnow use the serversByteBufferPoolto allocate temporary buffers for resources.Any allocated buffers are released back to the
ByteBufferPoolwhen theHttpContentrelease()method is called.Avoid using direct buffers before calling
BufferUtil.writeTo().