error reading body_io of a zero length http client response

[rdp]

Here's the example:

$ crystal -v
Using compiled compiler at `.build/crystal'
Crystal 0.32.0-dev [d1c1f5a9e] (2019-11-11)

LLVM: 8.0.0
Default target: x86_64-pc-linux-gnu

$ cat bad2.cr 
require "http/client"

HTTP::Client.get("http://bing.com") do |response|
  puts response.status_code  # => 301
  response.body_io
end

$ crystal bad2.cr 
Using compiled compiler at `.build/crystal'
301
Unhandled exception: HTTP::Client::Response#body_io cannot be nil (NilAssertionError)
  from dev/crystal/src/http/client/response.cr:10:3 in 'body_io'
  from bad2.cr:5:8 in '__crystal_main'
  from dev/crystal/src/crystal/main.cr:97:5 in 'main_user_code'
  from dev/crystal/src/crystal/main.cr:86:7 in 'main'
  from dev/crystal/src/crystal/main.cr:106:3 in 'main'
  from __libc_start_main
  from _start
  from ???

would have expected the empty IO or something, so that the examples would still work?

[RX14]

This should probably raise a better error but I think this was how the API was designed.

[rdp]

Response#body is the empty string in this case. Not sure why body_io wouldn't similarly be functional but empty... ? :)

[asterite]

I think the issue is valid. The culprits are these lines:

crystal/src/http/common.cr

Lines 41 to 44 in 440cd67

	if content_length != 0
	# Don't create IO for Content-Length == 0
	body = FixedLengthContent.new(io, content_length)
	end

The response from bing.com contains Content-Length: 0 so I would expect an empty IO, not nil.

Maybe it's an optimization we thought would make sense, but it doesn't.

[straight-shoota]

What about the case body_type.prohibited? This is the only other case where body_io would be nil. It's only used for HEAD requests (or Response.from_io is manually called with ignore_body: true).
But when performing a HEAD request and accessing body_io, the same NilAssertionError will be raised. I think in this case, should also just return an empty IO instead of raising.

An alternative solution to #8503 could be do change Request#body_io to return an empty IO instead of raising when @body_io is nil (the empty IO can also be cached). This would completely avoid allocating an IO if it would be empty and is not accessed.

[asterite]

Yes, an empty IO might be good. Maybe just using FixedContentLength.new(io, 0) would be enough, I don't know. I feel like introducing yet another IO type is actually more complex, I don't know. Or maybe just returning IO::Memory over an empty slice?

I don't think creating FixedLengthContent with size zero for every request has a big impact on memory consumption or performance, so we could do that too.

[straight-shoota]

Yeah, I wasn't suggesting a new type, just an IO instance with no content. Returning IO::Memory directly should be fine. I don't think there is a reason to wrap it in FixedContentLength.

I don't think creating FixedLengthContent with size zero for every request has a big impact on memory consumption or performance, so we could do that too.

Probably not, but it's a cheap optimization, easy to implement. And nonetheless, it saves two heap allocations (or maybe just one, we could return an empty IO::Memory in parse_headers_and_body as well).

[asterite]

Let's do that. I'll update my PR later.

[asterite]

Actually, if someone wants to tackle this, please go ahead. I think my PR is fine. If you get a response in the non-block form then body_io should raise when you access it, but if you lazily instantiate it to IO::Memory then it won't raise. And from inside the response there's no way to know whether it was initialized with a block or not.

[straight-shoota]

Maybe this is simply caused by a flaw in the API. When simply accessing a property is expected to raise when the API is used in a valid manner, it sounds some alarm bells. Is this really the correct way we want our API to work?

An alternative solution would be to remove body_io from HTTP::Client::Response and pass it as a separate argument to the block. It's only used in the initializer and the undocumented method #consume_body_io. Both can be refactored.

It's also used in #to_io, but I'm not entirely sure about practical use cases for that when it's only piping the IO. Maybe just for proxying? I suppose it would be fine to require such use cases to handle serialization with the IO explicitly.

[asterite]

@straight-shoota Yes, proxying is a use case that should be supported. I don't think having to carry the IO separate from the response seems a bit more troublesome than the current approach.