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

Flatten request body to prevent hackney adding unnecessary headers #6

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pvsr
Copy link

@pvsr pvsr commented Jan 22, 2025

Fixes #5.

Hackney sees a body of <<>> as empty and doesn't set any extra headers for it. But it sees [<<>>] as a non-empty body that happens to have a length of zero, so it gets a content type of application/octet-stream and a content length of 0 (edit: this is intended for POST/PUT requests only). This PR flattens the body into a single binary so that hackney can detect empty bodies correctly.

Unfortunately this isn't practical to unit test, but I can confirm that it fixes the 500 errors I saw in #5.

@pvsr pvsr marked this pull request as ready for review January 22, 2025 03:45
@pvsr
Copy link
Author

pvsr commented Jan 22, 2025

Actually, this could possibly be considered a bug in hackney itself. I'll send a PR upstream and see how it turns out.

edit: benoitc/hackney#750. It is a bug in hackney imo, albeit an edge case. But this PR is still worth merging to work better with current releases of hackney.

@lpil
Copy link
Member

lpil commented Jan 22, 2025

This negatively impacts performance and makes use of the iolist type all overhead rather than a performance optimisation, so it's not a suitable fix I'm afraid. It would need to be only done for bodiless methods.

@pvsr
Copy link
Author

pvsr commented Jan 22, 2025

When we pass in an iolist hackney does the iolist_to_binary conversion itself, so I believe the net performance is the same. I'll confirm that and cite the specific code involved when I'm at my home computer later.

@pvsr
Copy link
Author

pvsr commented Jan 23, 2025

With an iolist request body we get to this case statement in hackney_request.erl which handles iolists with:

  _ when is_list(Body0) -> % iolist case
    Body1 = iolist_to_binary(Body0),
    S = erlang:byte_size(Body1),
    CT = hackney_headers_new:get_value(
           <<"content-type">>, Headers, <<"application/octet-stream">>
          ),
    {S, CT, Body1};

By calling iolist_to_binary we instead take the binary case, which is identical but without the conversion. Either way iolist_to_binary is only called once. And in the empty body case we instead hit this empty body check earlier on and skip the iolist_to_binary call and the unnecessary header work we're doing now. So to my understanding performance should be as good or better in all cases with this PR.

@lpil
Copy link
Member

lpil commented Jan 23, 2025

Hackney performing unnecessary work is not an argument in favour of us also performing unnecessary work.

And in the empty body case we instead hit this empty body check earlier on and skip the iolist_to_binary call and the unnecessary header work we're doing now. So to my understanding performance should be as good or better in all cases with this PR.

The code you have linked also matches the empty iolist that we send.

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.

Unnecessary Content-Type and Content-Length headers are included with GET requests
2 participants