-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Clarify that Response::content_length() is not derived from a Content-Length header in docs
#2588
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
Clarify that Response::content_length() is not derived from a Content-Length header in docs
#2588
Conversation
Response::content_length() is not derived from a Content-Length headerResponse::content_length() is not derived from a Content-Length header in docs
2962dae to
95d3a79
Compare
src/async_impl/response.rs
Outdated
| /// | ||
| /// This value is not computed by parsing the `Content-Length` header of the | ||
| /// response, but by looking at the number of bytes actually streamed from | ||
| /// the server. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't quite true, either. It does depend on the content-length header. Without out, we can't know. But it doesn't only look at it. If the semantics of the request method or response status don't allow for a body, then it gets changed to 0.
Get the number of bytes that make up the response body, if known.
The value is based off the `content-length` header, but also depends on the
semantics of the request method or response status. For instance, a response
to a `HEAD` request may include a `content-length`, but the response body
will have exactly 0 bytes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can tell, this is not directly true, though. Strictly speaking, the content_length method seems to only look at the size of the response body, and not at any header - at least as far as I could see while following the source code.
It might be that the number of bytes reqwest (or the http crate) writes into the response body when receiving it is limited in some way by the received Content-Length header, but I'm failing to find the place where this is done either in reqwest or in http (though please feel free to point me to it, as I'm curious to figure this out).
Regardless, my point in opening this PR is more about informing the user of what this return value represents, i.e. the size of the response's body and not the direct value of the Content-Length header, without bothering users with implementation details that aren't relevant to them. Though I maybe could have phrased this differently. How would something like this sound to you?
This value does not directly represents the value of the
Content-Lengthheader, but rather the size of the response's body. To read the header's value, please use theResponse::headersmethod instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strictly speaking, the content_length method seems to only look at the size of the response body, and not at any header
True, and the body type coming from hyper sets the size based on the content-length header.
I think the suggested block I provided last comment strikes a good balance with what it is, and also the most common reeason it isn't what they might have hoped. Well, but re-reading both again, I can see how what you suggested just now is even simpler. I don't know, we can try the simpler one if you prefer. Hopefully people will find it less confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think documenting implementation details in usage docs can be a slippery slope, both in terms of providing information most users might not really need, and the risk of this documentation getting out of sync with reality if the implementation (either in the "main" code or in a dependency) ends up changing.
I've updated the PR with my previous suggestion, let me know if it looks good to you, or if you'd like me to make changes to the wording 🙂
13cab48 to
9ddc869
Compare
|
(force-pushed to fix authorship and signatures in the commits) |
…not based on the `Content-Length` header (#558) sync upstream: seanmonstar/reqwest#2588
Bumps reqwest from 0.12.15 to 0.12.16. Release notes Sourced from reqwest's releases. v0.12.16 Highlights Add ClientBuilder::http3_congestion_bbr() to enable BBR congestion control. Add ClientBuilder::http3_send_grease() to configure whether to send use QUIC grease. Add ClientBuilder::http3_max_field_section_size() to configure the maximum response headers. Add ClientBuilder::tcp_keepalive_interval() to configure TCP probe interval. Add ClientBuilder::tcp_keepalive_retries() to configure TCP probe count. Add Proxy::headers() to add extra headers that should be sent to a proxy. Fix redirect::Policy::limit() which had an off-by-1 error, allowing 1 more redirect than specified. Fix HTTP/3 to support streaming request bodies. (wasm) Fix null bodies when calling Response::bytes_stream(). What's Changed Clarify that Response::content_length() is not derived from a Content-Length header in docs by @babolivier in seanmonstar/reqwest#2588 docs: link to char::REPLACEMENT_CHARACTER by @marcospb19 in seanmonstar/reqwest#1880 feat: add H3 client config support by @smalls0098 in seanmonstar/reqwest#2609 chore: update brotli to v7 by @nyurik in seanmonstar/reqwest#2620 Do not pull in an entirely different DEFLATE implementation just for tests by @Shnatsel in seanmonstar/reqwest#2625 chore: fix some typos in comment by @xixishidibei in seanmonstar/reqwest#2628 fix(wasm): handle null body in bytes_stream by @alongubkin in seanmonstar/reqwest#2632 ClientBuilder::interface on macOS/Solarish OSes by @hawkw in seanmonstar/reqwest#2623 ci: use ubuntu-latest in nightly job by @seanmonstar in seanmonstar/reqwest#2646 feat: BBR congestion control for http3 by @threeninesixseven in seanmonstar/reqwest#2642 feat: Add extentions for Request by @Xuanwo in seanmonstar/reqwest#2647 refactor: Store request timeout in request extensions instead by @Xuanwo in seanmonstar/reqwest#2650 chore: make ci pass by @linyihai in seanmonstar/reqwest#2666 update h3 dependencys by @Ruben2424 in seanmonstar/reqwest#2670 Document reqwest can make TLS and cookie requests with Wasm by @nickbabcock in seanmonstar/reqwest#2661 fix(redirect): make the number of redirects of policy matches its maximum limit. by @linyihai in seanmonstar/reqwest#2664 Exposed hyper tcp keepalive interval and retries parameters by @mackliet in seanmonstar/reqwest#2675 refactor: use hyper-util's proxy::Matcher by @seanmonstar in seanmonstar/reqwest#2681 Support streaming request body in HTTP/3 by @ducaale in seanmonstar/reqwest#2673 refactor: use hyper-util Tunnel by @seanmonstar in seanmonstar/reqwest#2684 Upgrade webpki-roots to 1 by @djc in seanmonstar/reqwest#2688 refactor: remove futures-util unless using stream/multipart/compression/blocking by @paolobarbolini in seanmonstar/reqwest#2692 chore: replace rustls-pemfile with rustls-pki-types by @tottoto in seanmonstar/reqwest#2541 Ensure H3ResponseFuture Implements Sync by @ducaale in seanmonstar/reqwest#2685 feat(redirect): Using FollowRedirect from tower-http to handle the redirect loop by @linyihai in seanmonstar/reqwest#2617 feat: add customizable headers in proxy mode by @chanbengz in seanmonstar/reqwest#2600 Prepare v0.12.16 by @seanmonstar in seanmonstar/reqwest#2694 New Contributors @babolivier made their first contribution in seanmonstar/reqwest#2588 @marcospb19 made their first contribution in seanmonstar/reqwest#1880 @smalls0098 made their first contribution in seanmonstar/reqwest#2609 @Shnatsel made their first contribution in seanmonstar/reqwest#2625 @xixishidibei made their first contribution in seanmonstar/reqwest#2628 @alongubkin made their first contribution in seanmonstar/reqwest#2632 ... (truncated) Changelog Sourced from reqwest's changelog. v0.12.16 Add ClientBuilder::http3_congestion_bbr() to enable BBR congestion control. Add ClientBuilder::http3_send_grease() to configure whether to send use QUIC grease. Add ClientBuilder::http3_max_field_section_size() to configure the maximum response headers. Add ClientBuilder::tcp_keepalive_interval() to configure TCP probe interval. Add ClientBuilder::tcp_keepalive_retries() to configure TCP probe count. Add Proxy::headers() to add extra headers that should be sent to a proxy. Fix redirect::Policy::limit() which had an off-by-1 error, allowing 1 more redirect than specified. Fix HTTP/3 to support streaming request bodies. (wasm) Fix null bodies when calling Response::bytes_stream(). Commits 99259cb v0.12.16 57670ac feat: add customizable headers for reqwest::Proxy (#2600) d9cf60e refactor: Using FollowRedirect from tower-http to handle the redirect l... 75f62f2 fix: ensure H3ResponseFuture is sync (#2685) 0e1d188 chore: replace rustls-pemfile with rustls-pki-types (#2541) 705b613 refactor: remove futures-util unless using stream/multipart/compression... 7b80718 Upgrade webpki-roots to 1 (#2688) 152a560 refactor: use hyper-util Tunnel (#2684) df09c9e feat: support streaming request body in HTTP/3 (#2673) 4ec1fe5 refactor: use hyper-util's proxy::Matcher (#2681) Additional commits viewable 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)
Something that's bitten me a few times (and that I see others get bitten by fairly often) is that the current documentation of
Response::content_length()can lead users to believe its return value is a representation of theContent-Lengthheader in responses. While the return value of this method and that header's value might happen to match in quite a few cases, it makes things very confusing when they don't, e.g. inHEADrequests.This change updates the method's documentation (in both the blocking and async implementations) to clarify that its return value is calculated independently from the value of any
Content-Lengthheader, and directs users to useResponse::headers()instead. It also removes the line about a lack ofContent-Lengthheader causing the method to "fail", since this doesn't match the implementation.