Set middleware_mutex when middleware/adapter classes are defined

[technoweenie]

This is an attempt to fix #1065 by calling #middleware_mutex when the classes are defined. This should prevent @middleware_mutex from being undefined when a Faraday::Connection instance is used in a multi-threaded environment.

Right now, the proper way to build a Faraday::Connection for use in a multi-threaded environment should look something like this:

conn = Faraday.new(url: "some-base-url") do |f|
  f.request :something
  f.response :or_other
  f.adapter :some_http_client
end

conn.app

faraday/lib/faraday/rack_builder.rb

Lines 156 to 168 in d51a6c6

	# The "rack app" wrapped in middleware. All requests are sent here.
	#
	# The builder is responsible for creating the app object. After this,
	# the builder gets locked to ensure no further modifications are made
	# to the middleware stack.
	#
	# Returns an object that responds to `call` and returns a Response.
	def app
	@app ||= begin
	lock!
	to_app
	end
	end

Faraday::Connection#app is delegated to Faraday::RackBuilder#app, which does the following:

• Uses the Faraday::Request middleware registry to load :something
• Uses the Faraday::Response middleware registry to load :or_other
• Uses the Faraday::Adapter middleware registry to load :some_http_client
• Locks the Faraday::RackBuilder instance from changing the middleware chain (conn.builder.handlers)
• Instantiates all of the middleware instances
• Builds the "app" instance from the loaded recursive middleware instances.

At this point, it should be safe to use shared connection object across multiple threads. But, I don't know how often it's used like this, so there are likely some more bugs.

I also have another larger solution in mind that requires some refactoring. I'll publish once I have it working.

[ioquatix]

Another option is to just define Faraday::Connection as thread unsafe by default, and expect the user to put locking around it.

Async::HTTP::Faraday is not designed to be used across threads. One way to deal with this is to either:

• Issue a warning and fail if use from a different thread, or
• Use a hash table of thread -> client pool.

Just thoughts FYI.

[olleolleolle]

When I read your comment, I took it as something like this:

diff --git a/lib/faraday/adapter.rb b/lib/faraday/adapter.rb
index 535ecb5..565c490 100644
--- a/lib/faraday/adapter.rb
+++ b/lib/faraday/adapter.rb
@@ -33,7 +33,7 @@ module Faraday
 
       def inherited(subclass)
         super
-        subclass.middleware_mutex
+        subclass.init_mutex
         subclass.supports_parallel = supports_parallel?
       end
     end
diff --git a/lib/faraday/middleware_registry.rb b/lib/faraday/middleware_registry.rb
index 692d0a0..51e664f 100644
--- a/lib/faraday/middleware_registry.rb
+++ b/lib/faraday/middleware_registry.rb
@@ -8,7 +8,7 @@ module Faraday
   module MiddlewareRegistry
     def self.extended(klass)
       super
-      klass.middleware_mutex
+      klass.init_mutex
     end
 
     # Register middleware class(es) on the current module.
@@ -96,9 +96,12 @@ module Faraday
         raise(Faraday::Error, "#{key.inspect} is not registered on #{self}")
     end
 
+    def init_mutex
+      @middleware_mutex = Monitor.new
+    end
+
     def middleware_mutex(&block)
       puts "#{self}: middleware_mutex" if @middleware_mutex.nil?
-      @middleware_mutex ||= Monitor.new
       @middleware_mutex.synchronize(&block) if block
     end

Or did you mean:

diff --git a/lib/faraday/middleware_registry.rb b/lib/faraday/middleware_registry.rb
index 692d0a0..51e664f 100644
--- a/lib/faraday/middleware_registry.rb
+++ b/lib/faraday/middleware_registry.rb
@@ -8,7 +8,7 @@ module Faraday
   module MiddlewareRegistry
     def self.extended(klass)
       super
-      klass.middleware_mutex
+      klass.init_mutex
     end
 
     # Register middleware class(es) on the current module.
@@ -96,9 +96,12 @@ module Faraday
         raise(Faraday::Error, "#{key.inspect} is not registered on #{self}")
     end
 
+    def init_mutex
+      @middleware_mutex = Monitor.new
+    end
+
     def middleware_mutex(&block)
       puts "#{self}: middleware_mutex" if @middleware_mutex.nil?
-      @middleware_mutex ||= Monitor.new
       @middleware_mutex.synchronize(&block) if block
     end

[ioquatix]

@middleware_mutex.synchronize(&block) if block could also become @middleware_mutex.synchronize(&block)

And yes something like that.

[olleolleolle]

Rephrasing, losing the if:

diff --git a/lib/faraday/middleware_registry.rb b/lib/faraday/middleware_registry.rb
index 692d0a0..5fefaff 100644
--- a/lib/faraday/middleware_registry.rb
+++ b/lib/faraday/middleware_registry.rb
@@ -8,7 +8,7 @@ module Faraday
   module MiddlewareRegistry
     def self.extended(klass)
       super
-      klass.middleware_mutex
+      klass.init_mutex
     end
 
     # Register middleware class(es) on the current module.
@@ -96,10 +96,13 @@ module Faraday
         raise(Faraday::Error, "#{key.inspect} is not registered on #{self}")
     end
 
+    def init_mutex
+      @middleware_mutex = Monitor.new
+    end
+
     def middleware_mutex(&block)
       puts "#{self}: middleware_mutex" if @middleware_mutex.nil?
-      @middleware_mutex ||= Monitor.new
-      @middleware_mutex.synchronize(&block) if block
+      @middleware_mutex.synchronize(&block)
     end
 
     def fetch_middleware(key)

[ioquatix]

Does def init_mutex need to be def self.init_mutex?

Also does it still work if you make it protected?

[olleolleolle]

If I make it self.init_mutex where it sits now, it fails the current test suite like:

An error occurred while loading spec_helper.
Failure/Error: klass.init_mutex

NoMethodError:
  undefined method `init_mutex' for Faraday::Middleware:Class
# ./lib/faraday/middleware_registry.rb:11:in `extended'
# ./lib/faraday/middleware.rb:6:in `extend'
# ./lib/faraday/middleware.rb:6:in `<class:Middleware>'
# ./lib/faraday/middleware.rb:5:in `<module:Faraday>'
# ./lib/faraday/middleware.rb:3:in `<top (required)>'
# ./lib/faraday.rb:94:in `require'
# ./lib/faraday.rb:94:in `block in require_libs'
# ./lib/faraday.rb:93:in `each'
# ./lib/faraday.rb:93:in `require_libs'
# ./lib/faraday.rb:161:in `<module:Faraday>'
# ./lib/faraday.rb:21:in `<top (required)>'
# ./spec/spec_helper.rb:34:in `require'
# ./spec/spec_helper.rb:34:in `block in <top (required)>'
# ./spec/spec_helper.rb:34:in `each'
# ./spec/spec_helper.rb:34:in `<top (required)>'
No examples found.

Will make an attempt re: protected.

That leads to:

NoMethodError:
  protected method `init_mutex' called for Faraday::Middleware:Class

(Since I'm calling it in those included(subclass), in other mixed in methods.)

[ioquatix]

I need to check it.