lukas-krecan / ShedLock

Distributed lock for your scheduled tasks
Apache License 2.0
3.65k stars 518 forks source link

LockProvider always returns and updates existing Lock instead of empty #2148

Closed Pawe98 closed 1 month ago

Pawe98 commented 1 month ago

Describe the bug

  1. 5.1.0
  2. JdbcTemplateLockProvider
  3. 
    @Configuration
    public class ShedLockConfiguration {
    
    @Bean
    public LockProvider lockProvider(DataSource dataSource) {
        return new JdbcTemplateLockProvider(
                JdbcTemplateLockProvider.Configuration.builder()
                        .withJdbcTemplate(new JdbcTemplate(dataSource))
                        .usingDbTime()
                        .build()
        );
    }
    }

**Expected behavior**
If a lock with the specified name already exists, lockProvider.lock should either return empty or indicate that the lock acquisition failed, preventing the operation from proceeding.

**Actual behavior**
The lockProvider.lock method always returns an existing lock and updates it, even if a lock with the same name already exists. This behavior leads to multiple concurrent executions instead of preventing them.

**_SAMPLE CODE_**
Using a long running method and locking it:

String lockName = "recreateIndex"; LockConfiguration lockConfiguration = new LockConfiguration( Instant.now(), lockName, Duration.ofSeconds(180), Duration.ZERO );

Optional<SimpleLock> lockOptional = lockProvider.lock(lockConfiguration);
if (lockOptional.isEmpty()) {
    log.warn("Index recreation process is already running");
    throw new RuntimeException("Index recreation process is already running");
}
//long running code
lock.unlock()

The problem is that lockProvider.lock(lockConfiguration); does not return empty on the second, parallel execution. When debugging, I can see that the lock exists (in DB and registry) and gets updated from here:

protected boolean doLock(LockConfiguration lockConfiguration) { String name = lockConfiguration.getName();

    boolean tryToCreateLockRecord = !lockRecordRegistry.lockRecordRecentlyCreated(name);
    if (tryToCreateLockRecord) {
        // create record in case it does not exist yet
        if (storageAccessor.insertRecord(lockConfiguration)) {
            lockRecordRegistry.addLockRecord(name);
            // we were able to create the record, we have the lock
            return true;
        }
        // we were not able to create the record, it already exists, let's put it to the cache so we do not try again
        lockRecordRegistry.addLockRecord(name);
    }

    // let's try to update the record, if successful, we have the lock
    try {
        return storageAccessor.updateRecord(lockConfiguration); //  THIS IS CALLED AND RETURNS EXISTING LOCK
    } catch (Exception e) {
        // There are some users that start the app before they have the DB ready.
        // If they use JDBC, insertRecord returns false, the record is stored in the recordRegistry
        // and the insert is not attempted again. We are assuming that the DB still does not exist
        // when update is attempted. Unlike insert, update throws the exception, and we clear the cache here.
        if (tryToCreateLockRecord) {
            lockRecordRegistry.removeLockRecord(name);
        }
        throw e;
    }
}

So it updates the lock in "
`return storageAccessor.updateRecord(lockConfiguration); //  THIS IS CALLED AND RETURNS EXISTING LOCK`
" and then just returns it.
Pawe98 commented 1 month ago

nvm it works as intended, I've called unlock() unexpectedly while the task was still running...