Use DefaultLockRegistry to support lock pooling#11130
Conversation
- verify that we don't aquire InterProcessMutex more than once per lock - adjust lock capacity to 256 Fixes: spring-projects#11113 **Auto-cherry-pick to 7.0.x & 6.5.x**
artembilan
left a comment
artembilan
left a comment
There was a problem hiding this comment.
I see now the problem.
Let's take it offline tomorrow!
Or whenever we can!
| * @param cacheCapacity The capacity of cached lock, (default 30_000). | ||
| * @param cacheCapacity The capacity of cached lock, (default 256 locks). | ||
| * @since 5.5.6 | ||
| * |
There was a problem hiding this comment.
No blank lines in method Javadocs
| } | ||
| catch (InterruptedException ie) { | ||
| this.delegate.unlock(); | ||
| throw ie; |
There was a problem hiding this comment.
What happened with Thread.currentThread().interrupt();?
Remember, that interrupted status is reset we call Thread.interrupted()?
See InterruptedException Javadocs.
See the same lockInterruptibly() implementation in the JdbcLock.
| ZkLock(CuratorFramework client, AsyncTaskExecutor mutexTaskExecutor, String path) { | ||
| private final ReentrantLock delegate; | ||
|
|
||
| ZkLock(CuratorFramework client, AsyncTaskExecutor mutexTaskExecutor, String path, |
There was a problem hiding this comment.
I wonder if it still justifies to keep this class as static.
Looks like dropping that would give us a good interaction with outer class.
Including new defaultLockRegistry property.
|
|
||
| ZkLock(CuratorFramework client, AsyncTaskExecutor mutexTaskExecutor, String path, | ||
| ReentrantLock registryDelegate) { | ||
| this.client = client; |
There was a problem hiding this comment.
Blank line after multi-line block header where method definition is also a block.
That makes code much easier to read.
Although, after removing static from this inner class, the ZkLock ctor would be much simpler.
|
|
||
| private int cacheCapacity = DEFAULT_CAPACITY; | ||
|
|
||
| private volatile DefaultLockRegistry defaultLockRegistry = new DefaultLockRegistry(); |
There was a problem hiding this comment.
I don't believe in volatile on setters.
That is really supposed to happen on the application context initialization, so no concurrent concern.
Therefore no need in hard thread state reset when we use this property.
Interesting though, that there is no volatile on same property in the JdbcLockRegistry.
| if (e instanceof RuntimeException runtimeException) { | ||
| throw runtimeException; | ||
| } | ||
| throw new RuntimeException(e); |
There was a problem hiding this comment.
Why JdbcLockRegistry.JdbcLock.rethrowAsLockException() doesn't fit into logic here?
| Future<Boolean> future = null; | ||
| try { | ||
| long startTime = System.currentTimeMillis(); | ||
| long startTime = System.nanoTime(); |
There was a problem hiding this comment.
Why have we started to use nanos?
| } | ||
| catch (Exception e) { | ||
| throw new MessagingException("Failed to acquire mutex at " + this.path, e); | ||
| throw new MessagingException("Failed to acquire lock at " + this.path, e); |
There was a problem hiding this comment.
The MessagingException is wrong in this contract.
OK. Let's leave for backward compatibility alongside with your new RuntimeException() in other places!
And let's raise an issue for new version to bring similar to others over here, too:
private void rethrowAsLockException(Exception e) {
throw new CannotAcquireLockException("Failed to lock mutex at " + this.lockKey, e);
}
| this.delegate.unlock(); | ||
| throw ie; | ||
| } | ||
| catch (Exception e) { |
There was a problem hiding this comment.
Just nit-pick: it appears more clear when the variable name is not one symbol.
I also know that Checkstyle from Spring Java Format rejects this kind of definitions.
I know we have already enough in this class and in other places, but still would be great to not introduce new.
2 participants
Fixes: #11113
Auto-cherry-pick to 7.0.x & 6.5.x