Redlock::Client.lock returns false when there is a connection issue

[marcilioLemos]

Hello, everyone. I want to use redlock to implement a rate limit so that duplicate requests within a certain time frame are dropped. According to the documentation, if an attempt to acquire a lock fails, the lock method returns false. At the same time, after rescuing exceptions, lock returns false too. Such behavior, in my opinion, is dangerous and an anti-pattern. If a client receives a false response from lock, there is no guarantee that this is because the lock has already been acquired. In the scenario described above , this behavior could result in the drop of all requests if the connection to the redis servers fails (once that Redis::BaseConnectionError is rescued).

[panozzaj]

Thanks for sharing this issue. I am a new user of this library and was scanning through issues and saw this. Wanted to share how I think this might affect us.

Currently a third-party vendor is intermittently sending us duplicate requests in short succession and we want to only process those requests once since they can have a side effect in our system (creating new records). Currently we are creating duplicate new records since the requests happen so quickly. We have some code that has the following logic to more correctly handle the duplicate requests (pseudocode):

redlock_client.lock('unique_id_from_request', 5_000) do |lock|
  if lock
    # process the request...
  else
    # assume that this is a duplicate request and drop
    return
  end
end

With the current behavior in lib/redlock/client.rb#lock of rescue Redis::BaseConnectionError => e, we would drop requests in the event that there is a Redis connection issue, since lock would return false. This could happen due to misconfiguration or if our Redis instance was (temporarily) down.

I agree with @marcilioLemos that a less surprising (to me) behavior would be to raise an error in the case of connection failure. Not connecting is different from a lock already being acquired.

Would you be open to a pull request for this behavior change? I think we might create a local fork to handle this and will pass along in a PR if that would be helpful. I'm thinking it might be a lock method contract change, which might necessitate increasing the revision. Thanks!

[marcilioLemos]

Thank you for sharing your thoughts on this issue. I am available to assist with the implementation or revision of the PR. On the other hand, it appears that this repository has not been maintained for long.

[leandromoreira]

It's stable with few changes here and there but I think it's still active, go ahead and do your PR any thoughts @maltoe ?

[LyleDavis]

@leandromoreira this is an ugly suggestion so apologies in advance, but if the concern is API changes an alternate lock method that distinguishes between the failure to acquire the lock and the connection error would give the flexibility without breaking current contracts.

The reason I suggest it is that in some scenarios it may actually be correct to treat a connection error and a failure to acquire a lock as the same, for example to allow retries to attempt to retrieve it or if you're merely locking for normal set of updates to records that are only allowed one at a time.

But in other scenarios you may not want any retries when a lock fails to acquire - in the situation that @panozzaj shared above then retries would potentially allow breaking of rate limits, and another scenario would be if you're using redlock to protect against duplicate running of background jobs in “at least once” delivery models like SQS. In both of these cases you would still need to retry if there is a connection error.

[panozzaj]

This should now be fixed by #110, motion to close.