socketry / async-http

MIT License
298 stars 45 forks source link

Async::Http::Client makes chunked requests on array body #7

Closed janko closed 6 years ago

janko commented 6 years ago

When I use an array for the request body:

require "async"
require "async/http"
require "json"

Async::Reactor.run do |task|
  endpoint = Async::HTTP::URLEndpoint.parse("https://httpbin.org")
  client   = Async::HTTP::Client.new(endpoint)

  response = client.put("/put", {}, ["request body"])

  puts JSON.parse(response.read).fetch("headers")

  client.close
end

Async::HTTP::Client uses chunked request encoding, as shown by the output:

{"Connection"=>"close", "Host"=>"httpbin.org", "Transfer-Encoding"=>"chunked"}

This is because array request body is wrapped in Async::HTTP::Body::Buffered, but when you call #length on it returns nil, so Async::HTTP decides to use chunked encoding instead. This didn't work for me when I tried to use Aync::HTTP::Client to upload to AWS S3, because AWS S3 doesn't accept chunked requests for uploads.

I think #length should just be aliased to #bytesize in HTTP::Body::Buffered, I can make that change.

ioquatix commented 6 years ago

Yeah, this is probably a glitch in the matrix. I think I already made this change locally, but feel free to make a PR to change it as you suggested. Please add a spec for it too.

ioquatix commented 6 years ago

Just FYI, async-http and falcon are a huge surface area and by no means do I claim that everything works correctly :) There are bound to be more issues like this. My goal was to try and make something that stretches over the entire problem space in a logical and consistent way. Unfortunately HTTP is anything but consistent :p - I appreciate these kinds of bug reports.

ioquatix commented 6 years ago

N.B. I also removed the unnecessary async.task blocks from your example. They don't hurt anything but they are also irrelevant in your example.

janko commented 6 years ago

Just FYI, async-http and falcon are a huge surface area and by no means do I claim that everything works correctly :) There are bound to be more issues like this. My goal was to try and make something that stretches over the entire problem space in a logical and consistent way. Unfortunately HTTP is anything but consistent :p

No worries, I'm still very impressed how you were able to make HTTP::Client and HTTP::Server share the same classes, it's really nice how symmetrical that is.

I will definitely try to report any unexpected behaviour I find. However, I'm just finding myself with a lack of energy lately to actually fix some of the reported problems (probably too much maintenance of my own projects 😛). But I should be able to open at least simple fixes like this one.

N.B. I also removed the unnecessary async.task blocks from your example. They don't hurt anything but they are also irrelevant in your example.

Great, thanks, I do want the example to be as minimal as possible 👍

janko commented 6 years ago

@ioquatix Awesome, thanks a lot!

ioquatix commented 6 years ago

Thanks!

It was an interesting bug fix. Because it revealed another bug which is also a tricky design issue:

If you enable compression, you end up needing to use chunked encoding (or connection close), because you don't know the content length ahead of time. I ended up doing this:

https://github.com/socketry/async-http/blob/94bc5bc7b70cb52b5357ec2463ef10c8ff1ec3d7/lib/async/http/body/deflate.rb#L58-L61