Update behavior of lock to behave closer to redis lock

[taylor-cedar]

I updated a project to use fakeredis and noticed some tests failing because the fakeredis lock implementation doesn't actually behave like the redis lock. If you create 2 locks with the same key, it doesn't actually fail to acquire the lock.

I added more tests and updated the implementation to handle multiple locks on the same key.

[taylor-cedar]

I originally had an implementation to sleep and wait for the lock to be released, but eventually changed it to an error because blocking would likely wait forever or until the blocking_timeout. In either case I think it's an error and it's better to expose an exception instead of an infinite wait.

[coveralls]

Coverage increased (+98.4%) to 98.432% when pulling 4a24a1f on cedar-team:make-fakelock-behave-closer-to-redis-lock into b87e885 on jamesls:master.

[bmerry]

Thanks! From a quick glance this looks good, but I'll need to come back to this in a few weeks when I'm planning to do some overhaul on fakeredis for thread safety. At present this won't be thread-safe (which is something a lock really should be) because the interactions with fakeredis functions it uses aren't thread-safe.

[taylor-cedar]

@bmerry Yes, this definitely isn't thread safe (as you said), but it wasn't before anyways. Can you merge it in and make it a minor version release for now? This isn't making anything worse and it fixes several issues that currently exist.

Thread safety is something that is important, but it seems outside the scope of this PR.

[bmerry]

The old code used threading.Lock, so it looks like it was thread-safe, as long as you used the same Lock object across threads.

[taylor-cedar]

@bmerry Yes the original code was thread safe because it didn't really do any locking at all. It was basically a noop. I guess this could cause random issues if used in a threading environment. I added code to make it thread safe

[bmerry]

Thanks, I'll try to find some time this week to review this.

[taylor-cedar]

Thanks! I think the correct long term change here is to use redis.lock.Lock instead of a custom lock implementation. The default redis Lock does not rely on the redis server at all. I attempted to use it, but I ran into a different bug with watch. If you modify a key after a pipeline.watch from the same client it throws a redis.WatchError which isn't correct, it should ignore changes from the current client. I will create an issue for that bug.

[bmerry]

Thanks. My first thought when I looked at what redis.lock.Lock is the same as yours i.e. that once fakeredis does everything else right we can just reuse it.

[bmerry]

I've started a branch called "thread-safe" where I'm working on thread safety, and I've also replaced _Lock with a copy of redis.lock.Lock plus a few bug fixes. Give it a try and let me know if it works for your use case. I may also borrow some tests from this PR still.

[bmerry]

I'm closing this because I've put in an implementation that's a copy of redis-py's plus bug fixes, and also taken some of your unit tests (thanks). Let me know if there are still issues.