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

HTTPClient is no longer maintained, switch to Faraday #2348

Open
mohamedhafez opened this issue Jan 11, 2021 · 9 comments
Open

HTTPClient is no longer maintained, switch to Faraday #2348

mohamedhafez opened this issue Jan 11, 2021 · 9 comments
Assignees
Labels
type: process A process-related concern. May include testing, release, or the like.

Comments

@mohamedhafez
Copy link

The generated libraries (or at least google-apis-calendar_v3) rely on HTTPClient, which hasn't had a release since 2016 and has a ton of ignored issues. Especially since it seems to re-implement the low level socket details of Net::HTTP, including establishing SSL connections, it's not really secure to be relying on something that's been abandoned.

It also uses Timeout.timeout in several places instead of using socket timeout options, which is inefficient and inherently unsafe (see https://www.mikeperham.com/2015/05/08/timeout-rubys-most-dangerous-api/ and http://blog.headius.com/2008/02/ruby-threadraise-threadkill-timeoutrb.html)

googleauth and signet already depend on Faraday, which is actively maintained. It would be great to switch over to that, even if just so as not to pull in an extra dependency, let alone an unmaintained one.

@mohamedhafez mohamedhafez changed the title HTTPClient is no longer supported, switch to Faraday HTTPClient is no longer maintained, switch to Faraday Jan 11, 2021
@yoshi-automation yoshi-automation added triage me I really want to be triaged. 🚨 This issue needs some love. labels Jan 12, 2021
@dazuma dazuma added type: process A process-related concern. May include testing, release, or the like. and removed 🚨 This issue needs some love. triage me I really want to be triaged. labels Jan 18, 2021
@dazuma
Copy link
Member

dazuma commented Jan 18, 2021

Yes, good call, we need to do this.

@mohamedhafez
Copy link
Author

mohamedhafez commented Jan 5, 2022

Just a little bump on this. I've been using a monkey patch that works seamlessly, except for some reason large file uploads if I remember correctly don't work correctly. Perhaps we could use the HTTPClient Faraday backend just for those, and the net-http Faraday backend for everything else, if nobody has the time to properly fix this?

@mohamedhafez
Copy link
Author

Just another bump on this as something that needs to be done, depending on abandonware that hasn't been touched since 2016 that handles establishing SSL connections is not secure at all

@matthewford
Copy link

Any update on this? @mohamedhafez could you share you're patch?

@mohamedhafez
Copy link
Author

mohamedhafez commented Jul 18, 2022

@matthewford this basic monkey patch has worked just fine for my needs for the last year and a half, a bit more work would need to be done on BaseService#new_client to support all the options.

module Google
  module Apis
    module Core
      class HttpCommand
        def execute_once(client)
          if body.respond_to?(:rewind)
            body_str = ""
            body.rewind
            size = body.size
            body.read(nil,body_str)
            raise "buffer and body aren't same size!\nsize: #{size}, body_str.bytesize: #{body_str.bytesize}\n\n#{body_str}" if size != body_str.bytesize
          elsif body.nil? || body.is_a?(String)
            body_str = body
          else
            raise "unexpected body class: #{body.class}"
          end

          begin
            logger.debug { sprintf('Sending HTTP %s %s', method, url) }
            request_header = header.dup
            apply_request_options(request_header)

            @http_res = client.run_request(method,
                                           url.to_s,
                                           body_str,
                                           request_header)

            logger.debug { @http_res.status }
            logger.debug { safe_response_representation @http_res }

            response = process_response(@http_res.status.to_i, @http_res.headers, @http_res.body)
            success(response)
          rescue => e
            logger.debug { sprintf('Caught error %s', e) }
            error(e, rethrow: true)
          end
        end

        def process_response(status, header, body)
          check_status(status, header, body)

          content_type = header['Content-Type']
          content_type = content_type.first unless content_type.is_a?(String)

          decode_response_body(content_type, body)
        end

        def error(err, rethrow: false, &block)
          logger.debug { sprintf('Error - %s', PP.pp(err, '')) }
          if err.is_a?(Faraday::Error)
            err = Google::Apis::TransmissionError.new(err)
          end
          block.call(nil, err) if block_given?
          fail err if rethrow || block.nil?
        end
      end

      class BaseService
        def new_client
          Faraday.new
        end
      end
    end
  end
end

@catlee
Copy link

catlee commented Jul 22, 2022

I've been working on a PR to switch over to Faraday here: #11087

One of the main challenges I've had with the tests are handling interrupted responses with redirects. Faraday doesn't provide a good (any?) mechanism to capture partial response content while also handling redirects.

I've reported this issue to Faraday here: lostisland/faraday#1426

@iMacTia
Copy link

iMacTia commented Aug 8, 2022

Thank you for considering faraday for this!
Following @catlee's input, we've just released v2.5.1 which supports a new API to access response codes while streaming responses 👍
I hope that helps unblock the migration, but feel free to reach out if you need anything else!

@runephilosof-karnovgroup

@dazuma Isn't this a security issue? Maybe add a priority label.

Also, we know it is a next major: breaking change

@hbontempo-cw
Copy link

Just adding information here:

I received a warning today when installing a projects dependencies:

/Users/xxx/.asdf/installs/ruby/3.3.4/lib/ruby/gems/3.3.0/gems/httpclient-2.8.3/lib/httpclient/auth.rb:11: warning: mutex_m was loaded from the standard library, but will no longer be part of the default gems since Ruby 3.4.0. Add mutex_m to your Gemfile or gemspec. Also contact author of httpclient-2.8.3 to add mutex_m into its gemspec.

Checking my Gemfile.lock I saw this dependency comes from Google's gem:

    google-apis-core (0.15.0)
      addressable (~> 2.5, >= 2.5.1)
      googleauth (~> 1.9)
      httpclient (>= 2.8.1, < 3.a)
      mini_mime (~> 1.0)
      representable (~> 3.0)
      retriable (>= 2.0, < 4.a)
      rexml

This needs to be at least addressed (migrate to Faraday or dependency added explicitly) or this gem will be incompatible with ruby 3.4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: process A process-related concern. May include testing, release, or the like.
Projects
None yet
Development

No branches or pull requests

8 participants