hackney icon indicating copy to clipboard operation
hackney copied to clipboard

Flatten request body to prevent hackney adding unnecessary headers

Open pvsr opened this issue 10 months ago • 5 comments

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 avatar Jan 22 '25 03:01 pvsr

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

edit: https://github.com/benoitc/hackney/pull/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.

pvsr avatar Jan 22 '25 04:01 pvsr

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.

lpil avatar Jan 22 '25 08:01 lpil

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 avatar Jan 22 '25 20:01 pvsr

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.

pvsr avatar Jan 23 '25 00:01 pvsr

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.

lpil avatar Jan 23 '25 13:01 lpil