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.

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

Decide whether to Chunked after body encoding. #1736

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

Decide whether to Chunked after body encoding. #1736

wants to merge 1 commit into from

Conversation

siwee
Copy link

@siwee siwee commented Oct 9, 2020

Motivation:

Decide whether to Chunked after body encoding. Netty will automatically calculate the Content-length. When the Content-length is greater than 8k, it will not set the Content-length header value, so we can decide whether to Chunk or not based on it.
If it is up to the developer to set the content-length value, it is likely to cause an error. In addition, when sending the application/x-www-form-urlencoded form, it will cause chunked=true, which is not necessary, usually the body is very small.

Conformance:

Your commits should be signed and you should have signed the Eclipse Contributor Agreement as explained in https://github.com/eclipse/vert.x/blob/master/CONTRIBUTING.md
Please also make sure you adhere to the code style guidelines: https://github.com/vert-x3/wiki/wiki/Vert.x-code-style-guidelines

@vietj
Copy link
Contributor

vietj commented Oct 9, 2020

it is not clear what this PR is doing, can you elaborate ?

@siwee
Copy link
Author

siwee commented Oct 10, 2020

@vietj Hi.

I'm so sorry, English is not my native language. 😂

I think these may be problematic.

When we send the application/x-www-form-urlencoded form, no matter the size of the body, it will be sent in Chunked mode. Is this necessary? Most of the body is very small, I think this increases the processing complexity.

@siwee
Copy link
Author

siwee commented Oct 10, 2020

Currently, the Content-length is verified through the HttpRequest Header, which may be incorrect, and we do not know the specific value through it.

@siwee
Copy link
Author

siwee commented Oct 10, 2020

I tested this PR and it works well.

@vietj
Copy link
Contributor

vietj commented Oct 10, 2020

You need to add a test, then I'll have a look, perhaps it can be handled in vertx-core instead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants