Remove executor/reloader for less interlocking #97
Conversation
sj26
force-pushed
the
less-interlocking
branch
from
e1c0b40
to
029c146
Compare
Take a leaf from actioncable's book and explicitly checkout and own a connection so we don't have to worry about locking. See: https://github.com/rails/rails/blob/acaa546026471354ae1ff15e96c6bf5d39b0e34a/actioncable/lib/action_cable/subscription_adapter/postgresql.rb#L36-L49
I've observed swallowed errors here.
The instrumentation is hairy, but they have their own interlocks during dispatch.
ActiveJob takes care of invoking a full executor around the actual piece of work which will invoke code reloading etc.
lib/good_job/scheduler.rb:140:76: C: Style/SymbolProc: Pass &:next as an argument to new instead of a block.
future = Concurrent::Future.new(args: [@Performer], executor: @pool) do |performer| ...
^^^^^^^^^^^^^^
Although I'm not sure I find this cleaner.
sj26
force-pushed
the
less-interlocking
branch
from
029c146
to
9bf1f42
Compare
| Rails.application.executor.wrap { output = performer.next } | ||
| output | ||
| end | ||
| future = Concurrent::Future.new(executor: @pool, &@performer.method(:next)) |
There was a problem hiding this comment.
This is very clean, but I think I may regret it in the future because it's easy to overlook this is where the work gets done.
But I'm happy to roll with it for now and just make a mental note that if it causes confusion in the future, to go to the more expansive version.
And just while I'm noodling:
- I think it would be possible to cache the proc so it doesn't have to be recreated everytime this method is called
- I wonder if the performer should alias
calltonextso it is directly callable (I'm imagining there was a reason why that wasn't done originally but I'm not remembering it... though it was likely that I wanted explicitness)
|
|
||
| def with_listen_connection | ||
| ar_conn = ActiveRecord::Base.connection_pool.checkout.tap do |conn| | ||
| ActiveRecord::Base.connection_pool.remove(conn) |
There was a problem hiding this comment.
Not a blocker, but I am curious: this will have the effect of using one additional database connection than the pool size?
There was a problem hiding this comment.
Yes, it will, good spot. It should probably be documented. But I don't think this is unreasonable — the total number of ideal connections will be one for the notifier, and then one for each worker thread.
It's the same approach as actioncable, and I think they have the same caveat documented somewhere...
There was a problem hiding this comment.
Oh, I don't think they do — I can't find it. I'd expect it to be here:
https://edgeguides.rubyonrails.org/action_cable_overview.html#postgresql-adapter
bensheldon
changed the title
|
🥳 |
I believe this will fix a bunch of dependency interlocking issues. The only thing good_job needs from the executor and/or reloader is activerecord connection management, and considering how tightly intertwined with activerecord it is, it's very reasonable to own the connection management in the same way actioncable does.
ActiveJob takes care of the interface to user rails code which requires reloading, and makes sure the executor wraps user code in the right places:
https://github.com/rails/rails/blob/acaa546026471354ae1ff15e96c6bf5d39b0e34a/activejob/lib/active_job/railtie.rb#L39-L47
The only reason Sidekiq etc have to get involved with the executor is because they execute user code outside of ActiveJob.
Fixes #95