Change HeaderMap extractor to clone the headers

[davidpdrsn]

We often hear from users who are hit by Headers taken by other extractor errors because they used HeaderMap as an extractor combined with Form<T>, or some other extractor that happens to use the headers.

The reason RequestParts::take_headers exists in the first place is for performance reasons. But I think I've realized that it was probably a mistake because it causes issues like this. So I think we should change it!

This PR removes RequestParts::take_headers and changes the HeaderMap extractor to clone the headers. We could also consider other workarounds such as recording which extractor removed and which requires the headers, but I think this solution is the least surprising, and worth the small perf hit.

If someone is really concerned about cloning the headers you can still write an extractor that does request_parts.headers_mut().take() which would leave an empty HeaderMap in place. I don't think axum should provide such an extractor though --- then the problem is just gonna stay.

This PR doesn't touch "extensions already taken by another extractor" which is basically the same problem. I'm not very concerned about that since it feels more niche but probably worth looking into as well.

TODO

• Changelog
• Document the cloning

[jplatte]

I like the various code simplifications! I think we should be consistent and do the same thing for extensions, as well as adding some warnings about the mostly-unnecessary cloning (of headers you're not interested in) to the affected FromRequest impls.

[SabrinaJewson]

Would there be the possibility of having RequestParts use an Arc<HeaderMap>? Then the cloning would become cheap. I don't know how useful it is to be able to mutate headers during extraction.

[davidpdrsn]

Hm not sure if it's worth it since it's only the HeaderMap extractor that got more expensive. If you don't use that things should work the same.

[rich-murphey]

I believe most session implementations would depend on cookies or JWT obtained from the HeaderMap, and rarely if ever mutate the headers. As it stands, would this make every API call that uses sessions more expensive?

[davidpdrsn]

As it stands, would this make every API call that uses sessions more expensive?

Like I just wrote, no it won't.

Only using HeaderMap as an extractor is more expensive.

If you need a single header for a handler you should use TypedHeader, or write your own extractor if the headers crate doesn't have the header you need.

If you need a header in your own extractor you just call request_parts.headers() which returns &HeaderMap and is free.

[rich-murphey]

Thanks for the clarification!