Handle early responses

[janko]

HTTP servers typically wait until the whole request body has been received before sending the response. However, some servers might send out a response early, before they've read the whole request body. For example, they might detect the request headers were invalid, and they want to tell that to the client immediately.

For example, sending a large request to http://google.com will return an early 400 Bad Request response. In this case http.rb will raise an Errno::EPIPE exception (wrapped in a HTTP::ConnectionError), because it tried to write to the socket but it couldn't because the server closed its end. However, the server still returned a valid response, so it's not really erroneous behaviour (e.g. curl doesn't print any errors in the same situations).

# Before
HTTP.get("http://google.com", body: "a" * 1*1024*1024)
#~> Errno::EPIPE

# After
HTTP.get("http://google.com", body: "a" * 1*1024*1024)
#=> #<HTTP::Response 400 Bad Request ...>

This commit changes Request::Writer to stop writing when an Errno::EPIPE exception occurred.

[mikegee]

If we return nil or something in that rescue, Rubocop will be satisfied.

[janko]

Since #send_request is a "command" method, I feel a bit weird explicitly returning a value. I always considered adding a comment to be enough when swallowing errors.

[mikegee]

I agree with you, but I'm slightly concerned about disabling this particular cop project-wide. It could catch a real bug someday. Perhaps disabling it just the once with an inline comment is an agreeable compromise?

[janko]

Agreed, updated.

[ixti]

IMO this code is pretty self-explanatory and doesn't need comment.

[ixti]

IMO it worth to explicitly add nil at the end of rescue block and remove rubocop disable.

[ixti]

Oh. It was discussed previously... Mmm... I still think we should just return nil from the rescue. Also body can be shortened:

def send_request
  each_chunk { |chunk| write chunk }
rescue Errno::EPIPE
  # no more data
  nil
end

[ixti]

On one hand I think that Lint/HandleExceptions is pretty useless and enforces developer to write more than needed. On the other hand I prefer to see:

begin
  # ...
rescue
  nil
end

than

begin
  # ...
rescue
end

IMO it's more readable that way. Easier to understand that nil is result in case of error.

[ixti]

I know that one might argue that we don't require explicit nil in case of return if something, but it's a bit different - it's one-liner. And reading it is pretty straight-forward to me. And when we explicitly return from non-one-liner code we add nil to return:

if something
  # ...
  return nil # <-- explicit `nil` here looks better than bare `return`
end

[janko]

Agreed, updated.

[ixti]

LGTM. Please rebase on top of master though.

[janko]

@ixti Done.

[ixti]

Thanks!