Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Empty Response with Faraday 2.5.1 #14

Closed
MrSerth opened this issue Aug 9, 2022 · 19 comments · Fixed by #15
Closed

Empty Response with Faraday 2.5.1 #14

MrSerth opened this issue Aug 9, 2022 · 19 comments · Fixed by #15

Comments

@MrSerth
Copy link

MrSerth commented Aug 9, 2022

Basic Info

  • Faraday Version: 2.5.1
  • Net::HTTP::Persistent Version: 2.0.1
  • Ruby Version: 3.1.2

Issue description

In our project, we use Faraday with the Net::HTTP::Persistent adapter. Using Faraday 2.4.0 works fine, however, using Faraday 2.5.1 breaks our application.

Probably, this adapter is not yet compatible with the new streaming API introduced in Faraday 2.5.0?

Steps to reproduce

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
    source "https://rubygems.org"

    git_source(:github) { |repo| "https://github.com/#{repo}.git" }

    gem "rspec"
    gem "faraday"
    gem "faraday-net_http_persistent"
end

require "rspec/autorun"

class Runner
    def self.check_status
        response = http_connection.get 'https://github.com'
        return response.status
    end

    def self.http_connection
        @http_connection ||= Faraday.new do |faraday|
            faraday.adapter :net_http_persistent
        end
    end
end

describe Runner do
    let(:action) { -> { described_class.check_status } }


    it 'works as expected' do
        expect(action.call).to eq(200)
    end
end

Executing the file with ruby <filename>.rb will render an error with the latest Faraday version:

Failures:

  1) Runner works as expected
     Failure/Error: expect(action.call).to eq(200)
     
       expected: 200
            got: nil
     
       (compared using ==)
     # faraday_bug.rb:35:in `block (2 levels) in <main>'

Finished in 0.45594 seconds (files took 0.10409 seconds to load)
1 example, 1 failure

When using a previous version (change gem "faraday" to gem "faraday", '~> 2.4.0' in the example above), the test will succeed. Additionally, switching the adapter to :net_http (instead of :net_http_persistent) will also result in a successful test execution.

@mattbrictson
Copy link
Contributor

I'm pinning faraday-net_http as a quick workaround:

gem "faraday-net_http", "< 3.0.0" # v3 currently breaks faraday-net_http_persistent

@iMacTia
Copy link
Member

iMacTia commented Aug 9, 2022

Thank you for reporting this @MrSerth, indeed we should have pinned faraday-net_http as @mattbrictson has done.
I'll do that now and release a fix 👍

@iMacTia iMacTia closed this as completed in 0fb45aa Aug 9, 2022
@iMacTia
Copy link
Member

iMacTia commented Aug 9, 2022

v2.0.2 is on its way 🎉

@MrSerth @mattbrictson I'd appreciate if you could give it a try and let me know if that solves the problem (after removing any pinning you might have added for faraday-net_http)

@mattbrictson
Copy link
Contributor

@iMacTia Short answer, yes this works and I am all set.

Long answer is that it is a little sensitive. If you have faraday-net_http 3.0.0 already installed (e.g. from an attempted upgrade that caused you to run into this bug), it seems like bundler's resolution algorithm will prefer it, so then faraday-net_http_persistent remains stuck at 2.0.1.

In that situation, if I try to force bundler to resolve 2.0.2 like this:

gem "faraday-net_http_persistent", ">= 2.0.2"

Then I get an error:

$ bundle up
Fetching gem metadata from https://rubygems.org/........
Resolving dependencies...
Bundler could not find compatible versions for gem "faraday-net_http":
  In snapshot (Gemfile.lock):
    faraday-net_http (= 3.0.0)

  In Gemfile:
    faraday was resolved to 2.5.1, which depends on
      faraday-net_http (>= 2.0, < 3.1)

    faraday-net_http_persistent (>= 2.0.2) was resolved to 2.0.2, which depends on
      faraday-net_http (< 3)

Deleting your Gemfile.lock file and running `bundle install` will rebuild your snapshot from scratch, using only
the gems in your Gemfile, which may resolve the conflict.

In that case deleting the Gemfile.lock does work, but it is a pretty drastic solution.

However if uninstall all versions of all faraday gems and create a fresh Gemfile like this:

source "https://rubygems.org"

# gem "rails"
gem "faraday"
gem "faraday-net_http_persistent"

Then it does work as expected:

$ bundle
Using bundler 2.3.19
Using connection_pool 2.2.5
Using faraday-net_http 2.1.0
Using ruby2_keywords 0.0.5
Using faraday 2.5.1
Using net-http-persistent 4.0.1
Using faraday-net_http_persistent 2.0.2

So I think people who upgraded to faraday-net_http 3.0.0 in the short window before faraday-net_http_persistent 2.0.2 was released might have some problems, but long term the issue is resolved. People that have problems can resolve it by temporarily adding gem "faraday-net_http", "< 3.0.0" to the Gemfile to force bundler to resolve properly, after which they can remove it.

@MrSerth
Copy link
Author

MrSerth commented Aug 9, 2022

Thanks for releasing a new version and pinning faraday-net_http for now, @iMacTia! I successfully fetched the new gem version (as well as the newest Faraday version) and can confirm that this fixes the issue I described here.

@iMacTia
Copy link
Member

iMacTia commented Aug 9, 2022

Thank you both for confirming, at least this should stop the bleeding.
I still have the other issue with the order in the Gemfile to fix and I'll try to look into this as well.

The biggest issue here is that this adapter heavily relies on the net-http one even though it is a different library, and this is getting things hard to maintain now that they're in separate gems.

The best course of action would probably be to remove the dependency from faraday-net_http and that would allow me to fix both issues at once 👍

@mattbrictson
Copy link
Contributor

Great! Let me know if there is anything I can do to help, whether it is testing, PR reviews, etc.

@rsl
Copy link

rsl commented Aug 10, 2022

people who upgraded to faraday-net_http 3.0.0 in the short window before faraday-net_http_persistent 2.0.2 was released might have some problems

confirmed ;)

wrestling this out and it's pretty hard in our case where our Ruby apps are pulling this gem via a private gem. i can get those gems installing the correct things but when i get to the full apps... the bundler resolution process pulls me back into the badlands of 2.0.1. if i nail down the version in the gemspec, i get the same errors detailed in #14 (comment). bundling works fine but when i require the gem, Unable to activate faraday-net_http_persistent-2.0.2, because faraday-net_http-3.0.0 conflicts with faraday-net_http (< 3) (Gem::ConflictError)

@rsl
Copy link

rsl commented Aug 10, 2022

for anyone else stumbling here, #14 (comment) seems to be working.

@iMacTia
Copy link
Member

iMacTia commented Aug 10, 2022

@mattbrictson @rsl I've just opened a PR to address this together with #13, I'd really appreciate if you could either:

  • Review the PR
  • Test the PR in your application without pinning faraday-net_http version yourself: the new code should work even with faraday-net_http 3.0 in your bundle 👍

@rsl
Copy link

rsl commented Aug 10, 2022

@iMacTia on it!

@rsl
Copy link

rsl commented Aug 10, 2022

@iMacTia not sure i can review some of that internals in lib/faraday/adapter/net_http_persistent.rb but the other code looks good. i made a small app and used gem 'faraday-net_http_persistent', github: 'lostisland/faraday-net_http_persistent', branch: 'remove-net-http-adapeter-dependency' [no direct faraday call even] and it makes the connection.

i noticed the version number is the same as main right now tho.

irb(main):002:0> Faraday::NetHttpPersistent::VERSION
=> "2.0.2"

i just ran this to verify it's running the correct version [out of curiosity]. the gemfile.lock does back it up tho.

GIT
  remote: https://github.com/lostisland/faraday-net_http_persistent.git
  revision: eb79a61225eb8e38a1a2695381d1ca0118436023
  branch: remove-net-http-adapeter-dependency
  specs:
    faraday-net_http_persistent (2.0.2)
      faraday (~> 2.5)
      net-http-persistent (~> 4.0)

@MrSerth
Copy link
Author

MrSerth commented Aug 10, 2022

I also tested the changes you pushed in our application, @iMacTia. With that development version, all tests pass as expected, the (implicitly required) gem faraday-net_http can be upgraded to v3.0.0, and otherwise the code behaves as expected. Thanks a lot for continuously working on a long-term fix! 👍

@iMacTia
Copy link
Member

iMacTia commented Aug 11, 2022

Thank you both! I'll merge the PR and release as 2.1 shortly 🎉

@mattbrictson
Copy link
Contributor

I'm using 2.1.0 in production now and it is working great so far. Thanks!

@rsl
Copy link

rsl commented Aug 11, 2022

i'm jealous. discovered that zendesk_api is keeping us back on some 1.x Faraday. no response from the developers this year on updating or anything. feels like we're going to be ditching that gem and just making a light wrapper for using their REST API instead. but not today. great work on solving this tho!

@iMacTia
Copy link
Member

iMacTia commented Aug 11, 2022

Are they responsive at all? I could try opening a PR to upgrade to Faraday 2.0

@rsl
Copy link

rsl commented Aug 11, 2022

i haven't seen them comment on any of the threads/issues about faraday versioning. i tried making a fork and updating but i think someone would have to know more about middleware [and the changes in the architecture] than i currently do.

@iMacTia
Copy link
Member

iMacTia commented Aug 12, 2022

Oh yeah, I can see this one has been waiting 3 months already 😨!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants