HTTP::Parser::Error in case of closed connection

[tycooon]

> HTTP.post("https://api.sendgrid.com/v3/mail/send123", body: "d" * 10000)
=> #<HTTP::Response/1.1 404 Not Found {"Server"=>"nginx", "Date"=>"Fri, 14 Jun 2019 15:42:56 GMT", "Content-Type"=>"text/plain; charset=utf-8", "Content-Length"=>"19", "Connection"=>"close", "X-Content-Type-Options"=>"nosniff", "Access-Control-Allow-Origin"=>"https://sendgrid.api-docs.io", "Access-Control-Allow-Methods"=>"POST", "Access-Control-Allow-Headers"=>"Authorization, Content-Type, On-behalf-of, x-sg-elas-acl", "Access-Control-Max-Age"=>"600", "X-No-Cors-Reason"=>"https://sendgrid.com/docs/Classroom/Basics/API/cors.html"}>
> _.body.to_s
HTTP::ConnectionError: error reading from socket: Could not parse data entirely (1 != 0)
from /Users/tycooon/.rbenv/versions/2.6.2/lib/ruby/gems/2.6.0/gems/http-4.1.1/lib/http/response/parser.rb:14:in `<<'
Caused by HTTP::Parser::Error: Could not parse data entirely (1 != 0)
from /Users/tycooon/.rbenv/versions/2.6.2/lib/ruby/gems/2.6.0/gems/http-4.1.1/lib/http/response/parser.rb:14:in `<<'

It looks like Sendgrid server is closing connection in case we try to POST a lot of data to some unknown endpoint. In this case we are getting HTTP::Parser::Error which is not very cool. For example, Net::HTTP is just ignoring missing body:

> Net::HTTP.post(URI("https://api.sendgrid.com/v3/mail/send123"), "d" * 10000)
=> #<Net::HTTPNotFound 404 Not Found readbody=true>
> _.body
=> ""

RestClient behaves the same as Net::HTTP.

[ixti]

Although I see how that is not very cool. I don't think that swallowing errors and silently returning empty body is good idea... It's not client's responsibility to handle that kinds of errors IMO.

I mean if it's OK with body silently returning empty string in case of underlying error, then probably there's no value in that body anyway, and you can simply ignore it. Or you can catch that exception.

Also, if we would swallow connection close during body streaming, would we need to silently swallow connection errors when they happen earlier (like during http headers send)?...

[ixti]

What I mean is, let's say our server behaves like this:

# frozen_string_literal: true

require "socket"

server = TCPServer.new(8080)

while (socket = server.accept)
  req = socket.gets[%r{^GET /([^ ]+)}, 1]

  socket << "HTTP/1.1 404 Not Found\r\n"
  socket << "Content-Type: text/plain\r\n"
  socket << "Content-Length: 161\r\n"

  case req
  when "a"
    socket << "X-Half: Heade"
  when "b"
    socket << "\r\n"
    socket << "half-bo"
  when "c"
    socket << "\r\n"
  end

  socket.close
end

Technically I think we can make client ignore any of those errors and return empty string as a body, but is it correct? Or is it what majority of users will expect? I don't know.

Probably it's actually a task for a Session class that would handle redirects and cookies in future.

[tycooon]

Well, maybe we can add some new error class for such cases to make handling easier. Or maybe even some configuration. Right now users see some generic exception in case where other libs are “just working” and they are frustrated. I also didn’t mention that when using persistent connections, you are actually getting another error after initial request: the one saying you did not read the body of the previous request.

[tarcieri]

Right now users see some generic exception in case where other libs are “just working” and they are frustrated.

You named Net::HTTP and RestClient. RestClient is a Net::HTTP wrapper. Have you investigated how other clients which aren't based on Net::HTTP behave?

Either this is a bug in our parser, or it's a framing error in the response. If it's the latter, my personal inclination is that it remain an error. The problem with trying to make it "just work", as @ixti already noted, is that such behaviors could manifest as silent truncation or corruption of response bodies.

[tycooon]

HTTPClient also swallows the error. I am not sure that we should do that, but here are the actual problems that I see in the current behaviour:

1. The error raised is HTTP::ConnectionError which is hard to handle since it's raised in many different situations.
2. The error occurs when trying to read the response body (and it leaves that unread) so basically it breaks the persistent client.

[tarcieri]

Some sort of EOFError would probably be a better fit for this case.

HTTPClient also swallows the error

@tycooon any chance you can try to reproduce this with curl and see what it does?

It'd be interesting to look at some examples of the specific logic for swallowing these errors in other clients to see exactly what they're doing.

[tycooon]

Sure, it returns the error:

curl: (18) transfer closed with 19 bytes remaining to read

[tarcieri]

Okay, nice to get a sanity check on that. I'd argue that what Net::HTTP and HTTPClient are doing is wrong, but perhaps we could raise a more useful/specific exception.

@tycooon what if we added HTTP::EOFError as a subclass of HTTP::ConnectionError which can be used in this case to swallow this exception?