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: avoid setting content-length before middleware #2897

Merged
merged 5 commits into from
Oct 6, 2024

Conversation

SabrinaJewson
Copy link
Contributor

Motivation

Axum currently sets content-length automatically inside Route. However, if this occurs before middleware is run then should the middleware add a body to the request, Axum will avoid overwriting the content-length and so the user is stuck with an incorrect content-length, leading to panics in Hyper.

Solution

Only set content-length for top-level Routes.

(there are a couple miscellaneous changes as well; one is a one-line change in route.rs to avoid double-boxing the body for middleware, another removes the unused enum RouteFutureKind)

Axum currently sets content-length automatically inside Route. However,
if this occurs before middleware is run then should the middleware add a
body to the request, Axum will avoid overwriting the content-length and
so the user is stuck with an incorrect content-length, leading to
panics in Hyper.
Copy link
Member

@jplatte jplatte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't dive into the details here, but nothing stands out as wrong and I'm pretty sure we have sufficient tests to cover this change (especially with you adding another one for the previous bug).

@jplatte
Copy link
Member

jplatte commented Oct 6, 2024

Do you want to add a changelog entry though?

@SabrinaJewson
Copy link
Contributor Author

Yup, good point – added.

@jplatte jplatte merged commit 5512b5b into tokio-rs:main Oct 6, 2024
18 checks passed
@SabrinaJewson SabrinaJewson deleted the content-length-fix branch October 6, 2024 20:25
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.

3 participants