Nil timeout not behaving as expected

[millerjs]

Bug Description

The README says lock_timeout: nil # lock indefinitely, but https://github.com/mhenrixon/sidekiq-unique-jobs/blob/620fe7503f1d0395cad337d2e45681504913c7ef/lib/sidekiq_unique_jobs/locksmith.rb#L273-L277 executes a non blocking call if config.timeout is nil.

Are these two things contradictory?

Expected behavior When lock_timeout is nil, lock acquisition will block indefinitely

Current behavior When lock_timeout is nil, lock acquisition is non-blocking

Worker class

class UniqueUntilAndWhileExecutingTestWorker
  include Sidekiq::Worker
  sidekiq_options queue: :medium, lock: :until_and_while_executing

  def perform(timestamp)
    Rails.logger.info("performing")
  end
end

Additional context

I would expect a spec like this to hang for "indefinitely"

  context "when lock_timeout is nil" do
    let(:lock_timeout) { nil }

    let(:infinite_brpoplpush) { proc { sleep(1_000_000) } }

    it "waits indefinitely" do
      allow(locksmith_one).to receive(:brpoplpush).with(anything, nil, &infinite_brpoplpush)
      locksmith_one.execute {}
      raise "We should have never gotten here"
    end
  end

original comment https://github.com/mhenrixon/sidekiq-unique-jobs/issues/617#issuecomment-883458295

[mhenrixon]

In the redis world a value of 0 would block indefinitely and nil would crash with invalid argument.

I found myself wanting zero as default (don't wait). To be perfectly honest though, there is absolutely no circumstances I can think of that would warrant to wait indefinitely. At least not for what this gem does so perhaps zero and nil should be treated the same and any positive integer would block.

[millerjs]

@mhenrixon gotcha, that makes sense.

We're definitely not setting nil, but I've got no idea if other people want that. The fact that nil is unexpectedly non-blocking suggests it's not a highly used feature 😁

[mhenrixon]

The way I was thinking was that if the key is not set it would mean a default of 0 and explicitly setting the key to nil would allow someone to forcefully wait indefinitely.

I left it in there as a last resort but I should probably restrict it to a positive integer value.