[#35782] Allow loading seeds without ActiveJob (~> 5.2.3)#35896
[#35782] Allow loading seeds without ActiveJob (~> 5.2.3)#35896gmcgibbon merged 1 commit intorails:masterfrom
Conversation
There was a problem hiding this comment.
I started with the assumption that removing ActiveJob during the "boot" process would be the appropriate approach, but it was not enough as I noted on lines 906-908. As such, it's possible this part of the change is no longer necessary.
There was a problem hiding this comment.
This might be tricky because we boot rails per test. If we can't recreate the NoMethodError we could try mocking it instead.
There was a problem hiding this comment.
@gmcgibbon - I'm not super familiar with minitest anymore, and it looks like stubbing this out correctly is just not possible without bringing in another test double library, which doesn't makes sense. (I don't want to get into an argument about libraries, but in RSpec stubbing this out without affecting the rest of config would be straightforward.)
There was a problem hiding this comment.
😄 Yes, rspec is much better at stubbing/mocking than minitest, I agree. Thinking further, you can just forgo mocking and do something like this:
test "seed data can be loaded when ActiveJob is not present" do
@plugin.write "db/seeds.rb", <<-RUBY
Bukkits::Engine.config.bukkits_seeds_loaded = true
RUBY
app_file "db/seeds.rb", <<-RUBY
Rails.application.config.app_seeds_loaded = true
RUBY
boot_rails
# In a real app, active_job would be undefined
undefine_config_option(:active_job)
Rails.application.load_seed
assert Rails.application.config.app_seeds_loaded
Bukkits::Engine.load_seed
assert Bukkits::Engine.config.bukkits_seeds_loaded
end
private
def undefine_config_option(name)
Rails.application.config.class.class_variable_get(:@@options).delete(name)
end
There was a problem hiding this comment.
@gmcgibbon - that's great! I've added both of your suggestions, and will squash the commits and force push my branch after this is approved.
c17b512 to
0e22fbe
Compare
railties/lib/rails/engine.rb
Outdated
There was a problem hiding this comment.
If this raises when invoking active_job, try is correct here, but I think &. is better for reading the queue adapter. That should exist when not nil.
There was a problem hiding this comment.
@gmcgibbon - I tried &. first, but it is less forgiving than try and threw exceptions (at least in my imperfect test scenario).
There was a problem hiding this comment.
Yes, I would expect it to throw an exception on the second option invoke. We still need try for the first call to suppress a potential NoMethodError. eg. config.try(:active_job)&.(:queue_adapter).
There was a problem hiding this comment.
This might be tricky because we boot rails per test. If we can't recreate the NoMethodError we could try mocking it instead.
|
I think #35905 supersedes this, and is already merged. |
|
Ah yes, I see now.
|
56900e7 to
36ee3e8
Compare
|
@gmcgibbon, could you review this again for me? Is there anything special to be done to help ensure that this fix is included in any future Rails 5.2.x release (instead of only being included in 6.x)? |
gmcgibbon
left a comment
There was a problem hiding this comment.
👍 Tested this locally and seeding now runs without Active Job. Please squash your commits. I believe since this is a fix for a released bug, we also need a changelog to railties too. We should be able to backport your commit to 5-2-stable and release from there, no need to do anything extra.
36ee3e8 to
bfc7363
Compare
bfc7363 to
b08daf4
Compare
|
@gmcgibbon - I've rebased from the latest (at least for the moment) master and amended my commit with a changelog entry. |
|
@gmcgibbon did you backport this PR to 5-2-stable? |
[#35782] Allow loading seeds without ActiveJob (~> 5.2.3)
|
Sorry for the delay, backported in a6bf032 |
|
Is this change going to be included in a patch release? |
|
Yes. Next 5.2.x |
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.
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.
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. (cherry picked from commit 648da12)
Summary
This fixes #35782.
#34953 introduced a change to process engine's seed files with ActiveJob. However, ActiveJob has always been an optional framework, and this change (released in Rails 5.2.3) breaks
rake db:seed(andrake db:setupfrequently used in CI/CD) for any apps that are not loading ActiveJob.This change should allow for engine seeding to continue with ActiveJob when available, and without ActiveJob when not available.
Other Information
This change should be applied to any future Rails 5.2 bug fix releases as well as Rails 6.x, but I'm not sure of the best way to go about that.