Use FileChannel.transferTo() to send static content #13631
Conversation
jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java
Outdated
Show resolved
Hide resolved
| { | ||
| if (sink instanceof Content.Source.Aware aware) | ||
| return aware; | ||
| if (sink instanceof Wrapper wrapper) |
There was a problem hiding this comment.
I think if the sink is wrapped then we should not bypass it with transferTo
Perhaps we need an AwareWrapper, that would allow itself to be bypassed?
| { | ||
| // Optimization to enable zero-copy. | ||
| aware.setContentSource(source); | ||
| sink.write(last, TRANSFER, callback); |
There was a problem hiding this comment.
I feel that we should allow some failures on the callback here to fall through to a normal copy. I.e. if something in the implementation is Aware but cannot do a transferTo (perhaps because it would be non optimal) then it can fail this write with a TransferToFailed exception, which would then just do the normal copy below.
| Content.Source.Aware aware = findContentSourceAware(sink); | ||
| if (aware != null) | ||
| { | ||
| // Optimization to enable zero-copy. |
There was a problem hiding this comment.
Explain in the comment how the optimization still uses the sink.write path, so that commit logic etc. can be triggered. Specifically, we do not do aware.transfer(source, callback) because we might need to commit the response.
| return; | ||
| } | ||
|
|
||
| // TODO: check that we don't have both a ByteBuffer and a Content.Source. |
There was a problem hiding this comment.
I think you should also check content==TRANSFER, because if some sink wrapper inserted different content, then the presence of the source will need to be ignored
Signed-off-by: Simone Bordet <[email protected]>
Implemented `Response.write(Response, boolean, Content.Source, Callback)`. Signed-off-by: Simone Bordet <[email protected]>
Implemented `Sink.write(Response, boolean, Content.Source, Callback)`. Now the API is just the `Sink.write()` above, and most of the rest is hidden. Updated IOResources to use the new method above, so normal file serving should be able to leverage zero-copy. Signed-off-by: Simone Bordet <[email protected]>
Signed-off-by: Simone Bordet <[email protected]>
Signed-off-by: Simone Bordet <[email protected]>
Signed-off-by: Simone Bordet <[email protected]>
sbordet
force-pushed
the
fix/jetty-12.1.x/13392/response-content-source
branch
from
090d016 to
5eef9e3
Compare
TODO: * Review ByteChannelContentSource and subclasses for test failures. * Implement SeekableByteChannelContentSource.rewind(). * Review Handler.Wrappers that wrap Response to be aware of Sink.TRANSFER_TO. * Write tests to cover all the branches introduced, for example: ** Content.transfer() ** Content.write(Sink, boolean, Source, Callback) ** Write some ByteBuffer, then a transferable source, and viceversa. ** Etc. Signed-off-by: Simone Bordet <[email protected]>
No description provided.