Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #411 Wrap job invocation in Rails executor when using ActiveRecord #412

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

tomgi
Copy link
Contributor

@tomgi tomgi commented Jan 4, 2024

See #411 first.

The connection is being released by que, because Rails.application.executor has this callback registered, being called here.

A smaller reproducible example is here (raises on master, doesn't raise on this branch anymore):

class TestQueJob < Que::Job
  def run(id)
    conn = ::ActiveRecord::Base.connection

    Que.pool.execute("SELECT 1;")
    
    if conn != ::ActiveRecord::Base.connection
      raise "Connection changed"
    end
  end
end

I think que should be executing the entire job code inside of Rails.application.executor, then the nested one in connection would be a noop and the connection would only be released after the whole job is finished.

That's what other background job libraries like sidekiq and shoryuken are doing, which follow the Rails guidelines for writing libraries here https://guides.rubyonrails.org/v7.1/threading_and_code_execution.html#wrapping-application-code

If you're writing a library or component that will invoke application code, you should wrap it with a call to the executor

@tomgi
Copy link
Contributor Author

tomgi commented Jun 27, 2024

I've added a small fix to this branch that we discovered while testing it in production.

The problem is very similar to #393 - it happens when nesting a synchronous job execution inside of another Que job, e.g.:

class TestQueJob < Que::Job
  def run
    ActiveRecord::Base.transaction do
      ActiveRecord::Base.connection_pool.active_connection? # true
      ActiveRecord::Base.connection.transaction_open? # true

      TestQueJob2.run
      # ^ the job runs synchronously, invokes a nested instance of Que::ActiveRecord::Connection::JobMiddleware 
      # and calls ::ActiveRecord::Base.clear_active_connections!

      raise "lost connection" unless ActiveRecord::Base.connection_pool.active_connection?
      # ^ our connection with open transaction got returned back to the pool
    end
  end
end

class TestQueJob2 < Que::Job
  def run; end
end

That sample job would raise before the patch applied in f24adb2

@tomgi tomgi force-pushed the wrap_job_in_rails_executor branch from 62517fd to f24adb2 Compare June 27, 2024 11:17
@owst
Copy link
Contributor

owst commented Jul 10, 2024

This would be great to get merged - @tomgi any thoughts on the cause of the failing checks?

@owst
Copy link
Contributor

owst commented Jul 10, 2024

Looks like #421 may well be related

owst added a commit to bambooengineering/que that referenced this pull request Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants