cfis / activejob-locking Goto Github PK

Using the latest version on my rails app I see that only the Memory adapter is being required by default. Is this by design?

Hi there. I'm using the suo adapter on my project and I had to hack activejob-locking to pass through a port and password for the redis server to the underlying suo gem. I'd submit a pull request, but the activejob-locking gem's current methodology for distributed servers is kind of counter-intuitive for the redis case. The suo adapter, for example, just uses { host: options.hosts.first } and drops any other hosts in the array. Would I ideally pass in an array of ports and passwords which would each get truncated to the first element?

In any case, supporting ports and passwords, or entire connection strings, seems like a pretty basic feature, which I would like to see supported at some point.

Cheers. :)

Heroku gives us a Redis connection string that looks kinda like this:
redis://h:[email protected]:6379

Unfortunately the SuoRedis adapter doesn't support passing AdapterOptions, so we can't do this:

ActiveJob::Locking.options.adapter = ActiveJob::Locking::Adapters::SuoRedis
ActiveJob::Locking.options.adapter_options = { connection: { url: ENV['REDIS_URL'] } }

My app experienced an explosion of redis connections after I started using this gem, which is a problem because the redis service I use has a connection cap. It seems that this gem never disconnects redis connections, nor does it use a redis connection pool. Redis connections are left open until their ruby objects are eventually garbage collected.

The easiest solution for me was to shim some disconnect code into the "unlock" method, since I was overriding the suo gem adapter anyway so I could specify a port and password (see #3).

In this case the relevant code is:

module SuoLockingAdapterExtension
  def unlock
    super
    lock_manager.client.close
  end
end
ActiveJob::Locking::Adapters::SuoRedis.send :prepend, SuoLockingAdapterExtension

Not sure how you want to incorporate that into the gem, but the line of code would presumably be needed by all redis services. I just happen to be using Suo.

ActiveJob::Locking::Serialized is enforcing serial execution correctly. Unfortunately, jobs execute serially, but out of order (not in the order they were enqueued using perform_later). Is there any way to enforce ordering (or is this a ThreadPool issue?).

• backend: async (in memory)
• activejob: 5.2.1
• rails: 5.2.1
• ruby: 2.4.0

Implementation: I have two types of jobs, both deal with the same external resource (one deletes a vsphere snapshot, the other creates one). Obviously, order matters. lock_key() locks on the hostname:

  # in a base class:
  def lock_key(*args)
    self.arguments.first
  end

  # in concrete class
  def perform(node)
    ...
  end

This works great, and I'm enqueuing them in order, eg: DeleteJob.perform_later(node), then CreateJob.perform_later(node). Jobs for the same node never execute concurrently. However, CreateJob executes first.

Any suggestions?

I was looking to extend this with another lock option (and submit a PR) but I have now spent several days trying to get the test suite to work and have not had any luck. It seems to fail different tests on different adapters, randomly. I got this hooked up to Travis (so I could run in a different environment) and that still fails.

I'm stumped about how to test this consistently. How do you make that work?

Hello,

We had a problem with our jobs not being unlocked when using the Memory Adapter. We tracked it down to the fact that @hash inside self.lock and @hash inside self.unlock are two different object IDs.

We added some debug logging to the Memory adapter:

module ActiveJob
  module Locking
    module Adapters
      class Memory < Base
        @hash = Hash.new
        @mutex = Mutex.new

        def self.lock(key)
          @mutex.synchronize do
            puts "class lock #{key} #{@hash.object_id} #{@hash}"
            if @hash[key]
              false
            else
              @hash[key] = Time.now
            end
          end
        end

        def self.unlock(key)
          puts "class unlock: #{key} #{@hash.object_id} #{@hash}"
          @mutex.synchronize do
            @hash.delete(key)
            puts "deleted #{key}"
          end
        end

Notice the first @hash has object ID 47239310005400 and the second has 47007828954140:

worker_1    | class unlock: activejoblocking:ReindexJob/Ministry 47239310005400 {}
worker_1    | deleted activejoblocking:ReindexJob/Ministry
worker_1    | 2019-09-11T18:29:42.756Z 1 TID-gqtg7pow1 ReindexJob JID-7d5a5088c3a4662c11f0742f INFO: done: 45.37 sec
web_1       | Started GET "/sidekiq/recurring-jobs/ReindexMinistries/enqueue" for 172.19.0.1 at 2019-09-11 18:32:00 +0000
web_1       | class lock activejoblocking:ReindexJob/Ministry 47007828954140 {"activejoblocking:ReindexJob/Ministry"=>2019-09-11 18:28:57 +0000}

This is why our ReindexJob is not being unlocked.

In my Rails app config/initializers/activejob_locking.rb file I have to write the following to load as adapter:

require "activejob/locking/adapters/redis-semaphore"
ActiveJob::Locking.options.adapter = ActiveJob::Locking::Adapters::RedisSemaphore

I believe the require line is necessary because the class names don't follow Rails' autoloading naming convention. The path would need to be active_job/locking/adapters/redis_semaphore. Note that Rails' ActiveJob code is in the active_job folder as expected.

I could open a PR to change this but I think it's worth a discussion — is this something that you want?

Also, if you changed this you'd want to release a major version or maybe retain the old paths for things like the adapters that people had previously required so as not to introduce a breaking change.