Skip to content
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

Issue #5048 - Review temporary buffer usage #5091

Closed
Closed
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
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@

package org.eclipse.jetty.http;

import org.eclipse.jetty.util.StringUtil;

public class CompressedContentFormat
{
public static final CompressedContentFormat GZIP = new CompressedContentFormat("gzip", ".gz");
Expand All @@ -44,13 +46,9 @@ public boolean equals(Object o)
{
if (!(o instanceof CompressedContentFormat))
return false;
CompressedContentFormat ccf = (CompressedContentFormat)o;
if (_encoding == null && ccf._encoding != null)
return false;
if (_extension == null && ccf._extension != null)
return false;

return _encoding.equalsIgnoreCase(ccf._encoding) && _extension.equalsIgnoreCase(ccf._extension);
CompressedContentFormat ccf = (CompressedContentFormat)o;
return StringUtil.equalsIgnoreCase(_encoding, ccf._encoding) && StringUtil.equalsIgnoreCase(_extension, ccf._extension);
}

public static boolean tagEquals(String etag, String tag)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,14 @@
import java.io.InputStream;
import java.nio.ByteBuffer;
import java.nio.channels.ReadableByteChannel;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

import org.eclipse.jetty.http.MimeTypes.Type;
import org.eclipse.jetty.util.BufferUtil;
import org.eclipse.jetty.io.ByteBufferPool;
import org.eclipse.jetty.io.ByteBufferPoolUtil;
import org.eclipse.jetty.util.resource.Resource;

/**
Expand All @@ -40,24 +43,31 @@ public class ResourceHttpContent implements HttpContent
final Resource _resource;
final String _contentType;
final int _maxBuffer;
Map<CompressedContentFormat, HttpContent> _precompressedContents;
String _etag;
final ByteBufferPool _bufferPool;
final Map<CompressedContentFormat, HttpContent> _precompressedContents;
final List<ByteBuffer> _allocatedBuffers = new ArrayList<>();

public ResourceHttpContent(final Resource resource, final String contentType)
{
this(resource, contentType, -1, null);
this(resource, contentType, -1, null, null);
}

public ResourceHttpContent(final Resource resource, final String contentType, int maxBuffer)
{
this(resource, contentType, maxBuffer, null);
this(resource, contentType, maxBuffer, null, null);
}

public ResourceHttpContent(final Resource resource, final String contentType, int maxBuffer, Map<CompressedContentFormat, HttpContent> precompressedContents)
{
this(resource, contentType, maxBuffer, null, null);
}

public ResourceHttpContent(final Resource resource, final String contentType, int maxBuffer, Map<CompressedContentFormat, HttpContent> precompressedContents, ByteBufferPool bufferPool)
{
_resource = resource;
_contentType = contentType;
_maxBuffer = maxBuffer;
_bufferPool = bufferPool;
if (precompressedContents == null)
{
_precompressedContents = null;
Expand Down Expand Up @@ -122,21 +132,6 @@ public String getLastModifiedValue()
return lm >= 0 ? DateGenerator.formatDate(lm) : null;
}

@Override
public ByteBuffer getDirectBuffer()
{
if (_resource.length() <= 0 || _maxBuffer > 0 && _resource.length() > _maxBuffer)
return null;
try
{
return BufferUtil.toBuffer(_resource, true);
}
catch (IOException e)
{
throw new RuntimeException(e);
}
}

@Override
public HttpField getETag()
{
Expand All @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

return byteBuffer;
}
catch (IOException e)
{
throw new RuntimeException(e);
}
}

@Override
public ByteBuffer getDirectBuffer()
{
if (_resource.length() <= 0 || _maxBuffer > 0 && _resource.length() > _maxBuffer)
return null;
try
{
ByteBuffer byteBuffer = ByteBufferPoolUtil.resourceToBuffer(_resource, true, _bufferPool);
_allocatedBuffers.add(byteBuffer);
return byteBuffer;
}
catch (IOException e)
{
Expand Down Expand Up @@ -198,6 +212,15 @@ public Resource getResource()
@Override
public void release()
{
if (_bufferPool != null)
{
for (ByteBuffer byteBuffer : _allocatedBuffers)
{
_bufferPool.release(byteBuffer);
}
}

_allocatedBuffers.clear();
_resource.close();
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
//
// ========================================================================
// Copyright (c) 1995-2020 Mort Bay Consulting Pty Ltd and others.
// ------------------------------------------------------------------------
// All rights reserved. This program and the accompanying materials
// are made available under the terms of the Eclipse Public License v1.0
// and Apache License v2.0 which accompanies this distribution.
//
// The Eclipse Public License is available at
// http://www.eclipse.org/legal/epl-v10.html
//
// The Apache License v2.0 is available at
// http://www.opensource.org/licenses/apache2.0.php
//
// You may elect to redistribute this code under either of these licenses.
// ========================================================================
//

package org.eclipse.jetty.io;

import java.io.IOException;
import java.io.InputStream;
import java.nio.ByteBuffer;

import org.eclipse.jetty.util.BufferUtil;
import org.eclipse.jetty.util.resource.Resource;

public class ByteBufferPoolUtil
{
public static ByteBuffer resourceToBuffer(Resource resource, boolean direct, ByteBufferPool bufferPool) throws IOException
{
long len = resource.length();
if (len < 0)
throw new IllegalArgumentException("invalid resource: " + resource + " len=" + len);

if (len > Integer.MAX_VALUE)
{
// This method cannot handle resources of this size.
return null;
}

int ilen = (int)len;
ByteBuffer buffer;
if (bufferPool != null)
buffer = bufferPool.acquire(ilen, direct);
else
buffer = direct ? BufferUtil.allocateDirect(ilen) : BufferUtil.allocate(ilen);

int pos = BufferUtil.flipToFill(buffer);
if (resource.getFile() != null)
BufferUtil.readFrom(resource.getFile(), buffer);
else
{
try (InputStream is = resource.getInputStream())
{
BufferUtil.readFrom(is, ilen, buffer);
Copy link
Contributor

Choose a reason for hiding this comment

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

It is the temporary buffers like the one used inside readFrom that this issue was initially raised about.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I think we can use our context to use bigger temp buffers inside readFrom

}
}
BufferUtil.flipToFlush(buffer, pos);

return buffer;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@
import org.eclipse.jetty.http.PreEncodedHttpField;
import org.eclipse.jetty.http.PrecompressedHttpContent;
import org.eclipse.jetty.http.ResourceHttpContent;
import org.eclipse.jetty.io.ByteBufferPool;
import org.eclipse.jetty.io.ByteBufferPoolUtil;
import org.eclipse.jetty.util.BufferUtil;
import org.eclipse.jetty.util.log.Log;
import org.eclipse.jetty.util.log.Logger;
Expand All @@ -63,6 +65,7 @@ public class CachedContentFactory implements HttpContent.ContentFactory
private final boolean _etags;
private final CompressedContentFormat[] _precompressedFormats;
private final boolean _useFileMappedBuffer;
private final ByteBufferPool _bufferPool;

private int _maxCachedFileSize = 128 * 1024 * 1024;
private int _maxCachedFiles = 2048;
Expand All @@ -79,6 +82,22 @@ public class CachedContentFactory implements HttpContent.ContentFactory
* @param precompressedFormats array of precompression formats to support
*/
public CachedContentFactory(CachedContentFactory parent, ResourceFactory factory, MimeTypes mimeTypes, boolean useFileMappedBuffer, boolean etags, CompressedContentFormat[] precompressedFormats)
{
this(parent, factory, mimeTypes, useFileMappedBuffer, etags, precompressedFormats, null);
}

/**
* Constructor.
*
* @param parent the parent resource cache
* @param factory the resource factory
* @param mimeTypes Mimetype to use for meta data
* @param useFileMappedBuffer true to file memory mapped buffers
* @param etags true to support etags
* @param precompressedFormats array of precompression formats to support
* @param bufferPool the ByteBufferPool to use
*/
public CachedContentFactory(CachedContentFactory parent, ResourceFactory factory, MimeTypes mimeTypes, boolean useFileMappedBuffer, boolean etags, CompressedContentFormat[] precompressedFormats, ByteBufferPool bufferPool)
{
_factory = factory;
_cache = new ConcurrentHashMap<>();
Expand All @@ -89,6 +108,7 @@ public CachedContentFactory(CachedContentFactory parent, ResourceFactory factory
_useFileMappedBuffer = useFileMappedBuffer;
_etags = etags;
_precompressedFormats = precompressedFormats;
_bufferPool = bufferPool;
}

public int getCachedSize()
Expand Down Expand Up @@ -193,9 +213,7 @@ public HttpContent getContent(String pathInContext, int maxBufferSize) throws IO
// Is the content in the parent cache?
if (_parent != null)
{
HttpContent httpContent = _parent.getContent(pathInContext, maxBufferSize);
if (httpContent != null)
return httpContent;
return _parent.getContent(pathInContext, maxBufferSize);
}

return null;
Expand All @@ -222,7 +240,7 @@ private HttpContent load(String pathInContext, Resource resource, int maxBufferS
return null;

if (resource.isDirectory())
return new ResourceHttpContent(resource, _mimeTypes.getMimeByExtension(resource.toString()), getMaxCachedFileSize());
return new ResourceHttpContent(resource, _mimeTypes.getMimeByExtension(resource.toString()), getMaxCachedFileSize(), null, _bufferPool);

// Will it fit in the cache?
if (isCacheable(resource))
Expand All @@ -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())
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 !.

{
compressedContent = null;
Resource compressedResource = _factory.getResource(compressedPathInContext);
Expand Down Expand Up @@ -290,13 +308,13 @@ private HttpContent load(String pathInContext, Resource resource, int maxBufferS
if (compressedResource.exists() && compressedResource.lastModified() >= resource.lastModified() &&
compressedResource.length() < resource.length())
compressedContents.put(format,
new ResourceHttpContent(compressedResource, _mimeTypes.getMimeByExtension(compressedPathInContext), maxBufferSize));
new ResourceHttpContent(compressedResource, _mimeTypes.getMimeByExtension(compressedPathInContext), maxBufferSize, null, _bufferPool));
}
if (!compressedContents.isEmpty())
return new ResourceHttpContent(resource, mt, maxBufferSize, compressedContents);
return new ResourceHttpContent(resource, mt, maxBufferSize, compressedContents, _bufferPool);
}

return new ResourceHttpContent(resource, mt, maxBufferSize);
return new ResourceHttpContent(resource, mt, maxBufferSize, null, _bufferPool);
}

private void shrinkCache()
Expand Down Expand Up @@ -335,7 +353,7 @@ protected ByteBuffer getIndirectBuffer(Resource resource)
{
try
{
return BufferUtil.toBuffer(resource, false);
return ByteBufferPoolUtil.resourceToBuffer(resource, false, _bufferPool);
}
catch (IOException | IllegalArgumentException e)
{
Expand Down Expand Up @@ -366,7 +384,7 @@ protected ByteBuffer getDirectBuffer(Resource resource)
{
try
{
return BufferUtil.toBuffer(resource, true);
return ByteBufferPoolUtil.resourceToBuffer(resource, true, _bufferPool);
}
catch (IOException | IllegalArgumentException e)
{
Expand Down Expand Up @@ -485,6 +503,8 @@ boolean isValid()

protected void invalidate()
{
release();

ByteBuffer indirect = _indirectBuffer.getAndSet(null);
if (indirect != null)
_cachedSize.addAndGet(-BufferUtil.length(indirect));
Expand Down Expand Up @@ -550,6 +570,16 @@ public Type getMimeType()
@Override
public void release()
{
if (_bufferPool != null)
{
ByteBuffer indirectBuffer = _indirectBuffer.get();
if (indirectBuffer != null)
_bufferPool.release(indirectBuffer);

ByteBuffer directBuffer = _directBuffer.get();
if (directBuffer != null)
_bufferPool.release(directBuffer);
}
}

@Override
Expand Down Expand Up @@ -579,10 +609,11 @@ public ByteBuffer getIndirectBuffer()
}
else
{
_bufferPool.release(buffer2);
buffer = _indirectBuffer.get();
}
}
return buffer == null ? null : buffer.asReadOnlyBuffer();
return buffer.slice();
}

@Override
Expand Down Expand Up @@ -615,6 +646,7 @@ else if (_resource.length() < _maxCachedFileSize)
}
else
{
_bufferPool.release(direct);
buffer = _directBuffer.get();
}
}
Expand Down
Loading