Liquibase tries to unlock unacquired lock

[sta-szek]

Search first

• [X] I searched and no similar issues were found

Description

Hi. I think i found issue in latest liquibase versions. What might be important for you is that we are using custom PostgresSessionLockService that depends on pg_try_advisory_lock in order to implement multithreaded, (schema based) multitenancy support.

Problem is that liquibase tries to release lock in finally block, and the lock was not acquired before. I think the problem might be in method liquibase.command.core.AbstractUpdateCommandStep#run

try {
            DatabaseChangeLog databaseChangeLog = (DatabaseChangeLog) commandScope.getDependency(DatabaseChangeLog.class);

// here in the next line, it checks for two things:
* isFastCheckEnabled - true
* isUpToDate - true
and then it jumps to the finally block, where lockService.relaseLock() is called, but
the getLockService(database).waitForLock(); was not called at all.

            if (isFastCheckEnabled && isUpToDate(commandScope, database, databaseChangeLog, contexts, labelExpression, resultsBuilder.getOutputStream())) {
                return; //// <---- this return does do return the method. but also, it will go through finally block 
            }
            if(!isDBLocked) {
                LockServiceFactory.getInstance().getLockService(database).waitForLock();
                // waitForLock resets the changelog history service, so we need to rebuild that and generate a final deploymentId.
                ChangeLogHistoryService changeLogHistoryService = Scope.getCurrentScope().getSingleton(ChangeLogHistoryServiceFactory.class).getChangeLogService(database);
                changeLogHistoryService.init();
                changeLogHistoryService.generateDeploymentId();
            }

            // rest of code does not matter...
        } catch (Exception e) {
            // rest of code does not matter...
        } finally {
            //TODO: We should be able to remove this once we get the rest of the update family
            // set up with the CommandFramework
            try {
                LockServiceFactory.getInstance().getLockService(database).releaseLock();
            } catch (LockException e) {
                Scope.getCurrentScope().getLog(getClass()).severe(MSG_COULD_NOT_RELEASE_LOCK, e);
            }
        }

This is problematic because after upgrade it starts to fail.

I tried to check when the isFastCheckEnabled flag was introduced, but unfortunately there is noting in docs: https://docs.liquibase.com/search.html?q=isFastCheckEnabled#gsc.tab=0&gsc.q=isFastCheckEnabled&gsc.page=1

Also, i have suspicious that the isFastCheckEnabled logic will break our multithreaded multitenancy: We run liquibase in parallel (create many SpringLiquibase objects and execute afterPropertiesSet) in the same JVM instance. Each liquibase will run migrations for single tenant. In order to implement that, we had to provide many custom beans (ScopeManager, LockService, etc...) because by default they assume liquibase can run for single database single tenant only. I am afraid that this isFastCheckEnabled toghether with things like upToDateFastCheck.containsKey(cacheKey) will break, because the cache checks dbname + schema name (in my case it is computed as /()/public/starter-db/jdbc:postgresql://localhost:5432/starter-db and if we want to run multiple migrations (from starters, from applications, etc..) then it might play a role and prevent migrations from being executed.

Is it possible to configure isFastCheckEnabled to false always somehow?

Steps To Reproduce

In my case, just upgrade version (with spring-boot 3.1.6 -> 3.2.1) from 4.20.0 to 4.24.0

Expected/Desired Behavior

Expected behaviour is that the lock will be aqcuired before unlocking, or the logic will be extended to check if any locks are acquired before releasing.

Liquibase Version

4.24.0

Database Vendor & Version

11

Liquibase Integration

spring boot

Liquibase Extensions

No response

OS and/or Infrastructure Type/Provider

No response

Additional Context

can be related:

• https://github.com/liquibase/liquibase/pull/4427
• https://github.com/liquibase/liquibase/pull/4016
• https://github.com/liquibase/liquibase/commit/831135b58ef12a99cbb7b5a7a66539d680193b47#diff-05554aee9a51989aac56e61327cd424c849ef84b68f5da6cc6e3b58187d355edR35 -> why true by default? how to change it?

Possible solutions:

Set isFastCheckEnabled to false

Currently not possible

Set liquibase contexts to smth like this: liquibase.setContexts(properties.getContexts() + System.nanoTime());

This is to enforce cache-miss all the time. This worked for me, however... i expect some OOM in liquibase after application runs many migrations (e.g. many tenants created / deleted + execute many migrations for each of them), because the cache is not cache actually, but unlimited map: private final Map<String, Boolean> upToDateFastCheck = new ConcurrentHashMap<>();

Additionally, to get rid of spammy errors (actaully error messages without error in business logic) i have to update my PostgresSessionLockService with check if lock was acquired before releasing:

    @Override
    public void releaseLock() throws LockException {
        try {
            if (super.hasChangeLogLock) {
                releaseLock(getConnection());
                logger.debug("Successfully released change log lock {}", database.getDefaultSchema());
            }

However, i find this solution as not clean code - i hide real problem in business logic (release before acquire) instead of solving the problem. It will definiately come back one day...

Are you willing to submit a PR?

• [ ] I'm willing to submit a PR (Thank you!)

[MalloD12]

Hi @sta-szek,

I was thinking we could update the finally block from liquibase.command.core.AbstractUpdateCommandStep#run to something like this:

 try {
                LockService lockService = LockServiceFactory.getInstance().getLockService(database);
                if(isDBLocked && lockService.hasChangeLogLock()) {
                    lockService.releaseLock();
                }
            } catch (LockException e) {
                Scope.getCurrentScope().getLog(getClass()).severe(MSG_COULD_NOT_RELEASE_LOCK, e);
            }

This code was originally thought to work on a single thread app, but I think something like this might help us to prevent the concurrency issue of releasing the lock when there is nothing to release. If it doesn't help maybe we need to think about the possibility of having a specific LockService implementation that handles better a multi-thread use case.

What do you think about it?

cc: @filipelautert

[sta-szek]

Thank you for checking this @MalloD12 !

We are using our custom PG lock service, heavily based on https://github.com/stanio/liquibase-sessionlock

[MalloD12]

Hi @sta-szek,

Oh, I see. Would you like me to push the change I suggested in my previous comment so that way we can see if that helps?

[Praveen-7163]

Hi,

Even we are also facing same issue post liquibase upgrade from 4.20.0 to 4.24.0.