Persistence breaks streaming

[wconrad]

Basic Info

• Faraday Version: 1.5.0
• Ruby Version: ruby 3.0.1p64 (2021-04-05 revision 0fb782ee38) [x86_64-linux]
• net-http-persistent version: 4.0.1

Issue description

Persistence appears to breaks streaming. Specifically, If the net_http_persistent_adapter is used as in this example, then a streaming get as shown in this example does not call its on_data procedure.

Steps to reproduce

require "faraday"

connection = Faraday.new("https://google.com") do |f|
  f.adapter :net_http_persistent
end

connection.get("/") do |request|
  request.options.on_data = Proc.new do |chunk, overall_received_bytes|
    puts "Received #{overall_received_bytes} characters"
  end
end

The expected output is "Received 220 characters", but instead there is no output.

Additional info

Gist with scripts to reproduce the problem: https://gist.github.com/wconrad/c6885b2a6dd6494880964c3d6fb8e1f1

[iMacTia]

Hi @wconrad, unfortunately the net_http_persistent does not support streaming at the moment. The only adapters supporting it are net_http and excon, thought I noticed the documentation page you shared only mentions the former, so that will need updating.

Even though the net_http_persistent adapter extends the net_http one, the perform_request method is overriden, which is the method where streaming is implemented in net_http.

At this point, I'm a bit unsure if this is expected or not, because other adapters present a warning message where streaming is not supported, but it seems like net_http_persistent doesn't.

It might be worth attempting to add support in a PR, maybe it will work like with net_http, but if that's unsuccessful then we should be adding the warning like all other adapters.

P.S. Adapters have been moved into separate repos, so I'll be moving this issue on the correct one 👍

[MikeRogers0]

At this point, I'm a bit unsure if this is expected or not, because other adapters present a warning message where streaming is not supported, but it seems like net_http_persistent doesn't.

https://github.com/lostisland/faraday-em_synchrony/blob/ea5dd3f4ca0f5b60fc26eaeda044bf67e4f20ebb/lib/faraday/adapter/em_synchrony.rb#L94-L97

@iMacTia - Maybe the solution is something like below being added to ( https://github.com/lostisland/faraday-net_http_persistent/blob/main/lib/faraday/adapter/net_http_persistent.rb ):

def call(env)
  if env[:request].stream_response?
    warn "Streaming downloads for #{self.class.name} " 'are not yet implemented.'
  end

  super
end

Would work in the short term (Though I'm also happy to jump into supporting streaming)? :)

[iMacTia]

I think it would be perfect in the short term, unless adding streaming support is a really quick job. We already have a streaming tests feature available from Faraday, which could be easily enabled in this gem's test suite if you already have an implementation in mind.