Skip to content

Commit

Permalink
One semaphore to load them all
Browse files Browse the repository at this point in the history
https://bugzilla.redhat.com/show_bug.cgi?id=1671458
Fixes ManageIQ#544

From MartinH's findings in ManageIQ#544, we had one thread at the
`cspec[:klass].constantize` line while another thread was trying to run
`klass.columns_hash`, causing a
`Circular dependency detected while autoloading...` error.

I then tried a bunch of things until I found a way to reliably recreate
this error:

```ruby
require_relative 'config/environment'

threads = []
4.times do
  threads << Thread.new { Api::Environment.time_attributes }
end
threads.collect(&:join)
```

I could then run the above script several times in my shell and get the
`circular dependency` error most of the time.

```
for x in `seq 1 10`; do; bundle exec ruby test.rb; done
```

With this test in place, I then tried a few solutions:

1) Move the `klass.columns_hash` block into the permit_concurrent_loads
block:

```
diff --git a/lib/api/environment.rb b/lib/api/environment.rb
index 87b34f99..f4b6554a 100644
--- a/lib/api/environment.rb
+++ b/lib/api/environment.rb
@@ -22,9 +22,10 @@ module Api
         # Temporary measure to avoid thread race condition which could lead to a deadlock
         ActiveSupport::Dependencies.interlock.permit_concurrent_loads do
           klass = cspec[:klass].constantize
-        end
-        klass.columns_hash.each do |name, typeobj|
-          result << name if %w(date datetime).include?(typeobj.type.to_s)
+
+          klass.columns_hash.each do |name, typeobj|
+            result << name if %w(date datetime).include?(typeobj.type.to_s)
+          end
         end
       end
     end
```

This did not fix the `circular dependency` error. Perhaps
`permit_concurrent_loads` doesn't handle arbitrarily deep nested autoloads
cross threads?

2) I tried Mutex#synchronize and this worked, but I'd rather we work
with the interlock provided by rails.

```
diff --git a/lib/api/environment.rb b/lib/api/environment.rb
index 87b34f99..f51de73f 100644
--- a/lib/api/environment.rb
+++ b/lib/api/environment.rb
@@ -1,5 +1,6 @@
 module Api
   class Environment
+    ONE_AUTOLOADER_LOCK = Mutex.new
     def self.url_attributes
       @url_attributes ||= Set.new(%w(href))
     end
@@ -19,12 +20,13 @@ module Api
         next if cspec[:klass].blank?
         klass = nil

-        # Temporary measure to avoid thread race condition which could lead to a deadlock
-        ActiveSupport::Dependencies.interlock.permit_concurrent_loads do
+        # Ensure we're the only thread trying to autoload classes and their columns
+        ONE_AUTOLOADER_LOCK.synchronize do
           klass = cspec[:klass].constantize
-        end
-        klass.columns_hash.each do |name, typeobj|
-          result << name if %w(date datetime).include?(typeobj.type.to_s)
+
+          klass.columns_hash.each do |name, typeobj|
+            result << name if %w(date datetime).include?(typeobj.type.to_s)
+          end
         end
       end
     end

```

3) I tried Sync.new with SH and EX locks instead of Mutex and this
failed with Thread killed errors.

4) I changed the `permit_concurrent_loads` on the interlock to `loading`
and this worked, but Yuri found this caused a deadlock.

5) Use `AS::Dep.interlock.loading` from 4) and move the `columns_hash` call into
the `loading` block.  This solves the circular dependency and avoids the
deadlock encountered in 4).

6) Finally, change to use `Rails.application.executor.wrap`, which
behaves similar to interlock.loading but is meants for informing rails
that a plugin needs to load application code.

I noticed, `permit_concurrent_loads` calls `yield_shares` with `compatible: [:load])`
and that method has this in the source code comment:

https://github.com/rails/rails/blob/bb22fe9d4a6102d2a28cb1adfd6fe9d38fc9bb22/activesupport/lib/active_support/concurrency/share_lock.rb#L166-L168

```
  Temporarily give up all held Share locks while executing the
  supplied block, allowing any +compatible+ exclusive lock request
  to proceed.
```

Perhaps, since we're loading code, `permit_concurrent_loads` is too
permissive of other exclusive lock requests and we really need to ensure
nothing else is trying to load.
  • Loading branch information
jrafanie committed Feb 5, 2019
1 parent 203ebed commit f9203f8
Showing 1 changed file with 5 additions and 5 deletions.
10 changes: 5 additions & 5 deletions lib/api/environment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,12 @@ def self.time_attributes
next if cspec[:klass].blank?
klass = nil

# Temporary measure to avoid thread race condition which could lead to a deadlock
ActiveSupport::Dependencies.interlock.permit_concurrent_loads do
# Ensure we're the only thread trying to autoload classes and their columns
Rails.application.executor.wrap do
klass = cspec[:klass].constantize
end
klass.columns_hash.each do |name, typeobj|
result << name if %w(date datetime).include?(typeobj.type.to_s)
klass.columns_hash.each do |name, typeobj|
result << name if %w(date datetime).include?(typeobj.type.to_s)
end
end
end
end
Expand Down

0 comments on commit f9203f8

Please sign in to comment.