Unnecessary blocking in ResourceService #5937
Description
Jetty version
9.4.27.v20200227
Java version
14
OS type/version
GNU/Linux
Description
It seems Jetty is doing unnecessary blocking in ResourceService. There's a decision taken based on the content size:
If the data size is smaller than buffer, it is still sent async but with blocking:
https://github.com/eclipse/jetty.project/blob/b16313b1c89d86f025cbbeccf7aa1a8861788c57/jetty-server/src/main/java/org/eclipse/jetty/server/ResourceService.java#L722
https://github.com/eclipse/jetty.project/blob/b16313b1c89d86f025cbbeccf7aa1a8861788c57/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java#L1136-L1143
(line is 1183 for the verison I report but I provide links to the current tree).
We tried to find out justification for doing that but we couldn't. As it's always sent async there's anyway a context switch. It also seems like an attack vector to starve thread pool by requesting small files.
We tried out version which always sends data async, independent of the size and didn't find any issues. As this looks like an oversight to us we would like to report this.
Activity
joakime commentedon Feb 2, 2021
Have you changed your response buffer size?
At default value, if the content size is less than the buffer size, that sendContent should never block.
It's a simpler code path than the async one, with the same end result, send of the content without blocking.
@gregw Perhaps we should just skip the simpler code path if the response buffer size has been changed from default?
sbordet commentedon Feb 3, 2021
I agree with the OP that the
ifstatement inResourceServiceshould be split.@charlag would you like to provide a PR?
charlag commentedon Feb 3, 2021
@sbordet we just removed the size conditon completely and as you mention "split" it seems like you have something else in mind.
sbordet commentedon Feb 3, 2021
@charlag yes I meant to remove the buffer size condition. @gregw do you recall why that check was added?
joakime commentedon Feb 3, 2021
The
ResourceServiceline for selecting async based on size came from a refactor to create theResourceServicein commit 460e6e2Prior to that, the line in question was in the
ResourceHandler.https://github.com/eclipse/jetty.project/blob/7b713507c68a07c3031dfd1aa4077450cd271573/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ResourceHandler.java#L522-L528
Which was added by commit 0176856#diff-6762e0c2681e4060b6d130dcfb33f458af0ce52a9c033d0fc11036b4fb3c98dbR506
gregw commentedon Feb 3, 2021
My recollection is that small content was not explicitly sent using asynchronous because it was aggregated into the output buffer, which could then asynchronously flushed. Also, small contents are less likely to block and can be entirely flushed within the call to sendContent, so the blocker may never block.
However, I'm not so sure that the response buffer size is a reasonable definition of "small content" now. Flip side is that I also don't think it is often worthwhile to startAsync just to send a few bytes of static content. Perhaps the
HttpConfiguration._outputAggregationSizewould be a better value to use here, short of making it directly configurable.charlag commentedon Feb 5, 2021
For us introduction of many small files resulted in exhausted thread pool (and downtime) with blocking in that place in HttpOutput. I checked and I didn't find us changing buffer size anywhere but I'm not 100% on this to be honest.
gregw commentedon Feb 5, 2021
@charlag Interesting! Can you give us some numbers on "small" and "many" ?
I think it is a no-brainer that we should change this to HttpConfiguration._outputAggregationSize, second option is make it configurable and third option is always go async if possible. @sbordet @lorban thoughts?
sbordet commentedon Feb 5, 2021
@gregw IIUC this is either past the aggregation, which only happens in
HttpOutput.write(), or a direct call to serve static content from e.g.DefaultServlet.I fail to see why there is any logic in there about buffer sizing -- I would just remove the check.
Fix #5937
charlag commentedon Feb 5, 2021
@gregw with new release of https://mail.tutanota.com (still up) we had around 50 bundles, few kb each. As browsers started detecting the update files started being downloaded by few hundred thousand users at once.
I hope this is enough info, feel free to ask for more details.
Fix #5937 (#5949)
Fix #5937