synchronize access to connections in the pool

also fixed the coalescing case where the connection may come from the pool, and should therefore be remmoved from there and selected/checked back in accordingly as a result

lib/httpx/connection.rb

@@ -713,7 +713,6 @@ module HTTPX
 
       alt_options = @options.merge(ssl: @options.ssl.merge(hostname: URI(origin).host))
 
-      # TODO: leaks connection object into the pool
       connection = @current_session.find_connection(alt_origin, @current_selector, alt_options)
 
       # advertised altsvc is the same origin being used, ignore

lib/httpx/plugins/internal_telemetry.rb

@@ -77,7 +77,7 @@ module HTTPX
           response
         end
 
-        def coalesce_connections(conn1, conn2, selector)
+        def coalesce_connections(conn1, conn2, selector, *)
           result = super
 
           meter_elapsed_time("Connection##{conn2.object_id} coalescing to Connection##{conn1.object_id}") if result

lib/httpx/pool.rb

@@ -12,22 +12,13 @@ module HTTPX
       @options = options
       @pool_options = options.pool_options
       @resolvers = Hash.new { |hs, resolver_type| hs[resolver_type] = [] }
+      @resolver_mtx = Thread::Mutex.new
       @connections = []
+      @connection_mtx = Thread::Mutex.new
     end
 
-    # def checkout_connection_by_options(options)
-    #   conn = @connections.find do |connection|
-    #     connection.options == options
-    #   end
-    #   return unless conn
-
-    #   @connections.delete(conn)
-
-    #   conn
-    # end
-
     def pop_connection
-      @connections.shift
+      @connection_mtx.synchronize { @connections.shift }
     end
 
     # opens a connection to the IP reachable through +uri+.
@@ -37,58 +28,63 @@ module HTTPX
     def checkout_connection(uri, options)
       return checkout_new_connection(uri, options) if options.io
 
-      conn = @connections.find do |connection|
-        connection.match?(uri, options)
-      end
+      @connection_mtx.synchronize do
+        conn = @connections.find do |connection|
+          connection.match?(uri, options)
+        end
+        @connections.delete(conn) if conn
 
-      return checkout_new_connection(uri, options) unless conn
-
-      @connections.delete(conn)
-
-      conn
+        conn
+      end || checkout_new_connection(uri, options)
     end
 
     def checkin_connection(connection, delete = false)
       return if connection.options.io
 
-      @connections << connection unless delete
+      @connection_mtx.synchronize { @connections << connection } unless delete
     end
 
     def checkout_mergeable_connection(connection)
       return if connection.options.io
 
-      @connections.find do |ch|
-        ch != connection && ch.mergeable?(connection)
+      @connection_mtx.synchronize do
+        conn = @connections.find do |ch|
+          ch != connection && ch.mergeable?(connection)
+        end
+        @connections.delete(conn) if conn
+
+        conn
       end
     end
 
     def reset_resolvers
-      @resolvers.clear
+      @resolver_mtx.synchronize { @resolvers.clear }
     end
 
     def checkout_resolver(options)
       resolver_type = options.resolver_class
       resolver_type = Resolver.resolver_for(resolver_type)
 
-      resolvers = @resolvers[resolver_type]
+      @resolver_mtx.synchronize do
+        resolvers = @resolvers[resolver_type]
 
-      resolver = resolvers.find do |res|
-        res.options == options
-      end
+        resolver = resolvers.find do |res|
+          res.options == options
+        end
+        resolvers.delete(resolver)
 
-      return checkout_new_resolver(resolver_type, options) unless resolver
-
-      resolvers.delete(resolver)
-
-      resolver
+        resolver
+      end || checkout_new_resolver(resolver_type, options)
     end
 
     def checkin_resolver(resolver)
-      resolvers = @resolvers[resolver.class]
+      @resolver_mtx.synchronize do
+        resolvers = @resolvers[resolver.class]
 
-      resolver = resolver.multi
+        resolver = resolver.multi
 
-      resolvers << resolver unless resolvers.include?(resolver)
+        resolvers << resolver unless resolvers.include?(resolver)
+      end
     end
 
     private

lib/httpx/session.rb

@@ -257,7 +257,6 @@ module HTTPX
     def build_requests(*args, params)
       requests = if args.size == 1
         reqs = args.first
-        # TODO: find a way to make requests share same options object
         reqs.map do |verb, uri, ps = EMPTY_HASH|
           request_params = params
           request_params = request_params.merge(ps) unless ps.empty?
@@ -266,7 +265,6 @@ module HTTPX
       else
         verb, uris = args
         if uris.respond_to?(:each)
-          # TODO: find a way to make requests share same options object
           uris.enum_for(:each).map do |uri, ps = EMPTY_HASH|
             request_params = params
             request_params = request_params.merge(ps) unless ps.empty?
@@ -377,16 +375,19 @@ module HTTPX
     end
 
     def on_resolver_connection(connection, selector)
-      found_connection = selector.find_mergeable_connection(connection) ||
-                         @pool.checkout_mergeable_connection(connection)
+      from_pool = false
+      found_connection = selector.find_mergeable_connection(connection) || begin
+        from_pool = true
+        @pool.checkout_mergeable_connection(connection)
+      end
 
       return select_connection(connection, selector) unless found_connection
 
       if found_connection.open?
-        coalesce_connections(found_connection, connection, selector)
+        coalesce_connections(found_connection, connection, selector, from_pool)
       else
         found_connection.once(:open) do
-          coalesce_connections(found_connection, connection, selector)
+          coalesce_connections(found_connection, connection, selector, from_pool)
         end
       end
     end
@@ -410,15 +411,19 @@ module HTTPX
       resolver
     end
 
-    def coalesce_connections(conn1, conn2, selector)
+    # coalesces +conn2+ into +conn1+. if +conn1+ was loaded from the connection pool
+    # (it is known via +from_pool+), then it adds its to the +selector+.
+    def coalesce_connections(conn1, conn2, selector, from_pool)
       unless conn1.coalescable?(conn2)
         select_connection(conn2, selector)
+        @pool.checkin_connection(conn1) if from_pool
         return false
       end
 
       conn2.emit(:tcp_open, conn1)
       conn1.merge(conn2)
       conn2.coalesced_connection = conn1
+      select_connection(conn1, selector) if from_pool
       deselect_connection(conn2, selector)
       true
     end

sig/pool.rbs

@@ -4,7 +4,9 @@ module HTTPX
 
     @options: Options
     @resolvers: Hash[Class, Array[resolver_manager]]
+    @resolver_mtx: Thread::Mutex
     @connections: Array[Connection]
+    @connection_mtx: Thread::Mutex
 
     def pop_connection: () -> Connection?
 

sig/session.rbs

@@ -74,7 +74,7 @@ module HTTPX
 
     def find_resolver_for: (Connection connection, Selector selector) -> (Resolver::Multi | Resolver::Resolver)
 
-    def coalesce_connections: (Connection conn1, Connection conn2, Selector selector) -> bool
+    def coalesce_connections: (Connection conn1, Connection conn2, Selector selector, bool from_pool) -> bool
 
     attr_reader self.default_options: Options
   end