Skip to content

Commit

Permalink
Issue #12695 - Cleanup FileBufferedResponseHandler
Browse files Browse the repository at this point in the history 
+ Remove useFileMapping config
+ Remove custom IteratingCallback
+ Use Content.Source.from(Path)
+ New InterceptorSink
  • Loading branch information
@joakime
joakime committed Jan 14, 2025
1 parent 238569c commit a7996c5
Show file tree
Hide file tree
Showing 3 changed files with 34 additions and 165 deletions.
15 changes: 15 additions & 0 deletions jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferPool.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,21 @@ class Sized extends Wrapper
private final boolean _direct;
private final int _size;

/**
* Get a sized pool for any arbitrary ByteBufferPool.
*
* @param pool the pool
* @param direct {@code true} for direct buffers (if wrapping pool)
* @param size The specified size in bytes of the buffer, or -1 for a default
* @return the pool as a sized pool
*/
public static ByteBufferPool.Sized as(ByteBufferPool pool, boolean direct, int size)
{
if (pool instanceof Sized sized)
return sized;
return new Sized(pool, direct, size);
}

/**
* Create a sized pool for non direct buffers of a default size from a wrapped pool.
* @param wrapped The actual {@link ByteBufferPool}
Expand Down
164 changes: 13 additions & 151 deletions ...ty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/FileBufferedResponseHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,17 +17,17 @@
import java.io.IOException;
import java.io.OutputStream;
import java.nio.ByteBuffer;
import java.nio.channels.SeekableByteChannel;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.StandardOpenOption;
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 Down Expand Up @@ -55,7 +55,6 @@ public class FileBufferedResponseHandler extends BufferedResponseHandler

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

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

public boolean isUseFileMapping()
{
return _useFileMapping;
}

public void setUseFileMapping(boolean useFileMapping)
{
this._useFileMapping = useFileMapping;
}

public int getBufferSize()
{
return _bufferSize;
Expand Down Expand Up @@ -215,154 +204,27 @@ private void commit(Callback callback)
}

// Create an iterating callback to do the writing
try
{
SendFileCallback sfcb = new SendFileCallback(this, _filePath, getBufferSize(), callback);
sfcb.setUseFileMapping(isUseFileMapping());
sfcb.iterate();
}
catch (IOException e)
{
callback.failed(e);
}
ByteBufferPool.Sized sizedPool = ByteBufferPool.Sized.as(getServer().getByteBufferPool(), true, getBufferSize());
Content.Source source = Content.Source.from(sizedPool, _filePath);
Content.Sink sink = new InterceptorSink(getNextInterceptor());
Callback disposer = Callback.from(callback, this::dispose);
Content.copy(source, sink, disposer);
}
}

// TODO: can this be made generic enough to put into jetty-io somewhere?
private static class SendFileCallback extends IteratingCallback
private static class InterceptorSink implements Content.Sink
{
private static final int MAX_BUFFER_SIZE = Integer.MAX_VALUE / 2;
private final Path _filePath;
private final long _fileLength;
private final FileBufferedInterceptor _interceptor;
private final Callback _callback;
private final int _bufferSize;
private long _pos = 0;
private boolean _last = false;
private Mode _mode = Mode.DISCOVER;

enum Mode
{
DISCOVER,
MAPPED,
READ
}

public SendFileCallback(FileBufferedInterceptor interceptor, Path filePath, int bufferSize, Callback callback) throws IOException
{
_filePath = filePath;
_fileLength = Files.size(filePath);
_interceptor = interceptor;
_callback = callback;
_bufferSize = bufferSize;
}

public void setUseFileMapping(boolean useFileMapping)
{
if (!useFileMapping)
_mode = Mode.READ; // don't even attempt file mapping
else
_mode = Mode.DISCOVER; // attempt file mapping first
}

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

long len = Math.min(MAX_BUFFER_SIZE, _fileLength - _pos);
ByteBuffer buffer = readByteBuffer(_filePath, _pos, len);
if (buffer == null)
{
buffer = BufferUtil.EMPTY_BUFFER;
_last = true;
}
else
{
_last = (_pos + buffer.remaining() == _fileLength);
}
int read = buffer.remaining();
_interceptor.getNextInterceptor().write(buffer, _last, this);
_pos += read;
return Action.SCHEDULED;
}
private final HttpOutput.Interceptor _interceptor;

@Override
protected void onCompleteSuccess()
public InterceptorSink(HttpOutput.Interceptor interceptor)
{
_interceptor.dispose();
_callback.succeeded();
this._interceptor = interceptor;
}

@Override
protected void onFailure(Throwable cause)
{
_interceptor.dispose();
_callback.failed(cause);
}

/**
* Read the ByteBuffer from the path.
*
* @param path the path to read from
* @param pos the position in the file to start from
* @param len the length of the buffer to use for memory mapped mode
* @return the buffer read, or null if no buffer has been read (such as being at EOF)
* @throws IOException if unable to read from the path
*/
private ByteBuffer readByteBuffer(Path path, long pos, long len) throws IOException
{
return switch (_mode)
{
case DISCOVER ->
{
ByteBuffer buffer = toMapped(path, pos, len);
if (buffer == null)
{
// if we reached here, then file mapped byte buffers is not supported.
// we fall back to using traditional I/O instead.
buffer = toRead(path, pos);
}
yield buffer;
}
case MAPPED ->
{
yield toMapped(path, pos, len);
}
case READ ->
{
yield toRead(path, pos);
}
};
}

private ByteBuffer toMapped(Path path, long pos, long len) throws IOException
public void write(boolean last, ByteBuffer byteBuffer, Callback callback)
{
if (pos > _fileLength)
{
// attempt to read past end of file, consider this an EOF
return null;
}
ByteBuffer buffer = BufferUtil.toMappedBuffer(path, pos, len);
if (buffer != null)
_mode = Mode.MAPPED;
return buffer;
}

private ByteBuffer toRead(Path path, long pos) throws IOException
{
try (SeekableByteChannel channel = Files.newByteChannel(path))
{
_mode = Mode.READ;
channel.position(pos);
ByteBuffer buffer = ByteBuffer.allocateDirect(_bufferSize);
int read = channel.read(buffer);
if (read == -1)
return null; // indicating EOF
buffer.flip();
return buffer;
}
_interceptor.write(last, byteBuffer, callback);
}
}
}
20 changes: 6 additions & 14 deletions ...e9-nested/src/test/java/org/eclipse/jetty/ee9/nested/FileBufferedResponseHandlerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,6 @@
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.condition.OS;
import org.junit.jupiter.api.extension.ExtendWith;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down Expand Up @@ -257,11 +255,9 @@ public void handle(String target, org.eclipse.jetty.ee9.nested.Request baseReque
assertThat(getNumFiles(), is(0));
}

@ParameterizedTest
@ValueSource(booleans = {true, false})
public void testFlushed(boolean useFileMapping) throws Exception
@Test
public void testFlushed() throws Exception
{
bufferedHandler.setUseFileMapping(useFileMapping);
bufferedHandler.setHandler(new AbstractHandler()
{
@Override
Expand Down Expand Up @@ -335,13 +331,11 @@ public void handle(String target, org.eclipse.jetty.ee9.nested.Request baseReque
}
}

@ParameterizedTest
@ValueSource(booleans = {true, false})
public void testBufferSizeBig(boolean useFileMapping) throws Exception
@Test
public void testBufferSizeBig() throws Exception
{
int bufferSize = 4096;
String largeContent = generateContent(bufferSize - 64);
bufferedHandler.setUseFileMapping(useFileMapping);
bufferedHandler.setHandler(new AbstractHandler()
{
@Override
Expand Down Expand Up @@ -451,14 +445,12 @@ public void handle(String target, org.eclipse.jetty.ee9.nested.Request baseReque
}
}

@ParameterizedTest
@ValueSource(booleans = {true, false})
public void testFileLargerThanMaxInteger(boolean useFileMapping) throws Exception
@Test
public void testFileLargerThanMaxInteger() throws Exception
{
long fileSize = Integer.MAX_VALUE + 1234L;
byte[] bytes = randomBytes(1024 * 1024);

bufferedHandler.setUseFileMapping(useFileMapping);
bufferedHandler.setHandler(new AbstractHandler()
{
@Override
Expand Down

0 comments on commit a7996c5

Please sign in to comment.