Incremental lock

[alex88]

Hi there,

I've actually this code:

mutex = RedisMutex.new lock_key(feed, user), block: 300, sleep: 0.5, expire: 300
if mutex.locked?
  Rails.logger.info 'Another feed is in processing, waiting for that and returning'
  # Another feed is running, just wait for the lock to expire and end
  mutex.unlock if mutex.lock
  Rails.logger.info 'Other feed finished, returning'
else
  # Not locked, aquire the lock and process the feed
  Rails.logger.info 'No feed are processing, acquiring lock'
  mutex.with_lock do
    Rails.logger.info 'feed lock acquired, processing feed'
    teams = get_teams(feed)
    teams.each { |team| sync_team(team, feed, user) }
    Rails.logger.info 'feed finished, releasing lock'
  end
end

running in a Sidekiq job in Heroku, this works fine, except when due a scale down the job is being SIGKILLed since the teams.each loop isn't exited in 10 seconds (heroku default KILL timeout).

Is there a way, inside the with_lock thing, to increase the expire time of the lock? Something like:

mutex.with_lock do
  Rails.logger.info 'feed lock acquired, processing feed'
  teams = get_teams(feed)
  teams.each do |team|
    sync_team(team, feed, user)
    mutex.lock_until(now + 10 .seconds)
  end
  Rails.logger.info 'feed finished, releasing lock'
end

this lets me lower down the expire time to 10 seconds and incrementally increase the expire time so that if the process is killed the lock is still active only for at max 10 seconds, not the remaining time to 300 seconds.

[kenn]

I see what you mean, but let me get things straight first:

if mutex.locked?
  mutex.unlock if mutex.lock
else
  ...

I wouldn't do this - for every command you add this way, you're creating more cases for race conditions. It's not worth it at all. Just let redis-mutex handle it and follow the idiom as documented.

begin
  RedisMutex.with_lock(options) do
    ...
  end
rescue RedisMutex::LockError
  ...
end

and now, if your concern is about processes SIGKILLed while shutting down on Heroku, lock extension won't solve it - your process will be SIGKILLed anyway when it goes beyond 10 seconds. You have two viable options:

• Split the job into multiple smaller jobs that ends within 10 seconds (e.g. single iteration per job)
• Redesign the job in an idempotent manner (a.k.a. retry-safe). It is a standard practice for background jobs anyway, because your processes can be killed at any time, for instance, by OOM killer.

Lock extension sounds like an interesting idea per se, but for this particular use case, I don't think that's going to cut it.

[alex88]

For the first code part, it's not what I need, if the mutex is already locked, I want just to wait and when it's been released go on as the worker waiting for the job has done its work, so skip the part of the code that runs when it's actually holding the lock.

About the second part, I've already tried to make smaller jobs but there are many conditions that can't be met asynchronously.

I'm not saying about not being killed, I'm saying that my current situation is this:

# Timeout is e.g. 10 minutes, imagine a job runs for 5 minutes usually
T0 - Job starts, locks mutex
T5 - Job finishes, release locks, everything fine

T0 - Job starts, locks mutex
T1 - Job is SIGKILLed
T10 - Any other job can take the mutex again

so it wastes 9 minutes waiting for lock to expire, what I'm trying to achieve is this

# Timeout is now 2 minutes
# Best case still works the same, job starts and unlocks, fine
# Worse case with sigkill

T0 - Job starts, locks mutex
T1 - Job still running, sets its own lock timeout to T3 (now + 2 minutes)
T2 - Job still running, sets its own lock timeout to T4 (now + 2 minutes)
T3 - Job is killed, mutex not unlocked
T4 - mutex expired

in this case I've much less time wasted waiting lock to expire and I can be sure that the lock doesn't expire during my job work since I'm "renewing" it's expire time

[kenn]

I still don't get it. Let's clarify the terminology first: SIGKILL is a UNIX signal that is sent to force quit a process. Your phrase "Job is SIGKILLed" is just impossible - only UNIX processes can be SIGKILLed, and SIGKILL should never be called regularly - it's basically for panic mode only.

What exactly are you killing? Process? Thread (because you mentioned Sidekiq)? If so, how?

Why are you SIGKILLing a job when you finish normally? Workers should be returned to the worker pool when they finish, not be killed. And redis-mutex uses ensure to unlock, which is even called with SIGTERM, SIGINT, etc. but not with SIGKILL.

As long as you don't SIGKILL regularly, lock is released when the job is done, and everything should be fine.

[alex88]

Sorry for that, so heroku runs dynos, each dyno runs a sidekiq worker process, sidekiq runs a job with the code above inside.

If the dyno has to be shut down due a scale down, heroku first sends a SIGTERM, if the process doesn't exit within 10 seconds (because it's in a loop) heroku sends a SIGKILL.

So redis-mutex, sidekiq or whatever, doesn't unlock the mutex nor gracefully exits the process, in fact every time a dyno has to be shut down and it's inside that loop, lock isn't released and all other workers waiting for that lock are idle for a lot of minutes.

That's because, since the loop could take some time I have to make an high timeout time and if the worker is killed and the lock not unlocked, all the remaining time is wasted.

[alex88]

I don't want to prevent the process to be SIGKILLed or anything like that, I want, if possible, to be able to set a low default lock expire time and increase it while in the loop, something like "hey, I'm still running, keep the mutex locked for another minute" and so on.

If it gets killed and for some reason it doesn't release the lock, other workers has just to wait 1 minute or less

[kenn]

OK, now we're on the same page, finally. Thanks for clarification. 👍

Now my question is, if SIGTERM is first sent to the worker that holds the lock, then it should trigger the ensure block, which in turn calls unlock immediately. 10 seconds should be enough - do you know if and why unlock is not processed in that case? Don't you have some code that traps signals yourself?

[kenn]

Ah, maybe it's because Sidekiq uses threads? Signals will be sent to the main thread, so it depends on how Sidekiq handles it internally.

[kenn]

Hmm, it looks like ensure gets triggered when thread.terminate is called, which is what Sidekiq seems to use. If so, everything should be fine. Can you check why the ensure / unlock is not called on your environment? I don't have a project that uses Sidekiq right now.

th1 = Thread.new do
  begin
    sleep 10
  ensure
    p "this will be displayed"
  end
end

sleep 0.1
th1.terminate

# => "this will be displayed"

[kenn]

Here's the proof that the mutex is automatically unlocked upon Thread#terminate - try it yourself. So I assume that the cause is in either Sidekiq or your environment.

require 'redis-mutex'

RedisClassy.redis = Redis.current

th1 = Thread.new do
  RedisMutex.with_lock :lock1 do
    sleep 10
  end
end

sleep 0.1
th1.terminate
sleep 0.1
p RedisMutex.new(:lock1).locked? # => false
p RedisMutex.keys                # => [] which means the lock key has been removed successfully

[alex88]

I know only that some times the mutex isn't unlocked and other workers wait for it, after timeout it works fine, maybe sidekiq is acting weird or something like that. I've also tried to handle the signal by myself, no luck.

Maybe it can be something with mutex.unlock if mutex.lock? But that should work, as soon as mutex.lock returns true because it waited and the mutex is unlocked, I unlock it

[kenn]

Found a timely blog post:

• http://jvns.ca/blog/2015/11/27/why-rubys-timeout-is-dangerous-and-thread-dot-raise-is-terrifying/
• http://stackoverflow.com/questions/1559255/whats-wrong-with-using-thread-abort
• https://www.reddit.com/r/programming/comments/3ui1sw/why_rubys_timeout_is_dangerous_and_threadraise_is/cxfg98b

The only correct way to terminate a thread is to have it cooperate and make the cooperation yourself. That means: have a "cancel" flag known to the thread and make code in the thread respect it. There is no magic and there is no way out.

So, Thread#terminate is flawed by design. In other words, it's a limitation of thread-based workers like Sidekiq. There should be a way to send messages between threads, then wind down cooperatively.

[alex88]

I think I'll just use mutex lock with a low timeout and manually use redis client to increase the timestamp on each loop cycle for that mutext key, in that way it should work fine and keep the mutex locked during the loop even in the worse case and it doesn't have to wait 10 minutes if thread gets terminated

[kenn]

You can do that, but keep in mind the implication of the flaw is far beyond mutex - it can bork any ensure statements in the stack of your app, which can be fatal if your job does state mutation or resource alloc/dealloc (e.g. file writes / database writes).

The correct way to handle it is use Thread.handle_interrupt in your code.

https://www.quora.com/How-does-threading-work-in-Ruby-2-0-and-how-is-this-different-from-Ruby-1-9-2

def your_method
  chunks_of_work = big_work.split
  Thread.handle_interrupt(RuntimeError => :never) do
    chunks_of_work.each do
      # do your job here in small chunks (should finish in 1-10 seconds to meet the grace period)
      ...
      # at the end of each iteration, check if you received the message from main thread
      if Thread.pending_interrupt?
        Thread.handle_interrupt(RuntimeError => :immediate) # it's safe to exit here
      end
    end
  end
end

This is a limitation of multithread programming in general, not only on Ruby - housekeeping concern (of frameworks / libraries) leaks into your app code. Fortunately, there's a solution in Ruby.

[alex88]

Sorry for the delay,

It worked fine as you did, I just had to change from RuntimeError to Exception as probably there's another exception that is sent to the thread

Thanks a lot for helping!