fix: Body's reader is now optional; may be checked for emptiness

[mmstick]

SUMMARY:

Fixes the integration issue with isahc, whereby a isahc::Body created from
isahc::Body::readerhas a length of None, which signals to isahc::Client
to send a body with an unknown length. A length of None is interpreted as
being unknown, and therefore a body is sent with every request. This causes a
501 error on servers which do not permit a body in the request, such as GET
requests to Cloudflare CDNs.

CHANGES:

• Changed http_client::Body's reader field to being optional
• Added a http_client::Body::is_empty method to check for emptiness
• On creating a new request for isahc::Client:
  • Create a isahc::Body::empty if http_client::Body is empty
  • Create a isahc::Body::reader if otherwise not empty

[SirVer]

This looks like a reasonable change to me, though I cannot comment if there is ever a use case where somebody wants to send a non-empty body, but with zero length. I assume there is, since isahc decided to model essentially three things:

1. Body::empty
2. Body("")
3. Body(s) where s.len() > 0

Http before this change only allowed 2 and 3, after this change only allows 1. and 3. Is that acceptable?

[mmstick]

Actually, 2 is still permitted if you create a body with an empty string or array. It's only considered empty if you create a body with Body::empty().

[SirVer]

@mmstick Huh, it reads as if this code is eating that case: https://github.com/http-rs/http-client/pull/14/files#diff-5f9a78c705c3d5bd9ffa2d115d3085acR46

i.e. if your string is empty, it will be turned into Body::empty by http-client. Or am I misunderstanding the code?

[yoshuawuyts]

The bug we're trying to fix sounds like: "if we don't have a body, actually don't send a body". My understanding of the use cases we need to support:

1. empty body (aka "no body")
2. body with known length
3. body with no known length (chunk encoding)

I just merged #15 which allows discerning between the second and third cases. The current patch should probably be rebased on master so we can cover all cases.

Is this all accurate?

[mmstick]

@SirVer That would be a negative, because the impl for From<String> / From<Vec<u8>> calls Body::from_reader(), rather than Body::empty(), and Body::from_reader() always creates Some(<trait object>) rather than None. The Body::is_empty() only checks if the reader field is None, as it has no way of knowing that a trait object is technically empty.

[SirVer]

lgtm. Thanks for the explanations!

[yoshuawuyts]

http-rs/surf#117 (comment) might need to be applied, but we can do that in a follow-up patch. Overall this looks great, and we should merge!

[mmstick]

Change made