-
Notifications
You must be signed in to change notification settings - Fork 2k
Use FileChannel.transferTo() to send static content #13631
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
base: jetty-12.1.x
Are you sure you want to change the base?
Use FileChannel.transferTo() to send static content #13631
Conversation
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]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm hating this less
interface Aware | ||
{ | ||
Source getContentSource(); | ||
|
||
void setContentSource(Source source); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #13656 as an alternate way to tunnel a Source through a ByteBuffer only API.
Although I'm hating the current iteration of Aware less than I did originally
{ | ||
if (sink instanceof Content.Source.Aware aware) | ||
return aware; | ||
if (sink instanceof Wrapper wrapper) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
No description provided.