Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Early Rejection of Large Requests Based on Content-Length Header #6032

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Early Rejection of Large Requests Based on Content-Length Header #6032

wants to merge 1 commit into from

Conversation

yzfeng2020
Copy link
Contributor

@yzfeng2020 yzfeng2020 commented Dec 12, 2024 •
edited
Loading

Motivation:

Currently, large requests are rejected only after the transferred bytes exceed the configured limit. This behavior is sub-optimal for requests that include a valid Content-Length header indicating the request is already too large. By rejecting such requests earlier—when the header is read—we can improve resource utilization and reduce unnecessary processing.

Modifications:

  • Added a field to ContentTooLargeException to indicate when the exception is raised during header processing.
  • Implemented content-length-based early rejection in Http1ObjectDecoder and Http2ObjectDecoder.
    Result:

Result:

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 12, 2024 •
edited
Loading

🔍 Build Scan® (commit: 990f4cb)

Job name Status Build Scan®
build-ubicloud-standard-8-jdk-8 ✅ https://ge.armeria.dev/s/t2cxpuxbrodm4
build-ubicloud-standard-8-jdk-21-snapshot-blockhound ✅ https://ge.armeria.dev/s/ssgorqbuo7wza
build-ubicloud-standard-8-jdk-17-min-java-17-coverage ✅ https://ge.armeria.dev/s/4o2c6wbejsz66
build-ubicloud-standard-8-jdk-17-min-java-11 ✅ https://ge.armeria.dev/s/pg6jhndu75hdk
build-ubicloud-standard-8-jdk-17-leak ✅ https://ge.armeria.dev/s/god7locg2ibu4
build-ubicloud-standard-8-jdk-11 ✅ https://ge.armeria.dev/s/kjde7a6ogrg5o
build-macos-latest-jdk-21 ✅ https://ge.armeria.dev/s/ivl7fole4rqbw

@yzfeng2020 yzfeng2020 force-pushed the early-reject-large-req branch from fd7fe9e to 0a25899 Compare December 12, 2024 08:20
yzfeng2020
yzfeng2020 commented Dec 12, 2024
View reviewed changes
core/src/test/java/com/linecorp/armeria/server/HttpServerTest.java Outdated
@@ -600,8 +600,8 @@ void testTimeoutAfterPartialContentWithPooling(WebClient client) throws Exceptio
void testTooLargeContentToNonExistentService(WebClient client) {
final byte[] content = new byte[(int) MAX_CONTENT_LENGTH + 1];
final AggregatedHttpResponse res = client.post("/non-existent", content).aggregate().join();
assertThat(res.status()).isSameAs(HttpStatus.NOT_FOUND);
assertThat(res.contentUtf8()).startsWith("Status: 404\n");
assertThat(res.status()).isSameAs(HttpStatus.REQUEST_ENTITY_TOO_LARGE);
Copy link
Contributor Author

@yzfeng2020 yzfeng2020 Dec 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i had to change this test as it's rejected earlier now, is there a way to fix it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To simulate a client request with no content-length header, we can probably make a transfer-encoding request.

e.g.

val req = HttpRequest.streaming(POST, "/non-existent);
client.execute(req)...
req.write(content)
req.close();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sense, updated!

yzfeng2020
yzfeng2020 commented Dec 12, 2024
View reviewed changes
core/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.java Outdated
@@ -472,6 +472,11 @@ public Boolean allowSemicolonInPathComponent() {
return false;
}

@Override
public Boolean allowLargeRequestEarlyRejection() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to guard this feature under a flag..

@yzfeng2020 yzfeng2020 marked this pull request as ready for review December 13, 2024 08:55
jrhee17
jrhee17 reviewed Dec 18, 2024
View reviewed changes
Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, left some minor comments

core/src/main/java/com/linecorp/armeria/common/ContentTooLargeExceptionBuilder.java Outdated Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/server/Http1RequestDecoder.java Outdated Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/server/Http1RequestDecoder.java Outdated Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/server/Http2RequestDecoder.java Outdated Show resolved Hide resolved
@yzfeng2020 yzfeng2020 force-pushed the early-reject-large-req branch from 36bb54d to c114764 Compare December 20, 2024 06:27
@yzfeng2020 yzfeng2020 changed the title early reject Early Rejection of Large Requests Based on Content-Length Header Dec 20, 2024
@yzfeng2020 yzfeng2020 force-pushed the early-reject-large-req branch from 06f9c52 to 990f4cb Compare December 20, 2024 09:42
@yzfeng2020 yzfeng2020 requested a review from jrhee17 December 20, 2024 09:43
jrhee17
jrhee17 approved these changes Dec 30, 2024
View reviewed changes
Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 👍 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrhee17 jrhee17 jrhee17 approved these changes

@ikhoon ikhoon Awaiting requested review from ikhoon ikhoon is a code owner

@minwoox minwoox Awaiting requested review from minwoox minwoox is a code owner

@trustin trustin Awaiting requested review from trustin trustin is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Early rejection for requests exceeding configured maxRequestLength
2 participants
@yzfeng2020 @jrhee17