Closed zarqman closed 9 months ago
zarqman
commented
9 months ago I found this helpful when debugging:
# sometime before contents of seeds.rb runs:
Rails.autoloaders.main.on_unload do |class_name, _, _|
puts "Removed const: #{class_name}"
pp caller if class_name == 'Car'
endIn the backtrace, the first mention of good_job (line 42) points to enqueue_at.
bensheldon
commented
9 months ago Thanks for reporting this!! It's a little unexpected because I think that seeds should already be wrapped with an Executor: https://github.com/rails/rails/pull/40626
...so wrap in GoodJob should be a noop. Though I'm still a little unclear on the implications of wrapping a reloader with an executor or vice versa.
I'm reluctant to remove those executors/reloaders from the Adapter because I have seen the problem of people enqueuing jobs inline without an Executor and then the database connection is dropped, losing the advisory lock prematurely.
So maybe the issue here is that it's a reloader and it should be an executor.
zarqman
commented
9 months ago Interesting. I tried manually wrapping the seeds with an Executor and it still fails. But if Rails is already wrapping in an Executor, then that would make sense. But when I wrap them with a Reloader, then they work. So it would seem than an outside Executor (Rails or seeds) and inside Reloader (GoodJob) still triggers an actual reload, whereas a Reloader both outside and inside causes the inside to no-op.
It sounds like you're using Executor/Reloader both for code reloading and database connection handling?
I'll try changing enqueue_at to use an executor and report back.
zarqman
commented
9 months ago I swapped out reloader to executor inside enqueue_at and that does resolve the errant reload. The app's test suite and a spot check don't indicate any negative side effects. Reloading of *Job classes still happens with env=development.
segiddins
commented
9 months ago Chiming in that changing to Rails.application.executor.wrap inside enqueue_at also resolves the reload error I was running into
bensheldon
commented
9 months ago @zarqman thank you for digging into that and @segiddins for the confirmation 🙇🏻 🙇🏻
I'll make a quick PR converting those places (anywhere that isn't in a thread that GoodJob for sure sets up itself) into an executor instead of a reloader.
I think I'm still unsure if it's possible to drop database connections moving between reloader <--> executor contexts, but let's solve the problem in front of us right now.
I'm running into an issue with missing/mismatched constants during
rails db:seed. I've tracked it down to changes made in #1124.Explanation
This is a super simplified example.
seeds.rbhas this general shape:and
Wheelhas:and
Carhas:GoodJob's use of the reloader during
enqueue_atis causing a code reload whenSomeJob.perform_lateris called.By the time Car's callbacks run (which is after Wheel's), Car != Car and the case statement errors.
Context
Rails 7.1.2 GoodJob 3.19.4 works properly GoodJob 3.20.0-3.22.0 are broken
Fixes / Workarounds
It's possible to surround the entire contents of seeds.rb with
Rails.application.reloader.wrap{ ... }. However, sinceseeds.rbis standard Rails behavior, it'd be nice to find a proper fix.I'll try to take a look at GJ's internals before too long and see what can be done to fix it. In the meantime, thought I'd create an issue now and see what you think?