JDBC locks not unique

[rudi-matricata]

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

spring-integration-jdbc 6.5.9

Describe the bug

JdbcLocks are not unique anymore. With spring-integration-jdbc 6.5.9 DefaultLockRegistry is used in some cases. This automatically results in non-uniqueness of the locks (delegates) which in general even contradicts the documented behavior.

To Reproduce

Obtain JdbcLock and tryLock for 2 different keys (e.g. "0" and "641").

Expected behavior

As documented in the JdbcLockRegistry class:

"... An {@link ExpirableLockRegistry} using a shared database to co-ordinate the locks. Provides the same semantics as the {@link org.springframework.integration.support.locks.DefaultLockRegistry}, but the locks taken will be global, as long as the underlying database supports the "serializable" isolation level in its transactions."

This change is also backward incompatible. Prior version 6.5.9 locks were not overlapping (e.g. the code below would work).

Sample

public static void main(String[] args) throws InterruptedException, ExecutionException {
    JdbcLockRegistry registry = new JdbcLockRegistry(...);

    ExecutorService executor = Executors.newSingleThreadExecutor();

    assert registry.obtain("0").tryLock();

    executor.submit(() -> {
      assert registry.obtain("641").tryLock() : "should be able to lock";
    }).get();
  }

[mrpiggi]

probably same as #11146

[artembilan]

Well, you can just increase the cache size of this lock registry.

We did a security hardening with such a fix to avoid memory exhausting with too many distinctive locks.
The second lock in another thread might be blocked for many other reasons, too.

We might consider some other algorithm to try to distributed different keys evenly in that shared lock cache.
But that's different story.
Even if it is a breaking change for some applications, it was intended for a good reason in our modern reality.

Let us know what is your opinion or what ideas do you have how to mitigated inconvenience you got from the current solution!

[rudi-matricata]

Hi,

In my opinion a problem was fixed with 202b092 and a bigger one was introduced. Purely in terms of probability, after this change it would be close to 0% chance to reach eviction at all (in some peak periods where locks are acquired) as you would get so many collision errors for trying to (obtain) lock because of the shared in-memory locks.

I think in our modern reality, in considerable part of the cases you won't need to acquire just 2-3-5 locks. In our scenarios we sometimes need to acquire hundreds of locks in short periods. Imagine a multi-tenant application and locks acquired by tenant id (e.g. lock per tenant). Even with a huge increase of the cache capacity (2 to the power of 16 ~= 65K) there is ~25% probability of collision for just 200 different locks (keys). Even for 50 locks you still have 2% probability of collision which over time would definitely result in many collisions. While these are all just numbers, I think they show in a way what of a big impact this change introduced (for some of the cases).

The problem with the eviction of the still hold lock could be faced only if eviction occurs (to my understanding), which by default was when you reach 100_000+ different locks (keys). Also there is a method 'expireUnusedOlderThan' that can be proactively used for cleanup. Maybe another aspect that can be revised is in the direction of improved (maybe automated) cleanup of the not used locks. As hundreds of locks around the same time is a real usecase, tens of thousands (e.g. 100_000) seems really to many. Also, the lib (e.g. JdbcLockRegistry) might be extended in a way where its user can provide the cache/eviction policy etc so that the user could define what get evicted and what happens if sth. gets evicted. These are just some thoughts on the flight.

[artembilan]

Thanks, @rudi-matricata , for such a thorough analysis and kind report.

After sleeping on this I definitely agree that we made a wrong change in terms of concurrent usability.
Even if we prevent some memory exhausting that still does not justify a suffer from keys collision on the same in-memory lock.
And yes: you are totally right that with our current logic based on a pool of shared locks the cache eviction just impossible.

So, my current thinking is like this:

1. Since we have changes an internal logic, it is fine to revert it to the previous one.
2. Make a fix like is was suggested in the JdbcLockRegistry & RedisLockRegistry - Shared ReentrantLock from DefaultLockRegistry causes APP_LOCK entry not deleted on unlock when two different lock keys map to the same bucket #11146 (and it was original by @cppwfs 😄 ) - && !eldest.getValue().delegate.isLocked().
3. The point is really because we have that expireUnusedOlderThan() which can be used in the application for keeping cache clean from unused instances.
4. When we reach cache capacity, clean up it from unused in the obtain() logic.
5. If that is still not possible, throw an exception or emit an AplicationEvent, so the target consumer of our API can react respectively.
6. There is another idea like use Semaphore. But again: that would end-up with an exception over time anyway.

Any other ideas how to keep the cache clean and don't blow up the memory with an overhead?

Thank you again!