nickelser / suo Goto Github PK

I'm trying this as a replacement to https://github.com/dv/redis-semaphore

The issue is that with Suo the resources option works by discarding execution, instead of queueing execution.

Can Suo work with queueing blocks mandated by resources?

Right now, your gemspec has the following:

  spec.add_dependency "dalli"
  spec.add_dependency "redis"

We only use Redis as a backend, not Memcached. Adding the dependency on dalli bloats our code.

Would it be possible to make this a dynamic dependency? It seems that the Gemfile using Suo is likely to already have redis or dalli included, so can you just skip the dependencies and load them on-demand? (You'd still need them in development_dependencies for tests.) I think you'd need a note in the README about that requirement.

Hey guys,

I stumbled upon this gem while working on an updated test suite for activejob-locking. When I was using the Redis client/implementation my tests for parallel lock acquisition failed, so the lock was over-acquired. (Eg. 3 time parallel processes, on the same machine or on different machines, could acquire the same lock, each)

See: https://bit.ly/3LAT9cl

The issue is based on the Redis watch command in case there is no such key in the first place. To work around this a user must make sure to set the key (to an empty string) before any process wants to acquire the lock. This can be done at application boot or while deployments.

I set up a demo repository for reproducibility: https://github.com/Jack12816/suo-dlm-redis-bug

The Memcached client/implementation does not suffer from this issue as far as I can tell.

See Redlock for a canonical Redis DLM algorithm as an alternative, in case you care about the safety guarantee that at any given moment, only one client can hold a lock.

Proposal: Maybe we could add a hint about this in the readme?

Let's say I have an "Organization" model that the instances perform a few different rake tasks when called or triggered with whenever.
Am I able to lock the code block accordingly to the organization_id, allowing Org(id:2) to call the task even if Org(id:1) is already executing it, cause it's Org(id:2)'s first call?

Since I have a few types of rake tasks, can I lock it accordingly to the task type as well? Allowing Org(id:1) call task_1 and task_2 simultaneously?

This is similar to #4 (and related to why I discovered that in the first place).

We set acquisition_delay to 0.001 and acquisition_timeout to 0.002 because the default delay of 0.01 was causing job enqueuing to take a very long time when enqueuing thousands of jobs.

In Suo::Client::Base#retry_with_timeout there are some things I'm trying to wrap my head around with this design, so maybe you can shed some light:

      def retry_with_timeout
        start = Time.now.to_f

        retry_count.times do
          elapsed = Time.now.to_f - start
          break if elapsed >= options[:acquisition_timeout]

          synchronize do
            yield
          end

          sleep(rand(options[:acquisition_delay] * 1000).to_f / 1000)
        end
      rescue => _
        raise LockClientError
      end

Potential to never run the block
If acquisition_timeout is really low (as in our case) then the block never gets run. Shouldn't this ensure that the block gets run at least once?

Passed in block must break out
If we look at the default values:

      DEFAULT_OPTIONS = {
        acquisition_timeout: 0.1,
        acquisition_delay: 0.01,
        ...

Then wouldn't this potentially attempt to get the lock 10 times (0.01s delay for each pass until 0.1s have passed)? I see that in every use, you use break or return to exit the block that's yielded in #retry_with_timeout so that the delay isn't hit if the action succeeds. So this doesn't seem to be an issue in how it's used but could be a problem in a later use.

I'm happy to work on a PR for an update to this if either of these concerns is correct.

If a lock key has never been set, Suo::Client::Base#acquire_lock retries the acquisition rather than returning the token:

      retry_with_timeout do
          val, cas = get

          if val.nil?
            initial_set
            next
          end

        ...
      end

The next in the code above is effectively equivalent to not having that line (because the next retry would pass to the code following which clears locks). Is it necessary to make that pass? It seems that it should return token here.

Because of this, Suo won't acquire a lock unless the retry count is 2 or more. So if I configure with :acquisition_timeout less than or equal to : acquisition_delay, even though @retry_count gets the ceil of the dividend, 1 try isn't enough to acquire a lock.

I'm happy to open a PR to change this, but I want to make sure I understand this correctly and I'm not missing something important.

The message reads:

You can remove require 'dalli/cas/client' as this code has been rolled into the standard 'dalli/client'.

Would it be appropriate to change this require to dally/client instead?

I can add a PR if so.

Thanks!

Please remove line #19 from your gemspec file. Its boilerplate. You have no real global executables in you bin directory.