body: Introduce Bodies of Unknown Size#3742
body: Introduce Bodies of Unknown Size#3742lorenzleutgeb wants to merge 1 commit intotokio-rs:mainfrom
Conversation
lorenzleutgeb
force-pushed
the
body-unknown
branch
from
455bf25 to
3525293
Compare
|
While this works to resolve the issue I faced when reporting #3741, I think this is conceptually not perfectly clean. Consider the definition of
While the value returned from I lack knowledge about axum to judge how much of a problem this might be. |
I think that's fine. It's like Iterator's size hint, which is allowed to be wrong and shouldn't be relied on for soundness. |
|
I am adding two tests. |
lorenzleutgeb
force-pushed
the
body-unknown
branch
2 times, most recently
from
95dc10b to
fa82c3b
Compare
| impl IntoResponse for () { | ||
| fn into_response(self) -> Response { | ||
| Body::empty().into_response() | ||
| Body::unknown().into_response() |
There was a problem hiding this comment.
@davidpdrsn please note this change in particular. Tests are passing and it seems to achieve what I want, but for an outsider like me it is hard to judge whether this actually is a sane change.
What is difficult for me to assess is the intention behind the semantics of converting () into a response. Up to now, it meant an empty body, i.e. one of known size zero.
This seems to have been introduced in 18f613f#diff-6f9f8f882acd334c5bf0b598b309afef5f90f63d20b4cc6c26751d66bcb1dba6R19 with a slight change from ::default to ::empty in a005427#diff-6f9f8f882acd334c5bf0b598b309afef5f90f63d20b4cc6c26751d66bcb1dba6L18-R15
If it is intentional that () should convert to the empty body, then my suggestion here is probably bad and should be rejected.
If however it is accidental that () converts to the empty body, and it actually fits better to convert to a body of unknown size, then maybe I am on the right track.
There was a problem hiding this comment.
As a user, I think this would be very confusing for the typical user to see, and thus probably deserves a comment?
lorenzleutgeb
force-pushed
the
body-unknown
branch
from
fa82c3b to
378f9d4
Compare
davidpdrsn
left a comment
davidpdrsn
left a comment
There was a problem hiding this comment.
I've come around! Since it doesn't impact GET responses with () I think this solution is good.
One small comment about docs but otherwise LGTM!
Would you mind adding a line in axum and axum-core's changelogs as well?
| #[crate::test] | ||
| async fn unit_content_length_zero() { | ||
| let content_length: HeaderValue = HeaderValue::from_static("0"); | ||
|
|
||
| async fn handler() -> Response { | ||
| ().into_response() | ||
| } | ||
|
|
||
| let client = TestClient::new(Router::new().route("/", get(handler))); | ||
|
|
||
| let get = client.get("/").await; | ||
| assert_eq!(get.status(), http::StatusCode::OK); | ||
| assert_eq!(get.headers().get(CONTENT_LENGTH), Some(&content_length)); | ||
| } |
There was a problem hiding this comment.
Okay I think I see what's going on now and why this test passes:
Unknown::is_end_streamis true, which hyper detects and treats as no body (None):
https://github.com/hyperium/hyper/blob/v1.8.1/src/proto/h1/dispatch.rs#L339-L349- Later, for
None | Some(BodyLength::Known(0)), hyper calls
Server::can_have_implicit_zero_content_length. For this GET response it returns true,
so hyper writescontent-length: 0:
https://github.com/hyperium/hyper/blob/v1.8.1/src/proto/h1/role.rs#L907-L916
There was a problem hiding this comment.
Makes sense. Thanks for the explanation!
There was a problem hiding this comment.
Do you think putting some of this explanation as a comment would help future travellers?
lorenzleutgeb
force-pushed
the
body-unknown
branch
from
378f9d4 to
ae074a4
Compare
I did that in f65a1407 but am not sure I managed to match your style of changelogs well enough. Happy to work on it with some more feedback. |
lorenzleutgeb
force-pushed
the
body-unknown
branch
from
ae074a4 to
f65a140
Compare
As reported in <tokio-rs#3741>, in some responses to HEAD requests, the header `content-type: 0` would be included, even though the size of the body was not known and also not zero. The reason for this was that `axum::body::Body::empty`, which represents a body that is *known to be empty* was used, even though *nothing was known about the body*. To remedy, introduce a new utility type `axum::body::Unknown`, which represents an unknown body. This type could be considered for upstreaming to the `http-body-util` crate in the future. Fixes: tokio-rs#3741
lorenzleutgeb
force-pushed
the
body-unknown
branch
from
f65a140 to
f13cc1b
Compare
3 participants
As reported in #3741, in some responses to HEAD requests, the header
content-type: 0would be included, even though the size of the body was not known and also not zero.The reason for this was that
axum::body::Body::empty, which represents a body that is known to be empty was used, even though nothing was known about the body.To remedy, introduce a new utility type
axum::body::Unknown, which represents an unknown body. This type could be considered for upstreaming to thehttp-body-utilcrate in the future.