Lock#try_lock should not raise exception? #160

Closed
niv opened this Issue Sep 2, 2015 · 22 comments

Comments

Projects
None yet
2 participants
@niv

niv commented Sep 2, 2015

Hi,

I'm having some doubts about this line:

raise "Lock instance `#{key}' already locked" if @locked

It throws an exception when calling try_lock from the same lock twice. Shouldn't it just return false/true depending on if the current caller already holds the lock?

Cheers

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Sep 2, 2015

Owner

Hi @niv,

It could, but that doesn't make it better I think. If the same thread calls try_lock on the same mutex twice, something is probably funny in the caller's code. What's your usecase?

Owner

nviennot commented Sep 2, 2015

Hi @niv,

It could, but that doesn't make it better I think. If the same thread calls try_lock on the same mutex twice, something is probably funny in the caller's code. What's your usecase?

@niv

This comment has been minimized.

Show comment
Hide comment
@niv

niv Sep 2, 2015

Ah, you might (not) enjoy this.

I'm trying to reuse/extend NoBrainer::Lock for my own nefarious purposes. I basically want to hold locks alive from http requests; for this, I've written this piece of code:

class ReclaimableLock < NoBrainer::Lock
  def initialize key, token
    super(key)
    self.token = Digest::SHA1.hexdigest(token)

    @locked = NoBrainer.run do |r|
      selector.ne(nil)
    end
  end
end

It's not really tested or anything, just an experiment (I probably need to check for expiry too). I want to be able to reinstance a lock including the lock key (based on a secret the locker knows).

In the actual service, I do this:

lock = ReclaimableLock.new "characters:#{@character.id}", @server.api_key
if lock.try_lock
  # lock persists
end

Not sure if that is the best way to go about it - I probably need something like Lock#locked_by_me?. I can certainly understand that calling try_lock twice from the same context isn't good.

niv commented Sep 2, 2015

Ah, you might (not) enjoy this.

I'm trying to reuse/extend NoBrainer::Lock for my own nefarious purposes. I basically want to hold locks alive from http requests; for this, I've written this piece of code:

class ReclaimableLock < NoBrainer::Lock
  def initialize key, token
    super(key)
    self.token = Digest::SHA1.hexdigest(token)

    @locked = NoBrainer.run do |r|
      selector.ne(nil)
    end
  end
end

It's not really tested or anything, just an experiment (I probably need to check for expiry too). I want to be able to reinstance a lock including the lock key (based on a secret the locker knows).

In the actual service, I do this:

lock = ReclaimableLock.new "characters:#{@character.id}", @server.api_key
if lock.try_lock
  # lock persists
end

Not sure if that is the best way to go about it - I probably need something like Lock#locked_by_me?. I can certainly understand that calling try_lock twice from the same context isn't good.

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Sep 2, 2015

Owner

I don't understand what you are trying to accomplish.

I want to be able to reinstance a lock including the lock key (based on a secret the locker knows).

Can you clarify what that means? Can you describe the behavior of what you'd like?

Owner

nviennot commented Sep 2, 2015

I don't understand what you are trying to accomplish.

I want to be able to reinstance a lock including the lock key (based on a secret the locker knows).

Can you clarify what that means? Can you describe the behavior of what you'd like?

@niv

This comment has been minimized.

Show comment
Hide comment
@niv

niv Sep 2, 2015

I want to add a locking mechanism to NoBrainer models, so that only one authenticated client can make changes to them (protected with a simple guard in the API handler). Those locks need to expire after a time automatically, and be unlockable (by the authenticated client) before that.

I was going to roll my own, but I figured I could use NoBrainer::Lock the same way with minimum changes.

niv commented Sep 2, 2015

I want to add a locking mechanism to NoBrainer models, so that only one authenticated client can make changes to them (protected with a simple guard in the API handler). Those locks need to expire after a time automatically, and be unlockable (by the authenticated client) before that.

I was going to roll my own, but I figured I could use NoBrainer::Lock the same way with minimum changes.

@niv

This comment has been minimized.

Show comment
Hide comment
@niv

niv Sep 2, 2015

Example usecase:

(client 1) POST /model/1/lock ? duration=5min -> locked
(client 1) PUT /model/1/changes -> OK
(client 2) POST /model/1/lock -> 403 locked by someone else
(client 2) PUT /model/1/changes -> 403 locked by someone else
(client 1) POST /model/1/unlock -> OK unlocked

niv commented Sep 2, 2015

Example usecase:

(client 1) POST /model/1/lock ? duration=5min -> locked
(client 1) PUT /model/1/changes -> OK
(client 2) POST /model/1/lock -> 403 locked by someone else
(client 2) PUT /model/1/changes -> 403 locked by someone else
(client 1) POST /model/1/unlock -> OK unlocked

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Sep 2, 2015

Owner

I see. Let me come up with something.

Owner

nviennot commented Sep 2, 2015

I see. Let me come up with something.

@niv

This comment has been minimized.

Show comment
Hide comment
@niv

niv Sep 2, 2015

Thank you very much!

niv commented Sep 2, 2015

Thank you very much!

@nviennot nviennot closed this in 88a526c Sep 2, 2015

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Sep 2, 2015

Owner

I've pushed an implementation of NoBrainer::ReentrantLock. Here are the new semantics:

  • you can call try_lock/unlock many times on the same lock. A counter is saved on the db to keep track of how many unlocks are needed before unlocking for other instances.
  • you cannot unlock more times than try_lock has been called (so it's not a really a semaphore).
  • you can provide an :instance_token in new(...) to name your instances.

Here's a usage that you might like for your usecase:

class SomeController
  def ownership_lock(model)
    NoBrainer::ReentrantLock.new("ownership:models:#{model.id}",
                                 :instance_token => current_user.id,
                                 :timeout => 0)
    # specifying timeout = 0 will make all the call to lock/synchronize raise if
    # the lock is not available.
  end

  def action_lock
    # careful, calling try_lock multiple times will require multiple unlock calls.
    if ownership_lock(model).lock
      ...
    else
      ...
    end
  end

  def action_unlock
    ownership_lock(model).unlock
  end

  def update_model
    ownership_lock(model).synchronize do
      model.update(...)
    end
  end
end

I will write the documentation later, but this should help you getting where you need to get.

Owner

nviennot commented Sep 2, 2015

I've pushed an implementation of NoBrainer::ReentrantLock. Here are the new semantics:

  • you can call try_lock/unlock many times on the same lock. A counter is saved on the db to keep track of how many unlocks are needed before unlocking for other instances.
  • you cannot unlock more times than try_lock has been called (so it's not a really a semaphore).
  • you can provide an :instance_token in new(...) to name your instances.

Here's a usage that you might like for your usecase:

class SomeController
  def ownership_lock(model)
    NoBrainer::ReentrantLock.new("ownership:models:#{model.id}",
                                 :instance_token => current_user.id,
                                 :timeout => 0)
    # specifying timeout = 0 will make all the call to lock/synchronize raise if
    # the lock is not available.
  end

  def action_lock
    # careful, calling try_lock multiple times will require multiple unlock calls.
    if ownership_lock(model).lock
      ...
    else
      ...
    end
  end

  def action_unlock
    ownership_lock(model).unlock
  end

  def update_model
    ownership_lock(model).synchronize do
      model.update(...)
    end
  end
end

I will write the documentation later, but this should help you getting where you need to get.

@niv

This comment has been minimized.

Show comment
Hide comment
@niv

niv Sep 2, 2015

I'll try it ASAP.

Thank you for your outstanding timing and help.

niv commented Sep 2, 2015

I'll try it ASAP.

Thank you for your outstanding timing and help.

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Sep 2, 2015

Owner

My pleasure :)

Let me know how it goes

Owner

nviennot commented Sep 2, 2015

My pleasure :)

Let me know how it goes

@niv

This comment has been minimized.

Show comment
Hide comment
@niv

niv Sep 2, 2015

Tried it really quickly and found a couple of oddities.

user1 reentrant_lock.try_lock expire: 15.seconds.to_i
user1 reentrant_lock.try_lock expire: 15.seconds.to_i
user1 reentrant_lock.try_lock expire: 15.seconds.to_i
# lock_count now 3, wait for it to expire
user2 reentrant_lock.try_lock
# user2 still sees that lock_count: 3 since the document is never deleted
# even though the lock itself had expired

Also, I think in your example you meant user.try_lock, instead of lock, right? ReentrantLock#lock always errors for me with "lock unavailable".

Edit: #lock errors because of timeout: 0, since it never reaches inside the while loop that hits try_lock.

niv commented Sep 2, 2015

Tried it really quickly and found a couple of oddities.

user1 reentrant_lock.try_lock expire: 15.seconds.to_i
user1 reentrant_lock.try_lock expire: 15.seconds.to_i
user1 reentrant_lock.try_lock expire: 15.seconds.to_i
# lock_count now 3, wait for it to expire
user2 reentrant_lock.try_lock
# user2 still sees that lock_count: 3 since the document is never deleted
# even though the lock itself had expired

Also, I think in your example you meant user.try_lock, instead of lock, right? ReentrantLock#lock always errors for me with "lock unavailable".

Edit: #lock errors because of timeout: 0, since it never reaches inside the while loop that hits try_lock.

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Sep 2, 2015

Owner

Oops yes that's a bug. I'll fix that in ~1hour. Sorry about that

Owner

nviennot commented Sep 2, 2015

Oops yes that's a bug. I'll fix that in ~1hour. Sorry about that

@niv

This comment has been minimized.

Show comment
Hide comment
@niv

niv Sep 2, 2015

Don't worry about it! It's late over here so I will revisit after sleep at the soonest.

Cheers.

niv commented Sep 2, 2015

Don't worry about it! It's late over here so I will revisit after sleep at the soonest.

Cheers.

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Sep 3, 2015

Owner

alright, that should be fixed :) :)

Owner

nviennot commented Sep 3, 2015

alright, that should be fixed :) :)

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Sep 3, 2015

Owner

Also, I made a choice about the semantics which is debatable. Say that you hold an instance of the lock its count is 5 or whatever. You let the lock expire (maybe the machine died?). The next day, with the same lock instance, you proceed to lock. Should the counter be 6, or 1?.
I've changed the behavior to reset the counter when acquiring an expired lock (so it would be 1), as it makes the most sense in terms of failure recovery.

Owner

nviennot commented Sep 3, 2015

Also, I made a choice about the semantics which is debatable. Say that you hold an instance of the lock its count is 5 or whatever. You let the lock expire (maybe the machine died?). The next day, with the same lock instance, you proceed to lock. Should the counter be 6, or 1?.
I've changed the behavior to reset the counter when acquiring an expired lock (so it would be 1), as it makes the most sense in terms of failure recovery.

@niv

This comment has been minimized.

Show comment
Hide comment
@niv

niv Sep 3, 2015

This is looking good! And for what it's worth, I agree with your reasoning that expired locks should reset to zero.

Two minor things though:

  • ReentrantLock#lock still raises a error though if timeout is 0. Is this desired behaviour? Should it not be semantically the same as try_lock, except raise an exception when the lock is held by someone else?
  • Is there a sane way to make #try_lock not increment lock_count on each call (maybe as an option), in case all you want to track is lock status, not count, and be able to unlock it in one go with #unlock? Does that make sense?

niv commented Sep 3, 2015

This is looking good! And for what it's worth, I agree with your reasoning that expired locks should reset to zero.

Two minor things though:

  • ReentrantLock#lock still raises a error though if timeout is 0. Is this desired behaviour? Should it not be semantically the same as try_lock, except raise an exception when the lock is held by someone else?
  • Is there a sane way to make #try_lock not increment lock_count on each call (maybe as an option), in case all you want to track is lock status, not count, and be able to unlock it in one go with #unlock? Does that make sense?
@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Sep 3, 2015

Owner
  • I've pushed a fix for the timeout = 0 problem.
  • So what you are suggesting is similar to the original lock. I don't see how you would use it without seeing problematic races. If you could show a code example of how you would use it, that'd be great.
    Mean while, here's how you could do:
class SomeController
  def action_lock
    ownership_lock(model).synchronize do
      model.reload
      if model.owner != current_user
        model.update(:owner => current_user)
        ownership_lock(model).lock
      end
    end
  end

  def action_unlock
    ownership_lock(model).synchronize do
      model.reload
      if model.owner == current_user
        model.update(:owner => nil)
        ownership_lock(model).unlock
      end
    end
  end

  def update_model
    ownership_lock(model).synchronize do
      model.update(...)
    end
  end
end
Owner

nviennot commented Sep 3, 2015

  • I've pushed a fix for the timeout = 0 problem.
  • So what you are suggesting is similar to the original lock. I don't see how you would use it without seeing problematic races. If you could show a code example of how you would use it, that'd be great.
    Mean while, here's how you could do:
class SomeController
  def action_lock
    ownership_lock(model).synchronize do
      model.reload
      if model.owner != current_user
        model.update(:owner => current_user)
        ownership_lock(model).lock
      end
    end
  end

  def action_unlock
    ownership_lock(model).synchronize do
      model.reload
      if model.owner == current_user
        model.update(:owner => nil)
        ownership_lock(model).unlock
      end
    end
  end

  def update_model
    ownership_lock(model).synchronize do
      model.update(...)
    end
  end
end
@niv

This comment has been minimized.

Show comment
Hide comment
@niv

niv Sep 3, 2015

Here's how my current code looks like. @character and @server are both set someplace else, @server being the authenticated user, @character the current resource. POSTing to /lock simply locks @character to the current @server, and similarily /unlock will free it for other users to acquire.

The other side repeatedly calls /lock to refresh the lock until such time it no longer needs it and calls /unlock (or crashes and the lock expires by itself).

I don't understand how this would give a race condition though.

I have two (workaround) rql blocks in my code to
a) retrieve the time until expiry and pass it on to the user, and
b) to to set lock_count to 1 before unlocking - basically eating the repeated calls to /lock.

... (on @character)
  def reentrant_lock token
    NoBrainer::ReentrantLock.new "reentrant_lock:#{self.class.name.to_s}:#{self.id}",
      instance_token: token,
      timeout: 0
  end
...

      post :lock do
        lock = @character.reentrant_lock(@server.api_key)

        r = if lock.try_lock expire: 5.minutes.to_i
          NoBrainer.run {
            NoBrainer::ReentrantLock.rql_table.filter(key: lock.key).
              pluck(:expires_at)
            }.first
        else
          false
        end

        {
          "lock": r
        }
      end

      post :unlock do
        lock = @character.reentrant_lock(@server.api_key)

        NoBrainer.run {
          NoBrainer::ReentrantLock.rql_table.filter {|row|
            row[:key].eq(lock.key) && row[:lock_count].gt(0)
          }.update(lock_count: 1)
        }

        {
          "unlock": (begin
              lock.unlock
              true
            rescue NoBrainer::Error::LostLock, NoBrainer::Error::LockUnavailable
              false
            end)
        }
      end

Sorry if I'm being dense and missing somthing.

niv commented Sep 3, 2015

Here's how my current code looks like. @character and @server are both set someplace else, @server being the authenticated user, @character the current resource. POSTing to /lock simply locks @character to the current @server, and similarily /unlock will free it for other users to acquire.

The other side repeatedly calls /lock to refresh the lock until such time it no longer needs it and calls /unlock (or crashes and the lock expires by itself).

I don't understand how this would give a race condition though.

I have two (workaround) rql blocks in my code to
a) retrieve the time until expiry and pass it on to the user, and
b) to to set lock_count to 1 before unlocking - basically eating the repeated calls to /lock.

... (on @character)
  def reentrant_lock token
    NoBrainer::ReentrantLock.new "reentrant_lock:#{self.class.name.to_s}:#{self.id}",
      instance_token: token,
      timeout: 0
  end
...

      post :lock do
        lock = @character.reentrant_lock(@server.api_key)

        r = if lock.try_lock expire: 5.minutes.to_i
          NoBrainer.run {
            NoBrainer::ReentrantLock.rql_table.filter(key: lock.key).
              pluck(:expires_at)
            }.first
        else
          false
        end

        {
          "lock": r
        }
      end

      post :unlock do
        lock = @character.reentrant_lock(@server.api_key)

        NoBrainer.run {
          NoBrainer::ReentrantLock.rql_table.filter {|row|
            row[:key].eq(lock.key) && row[:lock_count].gt(0)
          }.update(lock_count: 1)
        }

        {
          "unlock": (begin
              lock.unlock
              true
            rescue NoBrainer::Error::LostLock, NoBrainer::Error::LockUnavailable
              false
            end)
        }
      end

Sorry if I'm being dense and missing somthing.

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Sep 3, 2015

Owner

You are not showing the code related to the character updates, so it's hard to show where these races could be (the (client 1) PUT /model/1/changes -> OK stuff). So what would the /changes API look like?

Other than that, the unlock update code you show has issues:
a) you are touching the lock even if the user no longer have it
b) even if you add a filter on the api_key, an arbitrary number of db operations could be executed between filter and update: by the the time you set lock_count to 1, the lock might have been changed to a different owner

Owner

nviennot commented Sep 3, 2015

You are not showing the code related to the character updates, so it's hard to show where these races could be (the (client 1) PUT /model/1/changes -> OK stuff). So what would the /changes API look like?

Other than that, the unlock update code you show has issues:
a) you are touching the lock even if the user no longer have it
b) even if you add a filter on the api_key, an arbitrary number of db operations could be executed between filter and update: by the the time you set lock_count to 1, the lock might have been changed to a different owner

@niv

This comment has been minimized.

Show comment
Hide comment
@niv

niv Sep 3, 2015

You're right, i missed that completely. I'll rework and get back to you tomorrow.

niv commented Sep 3, 2015

You're right, i missed that completely. I'll rework and get back to you tomorrow.

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Sep 3, 2015

Owner

Actually, my code doesn't work either. It would work if "Say that you hold an instance of the lock its count is 5 or whatever. You let the lock expire (maybe the machine died?). The next day, with the same lock instance, you proceed to lock. Should the counter be 6, or 1?." would return 6, and not 1.

That's because there is no way to tell if a lock was recovered or not. When it's recovered, the model.owner should be set to nil.

Owner

nviennot commented Sep 3, 2015

Actually, my code doesn't work either. It would work if "Say that you hold an instance of the lock its count is 5 or whatever. You let the lock expire (maybe the machine died?). The next day, with the same lock instance, you proceed to lock. Should the counter be 6, or 1?." would return 6, and not 1.

That's because there is no way to tell if a lock was recovered or not. When it's recovered, the model.owner should be set to nil.

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Sep 3, 2015

Owner

I've switched back to the non-reset of the counter when recovering a lock from the same instance. It makes coding easier (no need to deal with special recovery code for most use cases): 7f166a9

Owner

nviennot commented Sep 3, 2015

I've switched back to the non-reset of the counter when recovering a lock from the same instance. It makes coding easier (no need to deal with special recovery code for most use cases): 7f166a9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment