Skip to content
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

RedisLockRegistry improvement, using pub-sub for lock to without spinlock #3690

Closed
Meteorkor opened this issue Dec 12, 2021 · 3 comments
Closed

Comments

@Meteorkor
Copy link
Contributor

Expected Behavior

Remove spinlock for used by RedisLockRegistry
So, I thought about how to improve this part through redis pub-sub.

Current Behavior

Currently, spinlock is used to acquire redis-lock.

https://github.com/spring-projects/spring-integration/blob/main/spring-integration-redis/src/main/java/org/springframework/integration/redis/util/RedisLockRegistry.java#L231-L233

while (!obtainLock()) {
    Thread.sleep(100); //NOSONAR
}

https://github.com/spring-projects/spring-integration/blob/main/spring-integration-redis/src/main/java/org/springframework/integration/redis/util/RedisLockRegistry.java#L291-L298

long expire = now + TimeUnit.MILLISECONDS.convert(time, unit);
boolean acquired;
while (!(acquired = obtainLock()) && System.currentTimeMillis() < expire) { //NOSONAR
    Thread.sleep(100); //NOSONAR
}


If a vm instance acquires a lock and persists for a long time, or if multiple vm instances try to acquire a lock, I think the part where obtainLock() is called continuously is wasteful.

Context

I would like to contribute to the project.
If it's okay with you, I'd like to write a PR and get a review.

@Meteorkor Meteorkor added status: waiting-for-triage The issue need to be evaluated and its future decided type: enhancement labels Dec 12, 2021
@artembilan
Copy link
Member

I’m not fully sure what solution you suggest, but opening PR and continue discussion over there won’t harm. If you have time to code your idea, of course…

thank you!

Meteorkor added a commit to Meteorkor/spring-integration that referenced this issue Dec 13, 2021
@artembilan artembilan added this to the 5.5.7 milestone Dec 14, 2021
@artembilan artembilan added in: redis and removed status: waiting-for-triage The issue need to be evaluated and its future decided labels Dec 14, 2021
@vipul-mykaarma
Copy link

ever since we have upgraded srping-integration-redis to 5.5.8 we are encountering the following error
Connection failure occurred. Restarting subscription task after 5000 ms
Although lock functionality is working fine . The error is coming from RedisMessageListenerContainer present within redis lock registry

@artembilan
Copy link
Member

@vipul-mykaarma ,

that means you have some connectivity issue, although it is self-recovering.
The logic is like this:

if (ex instanceof RedisConnectionFailureException) {
			if (isRunning()) {
				logger.error("Connection failure occurred. Restarting subscription task after " + recoveryInterval + " ms");
				sleepBeforeRecoveryAttempt();
				lazyListen();
			}
		} else {

So, all should be OK with a couple retries.

There is nothing RedisLockRegistry-related though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants