Uncertain if contrib/lock works as intended

[tiptop96]

I am trying to learn more about how to do distributed locking and found the example in contrib/lock but it doesn't work as I would expect.

The readme in contrib/lock refers to this article by Martin Kleppmann about distributed locking which says that a monotonically increasing value issued by a locking service can be used by a storage engine to check if a lease is old and no longer valid by checking wether a requests version number is less than the the previous one.

The example however works by checking if the version number of the second request is equal to the version number of the first request. I could be misunderstanding but it seems incorrect to me.

Would a change like this make it more correct?

-			if req.Version != val.version {
+			if req.Version < val.version {

A thought that pops up if a check for "less than" was implemented is that something like the following could happen.

• Client A calls NewSession and gets lease id 1
• Client B calls NewSession and gets lease id 2
• Client B runs locker.Lock and gets the lock
• Client A runs locker.Lock and is blocked
• Client B calls the storage server sending it lease id 2 as the version
• Client B finishes and calls locker.Unlock
• Client A is unblocked and gets the lock
• Client A calls the storage server sending it lease id 1 as the version but is rejected due to it being a lower lease id than the previous request

I am not sure if this is a huge problem, but once client A has received the lock there really is no point in calling the storage service since it will have a old lease id. It seems more intuitive to me to first take the lock and then get a lease id, this would prevent unnecessary failed requests and ensure that whoever has the lock also has the most recent (also greatest) lease id?

[ahrtr]

Please refer to #18779

[tiptop96]

Awesome, thanks!