async-http-faraday adapter is not threadsafe

[tmertens]

When a faraday client is cached in a global or module/class-level singleton variable such that the same client instance may be used across multiple threads, then a concurrent request between threads to the same endpoint may result in a FiberError due to how async-http-faraday caches and reuses clients within the faraday middleware.

This can be demonstrated by adding the following test case to the adapter_spec.rb:

  it 'is threadsafe' do
    connection = Faraday.new(endpoint.url) do |faraday|
      faraday.response :logger
      faraday.adapter :async_http
    end

    delay = 0.1

    run_server(response_delay: delay) do
      expect do
        5.times.map do
          Thread.new { connection.get('/index') }
        end.map(&:value)
      end.not_to raise_error(FiberError)
    end
  end

Because the same Faraday instance is used across multiple threads, and the adapter caches @clients for reuse, the same @clients instance within the adapter may be reused by more than one thread. As a result, when this code runs one or more of these threads may raise FiberError: fiber called across threads while attempting to acquire a resource from the client pool:

     1.1) Failure/Error:
            expect do
              5.times.map do
                Thread.new { connection.get('/index') }
              end.map(&:value)
            end.not_to raise_error(FiberError)
          
            expected no FiberError, got #<FiberError: fiber called across threads> with backtrace:
              # ~/.rvm/gems/ruby-2.7.8/gems/async-1.31.0/lib/async/semaphore.rb:111:in `resume'
              # ~/.rvm/gems/ruby-2.7.8/gems/async-1.31.0/lib/async/semaphore.rb:111:in `release'
              # ~/.rvm/gems/ruby-2.7.8/gems/async-1.31.0/lib/async/semaphore.rb:101:in `acquire'
              # ~/.rvm/gems/ruby-2.7.8/gems/async-pool-0.4.0/lib/async/pool/controller.rb:290:in `available_resource'
              # ~/.rvm/gems/ruby-2.7.8/gems/async-pool-0.4.0/lib/async/pool/controller.rb:254:in `wait_for_resource'
              # ~/.rvm/gems/ruby-2.7.8/gems/async-pool-0.4.0/lib/async/pool/controller.rb:95:in `acquire'
              # ~/.rvm/gems/ruby-2.7.8/gems/async-http-0.60.2/lib/async/http/client.rb:90:in `call'
              # ./lib/async/http/faraday/adapter.rb:114:in `block (2 levels) in call'
              # ./lib/async/http/faraday/adapter.rb:137:in `with_timeout'
              # ./lib/async/http/faraday/adapter.rb:113:in `block in call'
              # ~/.rvm/gems/ruby-2.7.8/gems/async-1.31.0/lib/async/task.rb:261:in `block in make_fiber'
          # ./spec/async/http/faraday/adapter_spec.rb:116:in `block (3 levels) in <top (required)>'
          # ./spec/async/http/faraday/adapter_spec.rb:38:in `block in run_server'
          # ~/.rvm/gems/ruby-2.7.8/gems/async-1.31.0/lib/kernel/sync.rb:29:in `Sync'
          # ./spec/async/http/faraday/adapter_spec.rb:24:in `run_server'
          # ./spec/async/http/faraday/adapter_spec.rb:115:in `block (2 levels) in <top (required)>'
          # ~/.rvm/gems/ruby-2.7.8/gems/async-rspec-1.17.0/lib/async/rspec/reactor.rb:97:in `block (4 levels) in <module:RSpec>'
          # ~/.rvm/gems/ruby-2.7.8/gems/async-rspec-1.17.0/lib/async/rspec/reactor.rb:57:in `block in run_in_reactor'
          # ~/.rvm/gems/ruby-2.7.8/gems/async-1.31.0/lib/async/task.rb:261:in `block in make_fiber'

I'm unsure exactly how this should be resolved and/or why the client cache exists within the adapter in the first place; however, I'm open to suggestions.

In practice, this became a problem when we started using async-http-faraday to parallelize outbound API requests in an environment where multiple processes run in concurrent threads sharing singleton Faraday instances for the outbound APIs. This shared singleton was not a problem prior to adding the async-http-faraday adapter.

[MattFenelon]

We came across this issue when trying to switch the default adapter to this adapter because of some other libraries. Those libraries set a global Faraday connection and reuse it for all requests. They also don't call close on the connection so we will have had memory leaks eventually, had the FiberErrors not happened first.

[ioquatix]

I think we can make this safer by default... let me see what I can come up with.

[jhass]

I seem to be able to workaround this by simply patching out the caching for now:

# Workaround https://github.com/socketry/async-http-faraday/issues/31
module Async
  module HTTP
    module Faraday
      class Adapter < ::Faraday::Adapter
        def client_for(...)
          make_client(...)
        end

        def proxy_client_for(proxy_endpoint, endpoint)
          client_for(proxy_endpoint).proxied_client(endpoint)
        end
      end
    end
  end
end

However I didn't monitor this for memory usage yet.

My usecase is using async-http in Sidekiq for a bunch of different upstream APIs, where a lot of client libraries are using Faraday.

[ioquatix]

@jhass with the latest release, your monkey patch will no longer work (and/or may cause problems).