Wrap evaluation of db/seeds.rb with the executor#40626
Merged
rafaelfranca merged 1 commit intorails:masterfrom Nov 24, 2020
Merged
Wrap evaluation of db/seeds.rb with the executor#40626rafaelfranca merged 1 commit intorails:masterfrom
rafaelfranca merged 1 commit intorails:masterfrom
Conversation
jonathanhefner
commented
Nov 15, 2020
railties/lib/rails/engine.rb
Outdated
| initializer :set_executor, before: :load_environment_hook do |app| | ||
| @executor = app.executor | ||
| @reloader = app.reloader | ||
| end |
Member
Author
There was a problem hiding this comment.
Is this a bad idea? An alternative would be to add a load_seed callback and something like the following:
initializer :wrap_load_seed_with_executor do |app|
self.class.set_callback(:load_seed, :around) do |_, loading|
app.executor.wrap(&loading)
end
end
Member
There was a problem hiding this comment.
I don't think engines should have their own executors and reloaders. I think we should go with a different implementation.
Member
Could you elaborate on this? It's not clear to me how wrapping the evaluation of |
Member
Author
I was able to recreate the hang from #34939 with a Post.create!.image.attach(io: File.open("path/to/tiny.gif"), filename: "tiny.gif")
sleep 1
Post.create!The Running Main thread backtraceJob thread backtraceWeaving the relevant lines together in chronological order (via
After adding the Job thread backtraceThe job thread did not reach I didn't think about this before, but the hang does go away when |
Before rails#34953, when using the `:async` Active Job queue adapter, jobs enqueued in `db/seeds.rb`, such as Active Storage analysis jobs, would cause a hang (see rails#34939). Therefore, rails#34953 changed all jobs enqueued in `db/seeds.rb` to use the `:inline` queue adapter instead. (This behavior was later limited to only take effect when the `:async` adapter was configured, see rails#35905.) However, inline jobs in `db/seeds.rb` cleared `CurrentAttributes` values (see rails#37526). Therefore, rails#37568 changed the `:inline` adapter to wrap each job in its own thread, for isolation. However, wrapping a job in its own thread affects which database connection it uses. Thus inline jobs can no longer execute within the calling thread's database transaction, including seeing any uncommitted changes. Additionally, if the calling thread is not wrapped with the executor, the inline job thread (which is wrapped with the executor) can deadlock on the load interlock. And when testing (with `connection_pool.lock_thread = true`), the inline job thread can deadlock on one of the locks added by rails#28083. Therefore, this commit reverts the solutions of rails#34953 and rails#37568, and instead wraps evaluation of `db/seeds.rb` with the executor. This eliminates the original hang from rails#34939, which was also due to running multiple threads and not wrapping all of them with the executor. And, because nested calls to `executor.wrap` are ignored, any inline jobs in `db/seeds.rb` will not clear `CurrentAttributes` values. Alternative fix for rails#34939. Reverts rails#34953. Reverts rails#35905. Partially reverts rails#35896. Alternative fix for rails#37526. Reverts rails#37568. Fixes rails#40552.
5ab6e0d to
648da12
Compare
| class Railtie | ||
| autoload :Configuration, "rails/railtie/configuration" | ||
|
|
||
| extend ActiveSupport::DescendantsTracker |
Member
There was a problem hiding this comment.
Why this change? This is adding a lot of methods to railties that we don't need to add.
Member
Author
There was a problem hiding this comment.
This is to prevent Railtie.subclasses from being overridden by DescendantsTracker#subclasses when ActiveSupport::Callbacks is included in Engine (because ActiveSupport::Callbacks also extends DescendantsTracker). If Railtie.subclasses is overridden, then it will include abstract railties, which will raise an error when instantiated via Railties#initialize.
An alternative would be to move the abstract railties filtering logic to Railties#initialize, though that would change the contract of Railtie.subclasses.
It's worth mentioning, though, that DescendantsTracker would only add ::descendants and ::direct_descendants methods to railties.
Contributor
|
The problem described in #40552 is still happening in Rails 6.1.0rc2 (but is not happening in master). Can we get this merged to the 6.1.0 branch? |
|
Did the follow up PR for ActiveSupport.on_load(:active_storage_job) { self.queue_adapter = :inline } ever get opened? This is a regression. Inserting ActiveJob::Base.queue_adapter = :inline at the top of the seeds.rb file seems to be the work around for now. |
Contributor
|
The trick |
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Before #34953, when using the
:asyncActive Job queue adapter, jobs enqueued indb/seeds.rb, such as Active Storage analysis jobs, would cause a hang (see #34939). Therefore, #34953 changed all jobs enqueued indb/seeds.rbto use the:inlinequeue adapter instead. (This behavior was later limited to only take effect when the:asyncadapter was configured, see #35905.) However, inline jobs indb/seeds.rbclearedCurrentAttributesvalues (see #37526). Therefore, #37568 changed the:inlineadapter to wrap each job in its own thread, for isolation. However, wrapping a job in its own thread affects which database connection it uses. Thus inline jobs can no longer execute within the calling thread's database transaction, including seeing any uncommitted changes. Additionally, if the calling thread is not wrapped with the executor, the inline job thread (which is wrapped with the executor) can deadlock on the load interlock. And when testing (withconnection_pool.lock_thread = true), the inline job thread can deadlock on one of the locks added by #28083.Therefore, this commit reverts the solutions of #34953 and #37568, and instead wraps evaluation of
db/seeds.rbwith the executor. This eliminates the original hang from #34939, which was also due to running multiple threads and not wrapping all of them with the executor. And, because nested calls toexecutor.wrapare ignored, any inline jobs indb/seeds.rbwill not clearCurrentAttributesvalues.Alternative fix for #34939.
Reverts #34953.
Reverts #35905.
Partially reverts #35896.
Alternative fix for #37526.
Reverts #37568.
Fixes #40552.
It's worth noting that
rails db:seedwill no longer run:asyncadapter jobs, because those jobs will wait for a lock acquired viaexecutor.wrap, and then be killed byConcurrent::ThreadPoolExecutorwhen the process exits. However, this makes the:asyncadapter behave like all other non-:inlineadapters, so I think it's reasonable. If we specifically want Active Storage analysis jobs to run duringrails db:seed, we can add something like the following toload_seed:If there is interest in that, I will open a follow-up PR.