Unnecessary blocking in ResourceService

[charlag]

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:

https://github.com/eclipse/jetty.project/blob/b16313b1c89d86f025cbbeccf7aa1a8861788c57/jetty-server/src/main/java/org/eclipse/jetty/server/ResourceService.java#L686-L687

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.

[joakime]

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]

I agree with the OP that the if statement in ResourceService should be split.

@charlag would you like to provide a PR?

[charlag]

@sbordet we just removed the size conditon completely and as you mention "split" it seems like you have something else in mind.

[sbordet]

@charlag yes I meant to remove the buffer size condition. @gregw do you recall why that check was added?

[joakime]

The ResourceService line for selecting async based on size came from a refactor to create the ResourceService in commit 460e6e2

Prior 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]

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._outputAggregationSize would be a better value to use here, short of making it directly configurable.

[charlag]

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]

@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]

@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.

[charlag]

@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.