Introduces Connection pool in compatible adapters

[iMacTia]

Description

Attempt to introduce a ConnectionPool in compatible adapters.

Todos

List any remaining work that needs to be done, i.e:

• Tests: Can we write better tests? Can we test concurrent use of the pool? I tried executing a request before the previous one was completed, but the number of available connections in the pool was not decreasing (see 596d420)
• There's a strange error popping up only sometimes, possibly a race condition. Any help on finding the actual cause would be welcome:

NoMethodError:
    undefined method `+' for nil:NilClass
    # /Users/mattiagiuffrida/.rvm/gems/ruby-2.5.1/gems/connection_pool-2.2.2/lib/connection_pool.rb:89:in `checkout'
    # /Users/mattiagiuffrida/.rvm/gems/ruby-2.5.1/gems/connection_pool-2.2.2/lib/connection_pool.rb:62:in `block in with'
    # /Users/mattiagiuffrida/.rvm/gems/ruby-2.5.1/gems/connection_pool-2.2.2/lib/connection_pool.rb:61:in `handle_interrupt'
    # /Users/mattiagiuffrida/.rvm/gems/ruby-2.5.1/gems/connection_pool-2.2.2/lib/connection_pool.rb:61:in `with'
    # ./lib/faraday/adapter/patron.rb:19:in `call'
    # ./lib/faraday/response.rb:11:in `call'
    # ./lib/faraday/request/url_encoded.rb:23:in `call'
    # ./lib/faraday/request/multipart.rb:25:in `call'
    # ./lib/faraday/rack_builder.rb:153:in `build_response'
    # ./lib/faraday/connection.rb:504:in `run_request'
    # ./lib/faraday/connection.rb:233:in `options'
    # ./spec/support/shared_examples/request_method.rb:94:in `public_send'
    # ./spec/support/shared_examples/request_method.rb:94:in `block (2 levels) in <top (required)>'

Additional Notes

I've only added the connection pool to Patronand HTTPClient, trying to address #1001.
Net::HTTP adapter is actually incompatible by design, because we create a different object based on the env, so there's no way to reuse connections unless we first refactor that bit. The same is for Excon connections.
Net::HTTP::Persistent has an internal connection pool, no sense wrapping it into another one.
EM-related adapters have a complex concurrent behaviour which makes connection pooling unnecessary.
Typhoeus is maintained externally now :)
cc @technoweenie @olleolleolle @julik

[technoweenie]

Dang, I totally forgot to review this. It looks solid on an initial pass.

Can we test concurrent use of the pool?

I think this would be better handled in faraday-live. I'll work on adding tests as a way to get familiar with this patch, and hopefully find that intermittent bug.

[technoweenie]

Unfortunately, I don't think this is going to work without some major changes. Ideally, the pool should spit out a fully configured session object so the adapter can make the request. Faraday adapters were designed around a single method (#call(env)), so they configure and use the client each request. With this PR, the Patron and HTTPClient adapters modify the object after it's out of the pool.

The Patron adapter runs the config block each for each request:

Faraday.new do |f|
  f.adapter :patron do |session|
    session.max_redirects = 10
  end
end

faraday/lib/faraday/adapter/patron.rb

Lines 19 to 22 in 8567289

	response = pool.with do |session|
	@config_block&.call(session)
	perform_request(session, env)
	end

This can easily be fixed by calling @config_block when the pool is initialized:

--- a/lib/faraday/adapter/concerns/connection_pooling.rb
+++ b/lib/faraday/adapter/concerns/connection_pooling.rb
@@ -1,4 +1,5 @@
 # frozen_string_literal: true
+require 'connection_pool'

 # This module marks an Adapter as supporting connection pooling.
 module ConnectionPooling
@@ -26,11 +27,20 @@ module ConnectionPooling
   # @param opts [Hash] the options to pass to `ConnectionPool` initializer.
   def initialize_pool(opts = {})
     ensure_connection_defined!
-    @pool = ConnectionPool.new(opts, &method(:connection))
+
+    if !@config_block
+      @pool = ConnectionPool.new(opts, &method(:connection))
+      return
+    end
+
+    @pool = ConnectionPool.new opts do
+      connection.tap { |c| @config_block.call(c) }
+    end
   end

Both adapters only set certain SSL settings on the session if the current request is to an HTTPS server:

faraday/lib/faraday/adapter/httpclient.rb

Lines 59 to 61 in 8567289

	if env[:url].scheme == 'https' && (ssl = env[:ssl])
	configure_ssl(client, ssl)
	end

faraday/lib/faraday/adapter/patron.rb

Lines 66 to 69 in 8567289

	def perform_request(session, env)
	if (env[:url].scheme == 'https') && env[:ssl]
	configure_ssl(session, env[:ssl])
	end

The Work

Here's another way to approach this. It's a lot more work, but I think the benefits will be good.

1. Add #build_connection to every adapter, which outputs the fully-configured http client from options.
2. Move settings that configure the HTTP connection from RequestOptions to ConnectionOptions: :proxy, :bind, :timeout, :open_timeout, :write_timeout
3. Teach Adapter#initialize to save the connection if it can:

• If any of the moved RequestOptions settings are used, skip this!
• If the adapter supports pooling, @pool = ConnectionPool.new(opts, &method(:build_connection))
• If the adapter does not support pooling, @conn = build_connection

4. Add connection(&block) to yield a connection for the adapter to use:

• If any of the moved RequestOptions settings are used, yield #build_connection)
• If the adapter supports pooling, pass block to @pool.with
• If the adapter does not support pooling, yield @conn

How does this sound? Is it worth trying to get in for Faraday v1.0?

EDIT: I've created a tracking issue for this: #1024