Expose ignore_eof option for Net::HTTP (or set to it to false by default?)

[adamzapasnik]

Hey guys,
In 3.1.something ignore_eof option was added to Net::HTTP ruby/net-http#15 to fix a bug where EOFError gets swallowed: https://bugs.ruby-lang.org/issues/14972

I modified the code from the bug report a bit to provide a reproducible env. In the client code, there are 3 clients:

1. most recent/default faraday-net_http adapter that swallows an error
2. most recent faraday-exconadapter which raises an error
3. and monkey-patched faraday-net_http adapter that raises an error

Imo, this option should be set by default to false as it's probably the right thing to do. Especially that other adapter(s), at least excon, has a proper behaviour. But maybe setting this option to false by default is a breaking change that would require a major version release? If so, I'd propose to make the option configurable via env.

pseudo server code

require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'
  gem 'webrick' # I'm not sure if it's installed by default anymore
end

server = WEBrick::HTTPServer.new :Port => 6000
trap 'INT' do
  server.shutdown
end

server.mount_proc '/' do |req, res|
  res.status = 200
  res['Content-Type'] = 'text/plain'

  str = "0123456789"
  res.body = str
  res['Content-Length'] = str.length + 1
end

server.start

client code

require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'
  gem 'faraday', '~> 2.0'
  gem 'faraday-excon', '~> 2.0'
  gem 'faraday-net_http', '~> 3.0'
end


uri = URI("http://localhost:6000/")

# most recent net http, swallows an error
streamed = []
conn = Faraday.new(uri)
conn.get('/') do |req|
  req.options.on_data = Proc.new do |chunk, overall_received_bytes, env|
    puts "Received #{overall_received_bytes} characters"
    streamed << chunk
  end
end
streamed.join

# ------

# most recent excon, raises an error (Faraday::ConnectionFailed)
# uncomment this and comment out earlier net http version

# streamed = []
# conn = Faraday.new(uri) do |f|
#   f.adapter :excon
# end
# conn.get('/') do |req|
#   req.options.on_data = Proc.new do |chunk, overall_received_bytes, env|
#     puts "Received #{overall_received_bytes} characters"
#     streamed << chunk
#   end
# end
# streamed.join

# -----

# a patched version, raises an error (Faraday::ConnectionFailed)
# uncomment this and comment out earlier excon version

# module AdapterPatch

#   def build_connection(env)
#     http = super
#     http.ignore_eof = false
#     http
#   end
# end
# Faraday::Adapter::NetHttp.prepend(AdapterPatch)

# streamed = []
# conn = Faraday.new(uri)
# conn.get('/') do |req|
#   req.options.on_data = Proc.new do |chunk, overall_received_bytes, env|
#     puts "Received #{overall_received_bytes} characters"
#     streamed << chunk
#   end
# end
# streamed.join

[iMacTia]

Hi @adamzapasnik and thank you for the detailed issue submission!
I took some time to look into this new option, and I see why you suggest making it true by default.

However, as Faraday is mostly a "plumbing gem" that other libraries rely on, making this change would effectively be considered "breaking", which is also why the net_http team decided to default this to true.
Now, I can see the argument about other adapters behaving differently, but that would only make sense if we wanted this to be some sort of "officially supported" feature from Faraday which would require testing and fixing all other adapters as well.

To be honest, I'm not sure this is actually worth it as it seems like a very minor feature if it was ignored by even the out-of-the-box net_http until now, and I'm not sure we fully understand the possible implications of changing the default.

That said, let me clarify one thing, you don't really need to patch Faraday if you want to opt-in: as shown in the usage section of the README you can set that config when adding the adapter to your connection and it will apply to all the calls made through that connection.

[adamzapasnik]

thanks @iMacTia, didn't noticed that configuring a connection is so easy :)