Implement ::lock! and #lock! methods

[outoftime]

Howdy -- I love the library, very nice and clean implementation of the approach laid out in the Redis docs. For our particular use case, it would be most useful to have a method that returns the value of the block passed if the lock is successfully acquired, and raises an error if not. In this patch I've added a #lock! method to do just that. Thanks for taking a look! Mat

[kenn]

Great idea, and I agree that exception-based control flow is often more useful than conditionals. Also I like the bang method - akin to ActiveRecord's save / save! or create / create!, etc.

However, there's one thing that bothers me... that lock! only works with a block given, and that breaks parity with the lock method, which seems confusing. At least the API should require a block rather than letting block_given? be false and returns nil as a result. Also, if we always need a block, I don't see any point in having the instance method. The impedance mismatching here is nontrivial.

I think exception-based control flow has the potential to be the default behavior in the future major releases (however exceptions are significantly slow and thus it has performance drawbacks in use cases with frequent lock contentions), but first we need to give more thoughts on the consistency / compatibility, etc.

What do you think?

[outoftime]

Thanks for taking a look! I'm not clear how the lock! method requires a block, though -- the bit about returning the value of the block evaluation does, of course, but in terms of throwing an error if the lock can't be obtained, seems like that applies regardless of whether a block is provided?

On Oct 12, 2012, at 7:25, Kenn Ejima notifications@github.com wrote:

Great idea, and I agree that exception-based control flow is often more useful than conditionals. Also I like the bang method - akin to ActiveRecord's save / save! or create / create!, etc.

However, there's one thing that bothers me... that lock! only works with a block given, and that breaks parity with the lock method, which seems confusing. At least the API should require a block rather than letting block_given? be false and returns nil as a result. Also, if we always need a block, I don't see any point in having the instance method. The impedance mismatching here is nontrivial.

I think exception-based control flow has the potential to be the default behavior in the future major releases (however exceptions are significantly slow and thus it has performance drawbacks in use cases with frequent lock contentions), but first we need to give more thoughts on the consistency / compatibility, etc.

What do you think?

— Reply to this email directly or view it on GitHubhttps://github.com/kenn/redis-mutex/pull/2#issuecomment-9367706.

[kenn]

I'm not clear how the lock! method requires a block, though -- the bit about returning the value of the block evaluation does, of course, but in terms of throwing an error if the lock can't be obtained, seems like that applies regardless of whether a block is provided?

If it doesn't return a lock object, how do you unlock it once your job is done?

[kenn]

Also, the following code passes a block to the lock method, which means it's unlocked automatically and there's no critical section outside the lock! method.

acquired = lock { result = yield if block_given? }

[kenn]

I realized that my mistake was that I added ::lock as a shortcut to new and lock, as it actually does unlock too - it must be the root cause of the confusion.

It should have been something like ::with_lock (or ::with_lock!) that requires a block, and the block syntax should be removed from the #lock instance method as it's pointless (we use instance method when we need finer control for unlocking).

That way, we could stress the difference in semantics.

I'll probably add ::with_lock with your implementation (raise exception on lock failure, returns the value of the block), deprecate the block syntax in the instance method (I hope no one is using), and deprecate ::lock (everyone's using it so we should leave it available for a longer period).

How does that sound?

[outoftime]

Makes perfect sense to me!

On Oct 12, 2012, at 16:17, Kenn Ejima notifications@github.com wrote:

I realized that my mistake was that I added ::lock as a shortcut to new and lock, as it actually does unlock too - it must be the root cause of the confusion.

It should have been something like ::with_lock (or ::with_lock!) that requires a block, and the block syntax should be removed from the

lockinstance method as it's pointless (we use instance method when we

need finer control for unlocking).

That way, we could stress the difference in semantics.

I'll probably add ::with_lock with your implementation (raise exception on lock failure, returns the value of the block), deprecate the block syntax in the instance method (I hope no one is using), and deprecate ::lock(everyone's using it so we should leave it available for a longer period).

How does that sound?

— Reply to this email directly or view it on GitHubhttps://github.com/kenn/redis-mutex/pull/2#issuecomment-9379758.

[kenn]

I've just released v2.0 - please check if it works for you!

[outoftime]

Thanks! I'm actually traveling but will definitely take a look next week when I'm back.

On Oct 12, 2012, at 23:29, Kenn Ejima notifications@github.com wrote:

I've just released v2.0 - please check if it works for you!

— Reply to this email directly or view it on GitHubhttps://github.com/kenn/redis-mutex/pull/2#issuecomment-9395317.