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 #12695 - don't return ByteBuffer from .toMappedBuffer if FS reports as UnsupportedOperationException. #12696

Merged
merged 7 commits into from
Jan 15, 2025
Original file line number Diff line number Diff line change
Expand Up @@ -262,17 +262,25 @@ public static Response parseResponse(Input input) throws IOException
return parseResponse(input, false);
}

public static Response parseResponse(Input input, Response response) throws IOException
Copy link
Contributor Author

Choose a reason for hiding this comment

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

While restoring FileBufferedResponseHandlerTest (from earlier versions of Jetty) this method in HttpTester was also restored.

{
return parseResponse(input, response, false);
}

public static Response parseResponse(Input input, boolean head) throws IOException
{
Response response;
return parseResponse(input, new Response(), head);
}

public static Response parseResponse(Input input, Response response, boolean head) throws IOException
{
HttpParser parser = input.takeHttpParser();
if (parser != null)
{
response = (Response)parser.getHandler();
}
else
{
response = new Response();
parser = new HttpParser(response);
}
parser.setHeadResponse(head);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,8 @@ private SingleBufferFileMappedHttpContent(HttpContent content) throws IOExceptio
if (path == null)
throw new IOException("Cannot memory map Content whose Resource is not backed by a Path: " + content.getResource());
_buffer = BufferUtil.toMappedBuffer(path);
if (_buffer == null)
throw new IOException("Cannot memory map Content (not supported by underlying FileSystem): " + content.getResource());
_contentLength = new HttpField(HttpHeader.CONTENT_LENGTH, Integer.toString(_buffer.remaining()));
_lastModified = content.getLastModified();
_lastModifiedInstant = content.getLastModifiedInstant();
Expand Down Expand Up @@ -176,6 +178,8 @@ private MultiBufferFileMappedHttpContent(HttpContent content, int maxBufferSize)
{
long len = Math.min(contentLength - currentPos, maxBufferSize);
_buffers[i] = BufferUtil.toMappedBuffer(path, currentPos, len);
if (_buffers[i] == null)
throw new IOException("Cannot memory map Content (not supported by underlying FileSystem): " + content.getResource());
currentPos += len;
total += _buffers[i].remaining();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.io.UnsupportedEncodingException;
import java.nio.Buffer;
import java.nio.BufferOverflowException;
import java.nio.ByteBuffer;
Expand Down Expand Up @@ -1166,19 +1167,48 @@ public static ByteBuffer toDirectBuffer(String s, Charset charset)
return buf;
}

/**
* Attempt to return a ByteBuffer created via {@link FileChannel#map} call.
*
* @param path the path to map
* @return the mapped bytebuffer, or null if unable to map the {@link FileChannel}
* @throws IOException if unable to open mapped byte buffer
*/
public static ByteBuffer toMappedBuffer(Path path) throws IOException
{
return toMappedBuffer(path, 0, Files.size(path));
}

/**
* Attempt to return a ByteBuffer created via {@link FileChannel#map} call.
*
* @param filePath the path to map
* @param pos the position to start from
* @param len the length to support
* @return the mapped bytebuffer, or null if unable to map the {@link FileChannel}
* @throws IOException if unable to open mapped byte buffer
*/
public static ByteBuffer toMappedBuffer(Path filePath, long pos, long len) throws IOException
{
try (FileChannel channel = FileChannel.open(filePath, StandardOpenOption.READ))
{
return channel.map(MapMode.READ_ONLY, pos, len);
}
catch (UnsupportedEncodingException e)
{
if (LOG.isTraceEnabled())
LOG.trace("Ignored exception", e);
return null;
}
}

/**
* Attempt to return a ByteBuffer created via {@link FileChannel#map} call.
*
* @param resource the resource to map
* @return the mapped bytebuffer, or null if unable to map the {@link FileChannel} (usually due to Resource not supporting memory mapped files)
* @throws IOException if unable to open mapped byte buffer
*/
public static ByteBuffer toMappedBuffer(Resource resource) throws IOException
{
Path path = resource.getPath();
Expand All @@ -1187,6 +1217,15 @@ public static ByteBuffer toMappedBuffer(Resource resource) throws IOException
return toMappedBuffer(path);
}

/**
* Attempt to return a ByteBuffer created via {@link FileChannel#map} call.
*
* @param resource the resource to map
* @param pos the position to start from
* @param len the length to support
* @return the mapped bytebuffer, or null if unable to map the {@link FileChannel} (usually due to Resource not supporting memory mapped files)
* @throws IOException if unable to open mapped byte buffer
*/
public static ByteBuffer toMappedBuffer(Resource resource, long pos, long len) throws IOException
{
Path path = resource.getPath();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,11 @@
import java.util.Objects;

import org.eclipse.jetty.ee9.nested.HttpOutput.Interceptor;
import org.eclipse.jetty.io.ByteBufferPool;
import org.eclipse.jetty.io.Content;
import org.eclipse.jetty.util.BufferUtil;
import org.eclipse.jetty.util.Callback;
import org.eclipse.jetty.util.IO;
import org.eclipse.jetty.util.IteratingCallback;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand All @@ -52,6 +53,8 @@ public class FileBufferedResponseHandler extends BufferedResponseHandler
{
private static final Logger LOG = LoggerFactory.getLogger(FileBufferedResponseHandler.class);

private static final int DEFAULT_BUFFER_SIZE = 64 * 1024;
private int _bufferSize = DEFAULT_BUFFER_SIZE;
private Path _tempDir = new File(System.getProperty("java.io.tmpdir")).toPath();

public Path getTempDir()
Expand All @@ -64,6 +67,16 @@ public void setTempDir(Path tempDir)
_tempDir = Objects.requireNonNull(tempDir);
}

public int getBufferSize()
{
return _bufferSize;
}

public void setBufferSize(int bufferSize)
{
this._bufferSize = bufferSize;
}

@Override
protected BufferedInterceptor newBufferedInterceptor(HttpChannel httpChannel, Interceptor interceptor)
{
Expand All @@ -72,8 +85,6 @@ protected BufferedInterceptor newBufferedInterceptor(HttpChannel httpChannel, In

class FileBufferedInterceptor implements BufferedResponseHandler.BufferedInterceptor
{
private static final int MAX_MAPPED_BUFFER_SIZE = Integer.MAX_VALUE / 2;

private final Interceptor _next;
private final HttpChannel _channel;
private Boolean _aggregating;
Expand Down Expand Up @@ -193,41 +204,11 @@ private void commit(Callback callback)
}

// Create an iterating callback to do the writing
IteratingCallback icb = new IteratingCallback()
{
private final long fileLength = _filePath.toFile().length();
private long _pos = 0;
private boolean _last = false;

@Override
protected Action process() throws Exception
{
if (_last)
return Action.SUCCEEDED;

long len = Math.min(MAX_MAPPED_BUFFER_SIZE, fileLength - _pos);
_last = (_pos + len == fileLength);
ByteBuffer buffer = BufferUtil.toMappedBuffer(_filePath, _pos, len);
getNextInterceptor().write(buffer, _last, this);
_pos += len;
return Action.SCHEDULED;
}

@Override
protected void onCompleteSuccess()
{
dispose();
callback.succeeded();
}

@Override
protected void onFailure(Throwable cause)
{
dispose();
callback.failed(cause);
}
};
icb.iterate();
ByteBufferPool.Sized sizedPool = new ByteBufferPool.Sized(getServer().getByteBufferPool(), true, getBufferSize());
Content.Source source = Content.Source.from(sizedPool, _filePath);
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not mmap files naymore, but honestly I think it is for the best.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Supporting mmap from Content.Source.from(path) is still possible, but in a future PR.

Content.Sink sink = (last, bytebuffer, cb) -> getNextInterceptor().write(last, bytebuffer, cb);
Callback disposer = Callback.from(callback, this::dispose);
Content.copy(source, sink, disposer);
}
}
}
Loading
Loading