Issue #12695 - don't return ByteBuffer from .toMappedBuffer if FS reports as UnsupportedOperationException. #12696
Conversation
…orts as UnsupportedOperationException.
|
This is an incomplete fix, as the usages of |
…hat do not support it.
+ Fallback to normal filesystem read if memory mapping is not supported.
| @@ -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 | |||
There was a problem hiding this comment.
While restoring FileBufferedResponseHandlerTest (from earlier versions of Jetty) this method in HttpTester was also restored.
...jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/FileBufferedResponseHandler.java
Outdated
Show resolved
Hide resolved
...jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/FileBufferedResponseHandler.java
Outdated
Show resolved
Hide resolved
...jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/FileBufferedResponseHandler.java
Outdated
Show resolved
Hide resolved
+ Add new .setUseFileMapping(boolean) flag (defaults to true) + Add test case variations for non-mapping + Fix position calculation
There was a problem hiding this comment.
I'm wondering if this whole implementation could be replaced with a simple Content.copy()?
Writing a Content.Sink that does _interceptor.getNextInterceptor().write(buffer, last, callback) looks trivial; so if a memory-mapping Content.Source could be created, with a fallback to Content.Source.from(Path) if mmap is disabled or unavailable could be written then the whole Content logic could be re-used.
...jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/FileBufferedResponseHandler.java
Outdated
Show resolved
Hide resolved
...jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/FileBufferedResponseHandler.java
Outdated
Show resolved
Hide resolved
...jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/FileBufferedResponseHandler.java
Outdated
Show resolved
Hide resolved
+ Remove useFileMapping config + Remove custom IteratingCallback + Use Content.Source.from(Path) + New InterceptorSink
| * @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) |
There was a problem hiding this comment.
I don't think I like this new helper as it takes parameters that could be ignored without any hint.
| private final long fileLength = _filePath.toFile().length(); | ||
| private long _pos = 0; | ||
| private boolean _last = false; | ||
| ByteBufferPool.Sized sizedPool = ByteBufferPool.Sized.as(getServer().getByteBufferPool(), true, getBufferSize()); |
There was a problem hiding this comment.
I think I'd rather see a new instance of ByteBufferPool.Sized being created.
| _pos += len; | ||
| return Action.SCHEDULED; | ||
| } | ||
| private static class InterceptorSink implements Content.Sink |
There was a problem hiding this comment.
You could inline this class as a simple lambda:
Content.Sink sink = (last, byteBuffer, callback) -> _interceptor.write(last, byteBuffer, callback);| private long _pos = 0; | ||
| private boolean _last = false; | ||
| ByteBufferPool.Sized sizedPool = ByteBufferPool.Sized.as(getServer().getByteBufferPool(), true, getBufferSize()); | ||
| Content.Source source = Content.Source.from(sizedPool, _filePath); |
There was a problem hiding this comment.
This does not mmap files naymore, but honestly I think it is for the best.
There was a problem hiding this comment.
Supporting mmap from Content.Source.from(path) is still possible, but in a future PR.
To fix issue with UnsupportedOperationException, just have toMappedBuffer return null (that is an existing contract) if that exception is triggered.