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

check for http.NoBody as well as nil Body #5

Merged
merged 1 commit into from
Mar 9, 2023
Merged

check for http.NoBody as well as nil Body #5

merged 1 commit into from
Mar 9, 2023

Conversation

ajanata
Copy link
Contributor

@ajanata ajanata commented Mar 8, 2023

While converting some internal services to use HTTPS instead of HTTP, we were observing breakages related to having Transfer-Encoding: chunked and Content-Length: -1 on GET requests, which makes little sense. After some long debugging sessions, we finally located the cause: The internal Go http client is upgrading the request to HTTP2, and the code that determines the actual content length requires either a nil body or http.NoBody to send a Content-Length of 0. Since the check for http.NoBody was not here, an empty byte buffer was being created, which failed the checks and caused a Content-Length of -1 to be used elsewhere. Adding the check here fixes the problem for us.

(We use echo and it really does not like Content-Length: -1 when binding requests to structs, event for GET requests.)

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@mna
Copy link
Member

mna commented Mar 9, 2023

Thanks, the change looks good to me!

@mna mna merged commit 075425b into PuerkitoBio:master Mar 9, 2023
@ajanata ajanata deleted the fix-no-body branch March 9, 2023 05:44
@wolfeidau
Copy link

@mna are you going to tag a new release for this? Would be amazing if you did. 🙏🏻 😄

@mna
Copy link
Member

mna commented Jul 8, 2023

@wolfeidau done, thanks for the reminder!

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

Successfully merging this pull request may close these issues.

None yet

3 participants