Skip to content

RedisLockRegistry expireAfter doesn't work #3401

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
hero-zhanghao opened this issue Oct 10, 2020 · 5 comments
Closed

RedisLockRegistry expireAfter doesn't work #3401

hero-zhanghao opened this issue Oct 10, 2020 · 5 comments
Labels
status: waiting-for-reporter Needs a feedback from the reporter type: bug

Comments

@hero-zhanghao
Copy link

hero-zhanghao commented Oct 10, 2020

In what version(s) of Spring Integration are you seeing this issue?

For example:

5.3.1.RELEASE

Describe the bug

I create a RedisLockRegistry and set expireAfter to 1000L .

After obtain the Lock object, invoke tryLock (3000, TimeUnit.MILLISECONDS) method.

Print 'lock-test' to sleep 2000L milliseconds, then call unlock.

When two threads come in at the same time, the second thread should get the lock one second after the first thread,

However, what observe now is that the two threads are always two seconds apart, seemingly unaffected by the expireAfter field.

thanks

To Reproduce

2020-10-10 15:40:46,063 [boundedElastic-2] INFO c.e.s.raffle.utils.RedisLockUtils.lambda$null$1(RedisLockUtils.java:65) - try obtain lock

2020-10-10 15:40:46,066 [boundedElastic-2] INFO c.e.s.raffle.utils.RedisLockUtils.lambda$null$1(RedisLockUtils.java:68) - Got the distributed lock and executing , key = test , time = 2020-10-10T15:40:46.066

lock-test

2020-10-10 15:40:47,006 [boundedElastic-1] INFO c.e.s.raffle.utils.RedisLockUtils.lambda$null$1(RedisLockUtils.java:65) - try obtain lock

2020-10-10 15:40:48,069 [boundedElastic-1] INFO c.e.s.raffle.utils.RedisLockUtils.lambda$null$1(RedisLockUtils.java:68) - Got the distributed lock and executing , key = test , time = 2020-10-10T15:40:48.069

lock-test

@hero-zhanghao hero-zhanghao added status: waiting-for-triage The issue need to be evaluated and its future decided type: bug labels Oct 10, 2020
@hero-zhanghao
Copy link
Author

Also, I find that if the unlock method is not called, the lock cannot be obtained again.

This seems to have been affected by ReentrantLock

@artembilan
Copy link
Member

Would be great to have some code to play with.

There is no expire option for the ReentrantLock and we definitely can't unlock it from non-owner. So, your concern is not clear: as long as you hold a lock in one thread, it cannot be obtained from another one even if its distributed instance has been expired in Redis.

Thanks for understanding.

@artembilan artembilan added status: waiting-for-reporter Needs a feedback from the reporter and removed status: waiting-for-triage The issue need to be evaluated and its future decided labels Oct 12, 2020
@hero-zhanghao
Copy link
Author

But most of the time, we need to be able to release locks automatically after the lock holder has held them for a certain amount of time, which is one of the reasons for using Redis.

Maybe we can choose whether or not we need ReentrantLock

@artembilan
Copy link
Member

Well, that's not how locks are designed and you definitely can't release the lock if you are not a holder of it.
It even doesn't matter if we don't use ReentrantLock just because the holder may not know (or just doesn't care) if lock has been expired already or not. You even may release the lock somehow, but your main flow may not know about that at all and would continue to perform its logic. That's why it is recommended to design locks logic carefully, so you have some way to release lock safely even for failure reason. Since ReentrantLock is like that we can't release it from outside, so we have designed all the distributed locks the same way. The point of the expireAfter is more about a shared persistent store to not hold a record when the owner of the lock dies accidentally.

We have a feature request like this: https://jira.spring.io/browse/INT-4286, so probably we can have some compromise eventually. But still: I'm against unlocking outside from the lock owner...

See also this discussion: #2894

@hero-zhanghao
Copy link
Author

Ok, thank you for your answer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting-for-reporter Needs a feedback from the reporter type: bug
Projects
None yet
Development

No branches or pull requests

2 participants