GH-3690: RedisLockRegistry improvement, using pub-sub for lock to wit… by Meteorkor · Pull Request #3691 · spring-projects/spring-integration

Meteorkor · 2021-12-13T03:24:26Z

Replace spinLock with pub-sub

use redisMessageListenerContainer

as-is
if lock accquire fail, will repeat retry to accuire lock per 100ms

to-be
if lock accquire fail, waitLock regist(unlock msg subscribe)
and retry to accuire lock, because timing issue(if other vm unlock, before waitLock regist)
and wait unlock

…for lock to without spinlock

artembilan

Pulling this locally for a closer look...

Thanks

artembilan

I didn't do a deep investigation and comparison, but it doesn't look like we get too much difference in what is there just with a plain sleep_and_repeat and this distributed subscription from all the instances. I see there is a SpinBarrier.waitFor() in the RedisMessageListenerContainer. Probably subscription by itself is not so hard, but still: we got a lot of them from every single client... Plus in the end we are blocked on the LockSupport.parkNanos() for the Future.get(). I don't mind how complex the solution is, but worry if it is optimal enough to justify the maintenance burden in the future...

Would you mind to share more details what is your expectation from this fix and how it supposed to work? (See my comments on lines anyway)

I respect your work and appreciate the effort though.

Thank you!

artembilan · 2021-12-13T20:20:59Z

+				if (!RedisLockRegistry.this.redisMessageListenerContainer.isRunning()) {
+					RedisLockRegistry.this.redisMessageListenerContainer.start();
+				}
+				final Future future = RedisLockRegistry.this.unlockNotifyMessageListener.unlockWait(this.lockKey);


I wonder what is the difference between that Thread.sleep(); until expire and this Future.get().
Feels like not too much difference in the final logic.
Only the problem that Thread.sleep(); is a local function while that listener requires an additional subscription into Redis. Therefore it is more resource consuming.

What do I miss?

And what happened with UNLINK: https://redis.io/commands/UNLINK ?

I understand that we may move to it altogether since Redis 4.0.0 is old enough to worry about more older versions in support, but still that would be slightly breaking change in the current 5.5.x.
Although we may just fully treat your change as a new feature and move it to the next major version.
Just need to understand what a benefit we would gain from your solution?

I think the difference between Thread.sleep() and future.get() is
I think there is a difference between purpose and frequency.

If the purpose of Thread.sleep() was to simply repeat

future.get() doesn't need to check repeatedly until unlock() is done.
However, since ttl exists, I set the time.
it must be able to wake up without unlocking

Usually we expect to use the default value (1min), so we don't need to retry in the meantime.
And if you perform unLock() faster than 100ms in another vm, you can acquire the lock faster than 100ms.

second,
I think the use for unlink should be considered.
That part is not reflected yet.
(If we use this solution, I will fix it.)

you can acquire the lock faster than 100ms

That’s really convincing 😅. Only need to figure out what to do with dangling , not in demand unlock events you store in the cache…

probably time to look closer into that Redisson solution with pub-sub.

I’m not convinced yet that it is worth to manage one more optional dependency just for this lock solution… although they may have other useful features to consider in the future 😉

artembilan · 2021-12-13T20:47:21Z

+		}
+	}
+
+	private final class UnLockNotifyFuture extends FutureTask<Boolean> {


See org.springframework.util.concurrent.SettableListenableFuture - we don't need a dedicated custom type since one already exists.

artembilan · 2021-12-13T20:47:25Z

+		}
+
+		public Future unlockWait(String lockKey) {
+			return this.notifyMap.computeIfAbsent(lockKey, key -> new UnLockNotifyFuture());


I believe there is a flaw in this solution: we may never be interested in many, many locks in the Redis, but only one.
So, just after requesting our unavailable lock we start listener container and subscribe to the global topic.
When ever other clients release their locks, they are going to appear in this local store and will not be removed.

Remember you have fixed recently a memory leak in this registry: #3655 ?

Another concern that all the subscribers are going to get the same just released lock notification....

artembilan · 2021-12-13T21:53:40Z

Looks like you took some inspiration from the Redisson: https://github.com/redisson/redisson/blob/master/redisson/src/main/java/org/redisson/RedissonLock.java.
Docs are here: https://github.com/redisson/redisson/wiki/8.-distributed-locks-and-synchronizers

I wonder if we still need to keep our own solution when we just can use their solution and avoid copy/pasting and time consuming for the task which is out of scope of this Messaging framework...

Meteorkor · 2021-12-14T02:04:40Z

I looked into redisson while looking for how to improve spinlock, but I applied it briefly because I think adding a dependency to the project has a big impact.

If you don't apply a PR solution, it's ok to apply their solution.

And if you think the maintenance cost is going to be high, it's okay to keep the existing code.

It's just a suggestion to make a little bit more improvement.

* change remove timing for 'notifyMap' * use unlink * remove DUMMY_CALLABLE

artembilan

I think now it looks good.
Pulling locally for final review and code clean up...

Thank you!

artembilan · 2021-12-14T17:07:14Z

Merged as 3c32daa.

@Meteorkor ,

thank you again for a great contribution; looking forward for more!

Meteorkor · 2021-12-14T23:07:57Z

@artembilan
Thank you for your kind review as always.

Thank you for code clean up

However, there seems to be one change in the code.
3c32daa

It seems that the deleteUnlock code should be outside the catch.
3c32daa#diff-225f2af5365e6371c7b7d751c08bf2d69fd3e9b4ddcd12b697ddf175952adaecR458
RedisLockRegistry:458

If unlink fails the first time, there may be no problem, but if the second removeLockKey() is called, there seems to be a problem that it is not called.

It seems there are various ways.

Move out of catch and return when unlink is normally executed
Keep execution in catch, add code to delete when RedisLockRegistry.this.unlinkAvailable is false

See review: #3691 (comment)

artembilan · 2021-12-15T00:51:35Z

You are right!

Have just pushed the fix: 2d75289

Thank you again! 😄

Meteorkor added 2 commits December 13, 2021 12:06

spring-projectsGH-3690: RedisLockRegistry improvement, using pub-sub …

889391d

…for lock to without spinlock

rename and add comment

de2fba5

artembilan reviewed Dec 13, 2021

View reviewed changes

artembilan requested changes Dec 13, 2021

View reviewed changes

change remove timing for 'notifyMap'

697725c

* change remove timing for 'notifyMap' * use unlink * remove DUMMY_CALLABLE

Meteorkor requested a review from artembilan December 14, 2021 14:54

artembilan approved these changes Dec 14, 2021

View reviewed changes

artembilan closed this Dec 14, 2021

artembilan added a commit that referenced this pull request Dec 15, 2021

Fix UNLINK/DEL logic in the RedisLockRegistry

2d75289

See review: #3691 (comment)

Uh oh!

Conversation

Meteorkor commented Dec 13, 2021

Uh oh!

artembilan left a comment

Choose a reason for hiding this comment

Uh oh!

artembilan left a comment

Choose a reason for hiding this comment

Uh oh!

artembilan Dec 13, 2021

Choose a reason for hiding this comment

Uh oh!

Meteorkor Dec 14, 2021

Choose a reason for hiding this comment

Uh oh!

artembilan Dec 14, 2021

Choose a reason for hiding this comment

Uh oh!

artembilan Dec 13, 2021

Choose a reason for hiding this comment

Uh oh!

artembilan Dec 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artembilan commented Dec 13, 2021

Uh oh!

Meteorkor commented Dec 14, 2021

Uh oh!

artembilan left a comment

Choose a reason for hiding this comment

Uh oh!

artembilan commented Dec 14, 2021

Uh oh!

Meteorkor commented Dec 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

artembilan commented Dec 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

artembilan Dec 13, 2021 •

edited

Loading

Meteorkor commented Dec 14, 2021 •

edited

Loading