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

fix: accidentally set content-length to 0 #2207

Merged
merged 2 commits into from
Mar 27, 2024

Conversation

zuisong
Copy link
Contributor

@zuisong zuisong commented Mar 26, 2024

closes #2202

@seanmonstar
Copy link
Owner

This looks right to me. Did you want it to stay a draft?

@zuisong
Copy link
Contributor Author

zuisong commented Mar 26, 2024

I'm thinking about how to write a test case to ensure that this issue does not reoccur.
I noticed that all off test cases for the reqwest project can run offline. Can I add an online test case? (Or maybe it's better to write an offline version of the test case.)

@seanmonstar
Copy link
Owner

It's preferable that the tests be local, so that way CI doesn't fail because an external server is having problems.

@seanmonstar
Copy link
Owner

One option that doesn't involve a full integration test: add a unit test to the bottom of the body.rs file.

Also, I noticed in http-body-util (and confirmed it's what hyper notices first) is that Full with an empty string returns is_end_stream() as true. That tells hyper there is definitely no body.

@zuisong zuisong marked this pull request as ready for review March 26, 2024 13:46
@zuisong
Copy link
Contributor Author

zuisong commented Mar 26, 2024

Just now I discovered, when I set the client with no_proxy, the issue is also reproduced while using the test http service.

Now test case is ready

@zuisong zuisong marked this pull request as draft March 26, 2024 13:54
@zuisong zuisong marked this pull request as ready for review March 26, 2024 14:08
@zuisong zuisong force-pushed the issue-2202 branch 2 times, most recently from 7511014 to 707d757 Compare March 26, 2024 14:43
@zuisong zuisong marked this pull request as draft March 26, 2024 16:38
@zuisong
Copy link
Contributor Author

zuisong commented Mar 26, 2024

Got a strange question. see ducaale/xh#357 (comment)

may be we should find a better way to solve this question

@seanmonstar
Copy link
Owner

Yea, I understand now: the default size hint is "between 0 and infinity". In that case, hyper uses chunked encoding, since the exact size isn't known.

However, if you edit the is_end_stream method of Body to return true when empty, it will be fixed.

The unit test can be updated to also check for a transfer-encoding header.

@zuisong zuisong force-pushed the issue-2202 branch 4 times, most recently from bf9ad87 to 30c64a3 Compare March 27, 2024 00:26
@zuisong zuisong marked this pull request as ready for review March 27, 2024 00:27
@seanmonstar seanmonstar merged commit 68a3f58 into seanmonstar:master Mar 27, 2024
33 checks passed
@zuisong zuisong deleted the issue-2202 branch March 27, 2024 13:08
kodiakhq bot pushed a commit to pdylanross/fatigue that referenced this pull request Apr 8, 2024
Bumps reqwest from 0.12.2 to 0.12.3.

Release notes
Sourced from reqwest's releases.

v0.12.3
What's Changed

Add FromStr for dns::Name.
Add ClientBuilder::built_in_webpki_certs(bool) to enable them separately.
Add ClientBuilder::built_in_native_certs(bool) to enable them separately.
Fix sending content-length: 0 for GET requests.
Fix response body content_length() to return value when timeout is configured.
Fix ClientBuilder::resolve() to use lowercase domain names.

New Contributors

@​zuisong made their first contribution in seanmonstar/reqwest#2207
@​djc made their first contribution in seanmonstar/reqwest#2222
@​krant made their first contribution in seanmonstar/reqwest#2226
@​Kriskras99 made their first contribution in seanmonstar/reqwest#2236

Full Changelog: seanmonstar/reqwest@v0.12.2...v0.12.3



Changelog
Sourced from reqwest's changelog.

v0.12.3

Add FromStr for dns::Name.
Add ClientBuilder::built_in_webpki_certs(bool) to enable them separately.
Add ClientBuilder::built_in_native_certs(bool) to enable them separately.
Fix sending content-length: 0 for GET requests.
Fix response body content_length() to return value when timeout is configured.
Fix ClientBuilder::resolve() to use lowercase domain names.




Commits

0720159 v0.12.3
9209695 Remove duplicate example for ClientBuilder::default_headers (#2236)
e3a1565 fix: use lower case domain string when using resolve and resolve_to_addrs...
b4c491a feat: allow fine-grained root certs for rustls (#2232)
cf4295d chore: update winreg to 0.52.0 (#2226)
db25e80 chore: upgrade base64 to 0.22 (#2224)
13e27b7 fix: response body timeout forwards the size hint
872af0c refactor: upgrade to rustls-pemfile 2 (#2222)
68a3f58 fix: stop sending content-length: 0 for GET requests (#2207)
14e46ff FromStr trait implementation for Name (#2212)
See full diff in compare view




Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.


Dependabot commands and options

You can trigger Dependabot actions by commenting on this PR:

@dependabot rebase will rebase this PR
@dependabot recreate will recreate this PR, overwriting any edits that have been made to it
@dependabot merge will merge this PR after your CI passes on it
@dependabot squash and merge will squash and merge this PR after your CI passes on it
@dependabot cancel merge will cancel a previously requested merge and block automerging
@dependabot reopen will reopen this PR if it is closed
@dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
@dependabot show <dependency name> ignore conditions will show all of the ignore conditions of the specified dependency
@dependabot ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
@dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
@dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
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.

400 Bad Request after upgrading to 0.12.1
2 participants