libre.sh/keycloak-scim
3
0
Fork 0
Code Issues 15 Pull requests Releases Packages Activity Actions

Make lockTimeout more friendly for JPA map storage

Browse source 
Closes #16616
This commit is contained in:
Michal Hajas 2023-02-10 11:09:05 +01:00
parent bb0eb899a7
commit 1f929c78af
5 changed files with 73 additions and 29 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

34
model/map-jpa/src/main/java/org/keycloak/models/map/storage/jpa/JpaMapStorageProviderFactory.java
View file

@ -26,6 +26,7 @@ import java.sql.ResultSet;
import java.sql.SQLException;
import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
@ -146,6 +147,8 @@ import org.keycloak.models.map.storage.jpa.user.entity.JpaUserFederatedIdentityE
import org.keycloak.models.map.user.MapUserCredentialEntity;
import org.keycloak.models.map.user.MapUserCredentialEntityImpl;
import org.keycloak.provider.EnvironmentDependentProviderFactory;
import org.keycloak.provider.ProviderConfigProperty;
import org.keycloak.provider.ProviderConfigurationBuilder;
import org.keycloak.sessions.RootAuthenticationSessionModel;
import org.keycloak.transaction.JtaTransactionManagerLookup;
@ -161,6 +164,8 @@ public class JpaMapStorageProviderFactory implements
public static final String HIBERNATE_DEFAULT_SCHEMA = "hibernate.default_schema";
private static final long DEFAULT_LOCK_TIMEOUT = 10000;
private volatile EntityManagerFactory emf;
private final Set<Class<?>> validatedModels = ConcurrentHashMap.newKeySet();
private Config.Scope config;
@ -290,10 +295,10 @@ public class JpaMapStorageProviderFactory implements
EntityManager em = emf.createEntityManager();
// This is a workaround for Hibernate not supporting javax.persistence.lock.timeout
// config option for Postgresql/CockroachDB - https://hibernate.atlassian.net/browse/HHH-16071
// config option for Postgresql/CockroachDB - https://hibernate.atlassian.net/browse/HHH-16181
if ("postgresql".equals(databaseShortName) || "cockroachdb".equals(databaseShortName)) {
Long lockTimeout = config.getLong("lockTimeout");
if (lockTimeout != null) {
Long lockTimeout = config.getLong("lockTimeout", DEFAULT_LOCK_TIMEOUT);
if (lockTimeout >= 0) {
em.unwrap(SessionImpl.class)
.doWork(connection -> {
// 'SET LOCAL lock_timeout = ...' can't be used with parameters in a prepared statement, leads to an
@ -307,8 +312,6 @@ public class JpaMapStorageProviderFactory implements
resultSet.close();
}
});
} else {
logger.warnf("Database %s used without lockTimeout option configured. This can result in deadlock where one connection waits for a pessimistic write lock forever.", databaseShortName);
}
}
return em;
@ -403,10 +406,12 @@ public class JpaMapStorageProviderFactory implements
properties.put("hibernate.show_sql", config.getBoolean("showSql", false));
properties.put("hibernate.format_sql", config.getBoolean("formatSql", true));
properties.put("hibernate.dialect", config.get("driverDialect"));
Integer lockTimeout = config.getInt("lockTimeout");
if (lockTimeout != null) {
// This property does not work for PostgreSQL/CockroachDB - https://hibernate.atlassian.net/browse/HHH-16071
properties.put("javax.persistence.lock.timeout", lockTimeout);
Long lockTimeout = config.getLong("lockTimeout", DEFAULT_LOCK_TIMEOUT);
if (lockTimeout >= 0) {
// This property does not work for PostgreSQL/CockroachDB - https://hibernate.atlassian.net/browse/HHH-16181
properties.put(AvailableSettings.JPA_LOCK_TIMEOUT, String.valueOf(lockTimeout));
} else {
logger.warnf("Database %s used without lockTimeout option configured. This can result in deadlock where one connection waits for a pessimistic write lock forever.", databaseShortName);
}
logger.trace("Creating EntityManagerFactory");
@ -536,4 +541,15 @@ public class JpaMapStorageProviderFactory implements
return null;
});
}
@Override
public List<ProviderConfigProperty> getConfigMetadata() {
return ProviderConfigurationBuilder.create()
.property()
.name("lockTimeout")
.type("long")
.defaultValue(10000L)
.helpText("The maximum time to wait in milliseconds when waiting for acquiring a pessimistic read lock. If set to negative there is no timeout configured.")
.add().build();
}
}

5
quarkus/deployment/src/main/java/org/keycloak/quarkus/deployment/KeycloakProcessor.java
View file

@ -322,6 +322,11 @@ class KeycloakProcessor {
unitProperties.setProperty(AvailableSettings.JPA_TRANSACTION_TYPE, PersistenceUnitTransactionType.JTA.name());
}
ConfigValue lockTimeoutConfigValue = getConfig().getConfigValue("kc.spi-map-storage-jpa-lock-timeout");
if (lockTimeoutConfigValue != null && lockTimeoutConfigValue.getValue() != null) {
unitProperties.setProperty(AvailableSettings.JPA_LOCK_TIMEOUT, lockTimeoutConfigValue.getValue());
}
unitProperties.setProperty(AvailableSettings.QUERY_STARTUP_CHECKING, Boolean.FALSE.toString());
String dbKind = defaultDataSource.getDbKind();

4
testsuite/model/src/test/java/org/keycloak/testsuite/model/parameters/JpaMapStorage.java
View file

@ -54,6 +54,8 @@ import org.keycloak.testsuite.model.KeycloakModelParameters;
import org.testcontainers.containers.PostgreSQLContainer;
import org.testcontainers.utility.DockerImageName;
import static org.keycloak.testsuite.model.transaction.StorageTransactionTest.LOCK_TIMEOUT_SYSTEM_PROPERTY;
public class JpaMapStorage extends KeycloakModelParameters {
private static final Logger LOG = Logger.getLogger(JpaMapStorage.class.getName());
@ -96,7 +98,7 @@ public class JpaMapStorage extends KeycloakModelParameters {
.config("password", POSTGRES_DB_PASSWORD)
.config("driver", "org.postgresql.Driver")
.config("driverDialect", "org.keycloak.models.map.storage.jpa.hibernate.dialect.JsonbPostgreSQL95Dialect")
.config("lockTimeout", "1000");
.config("lockTimeout", "${" + LOCK_TIMEOUT_SYSTEM_PROPERTY + ":}");
cf.spi(AuthenticationSessionSpi.PROVIDER_ID).provider(MapRootAuthenticationSessionProviderFactory.PROVIDER_ID) .config(STORAGE_CONFIG, JpaMapStorageProviderFactory.PROVIDER_ID)
.spi("client").provider(MapClientProviderFactory.PROVIDER_ID) .config(STORAGE_CONFIG, JpaMapStorageProviderFactory.PROVIDER_ID)

4
testsuite/model/src/test/java/org/keycloak/testsuite/model/parameters/JpaMapStorageCockroachdb.java
View file

@ -56,6 +56,8 @@ import org.testcontainers.utility.DockerImageName;
import java.util.Set;
import static org.keycloak.testsuite.model.transaction.StorageTransactionTest.LOCK_TIMEOUT_SYSTEM_PROPERTY;
public class JpaMapStorageCockroachdb extends KeycloakModelParameters {
private static final Logger LOG = Logger.getLogger(JpaMapStorageCockroachdb.class.getName());
@ -97,7 +99,7 @@ public class JpaMapStorageCockroachdb extends KeycloakModelParameters {
.config("password", COCKROACHDB_DB_PASSWORD)
.config("driver", "org.postgresql.Driver")
.config("driverDialect", "org.keycloak.models.map.storage.jpa.hibernate.dialect.JsonbPostgreSQL95Dialect")
.config("lockTimeout", "1000");
.config("lockTimeout", "${" + LOCK_TIMEOUT_SYSTEM_PROPERTY + ":}");
cf.spi(AuthenticationSessionSpi.PROVIDER_ID).provider(MapRootAuthenticationSessionProviderFactory.PROVIDER_ID) .config(STORAGE_CONFIG, JpaMapStorageProviderFactory.PROVIDER_ID)
.spi("client").provider(MapClientProviderFactory.PROVIDER_ID) .config(STORAGE_CONFIG, JpaMapStorageProviderFactory.PROVIDER_ID)

55
testsuite/model/src/test/java/org/keycloak/testsuite/model/transaction/StorageTransactionTest.java
View file

@ -24,6 +24,7 @@ import org.keycloak.models.ModelException;
import org.keycloak.models.RealmModel;
import org.keycloak.models.RealmProvider;
import org.keycloak.models.map.storage.MapStorageProvider;
import org.keycloak.models.map.storage.MapStorageSpi;
import org.keycloak.models.map.storage.jpa.JpaMapStorageProviderFactory;
import org.keycloak.testsuite.model.KeycloakModelTest;
import org.keycloak.testsuite.model.RequireProvider;
@ -43,6 +44,11 @@ import static org.keycloak.testsuite.model.util.KeycloakAssertions.assertExcepti
@RequireProvider(RealmProvider.class)
public class StorageTransactionTest extends KeycloakModelTest {
// System variable is used to simplify configuration for more storages that support pessimistic locking.
// Instead of searching which storage is used and then configure its factory, we can just configure
// lockTimeout like this: .config("lockTimeout", "${keycloak.model.tests.lockTimeout:}") and
// system property will be picked when factory is reinitialized.
public static final String LOCK_TIMEOUT_SYSTEM_PROPERTY = "keycloak.model.tests.lockTimeout";
private String realmId;
@Override
@ -123,29 +129,42 @@ public class StorageTransactionTest extends KeycloakModelTest {
// LockObjectForModification is currently used only in map-jpa
@RequireProvider(value = MapStorageProvider.class, only = JpaMapStorageProviderFactory.PROVIDER_ID)
public void testLockObjectForModification() {
TransactionController tx1 = new TransactionController(getFactory());
TransactionController tx2 = new TransactionController(getFactory());
TransactionController tx3 = new TransactionController(getFactory());
String originalTimeoutValue = System.getProperty(LOCK_TIMEOUT_SYSTEM_PROPERTY);
try {
System.setProperty(LOCK_TIMEOUT_SYSTEM_PROPERTY, "300");
reinitializeKeycloakSessionFactory();
tx1.begin();
tx2.begin();
TransactionController tx1 = new TransactionController(getFactory());
TransactionController tx2 = new TransactionController(getFactory());
TransactionController tx3 = new TransactionController(getFactory());
// tx1 acquires lock
tx1.runStep(session -> LockObjectsForModification.lockRealmsForModification(session, () -> session.realms().getRealm(realmId)));
tx1.begin();
tx2.begin();
// tx2 should fail as tx1 locked the realm
assertException(() -> tx2.runStep(session -> LockObjectsForModification.lockRealmsForModification(session, () -> session.realms().getRealm(realmId))),
allOf(instanceOf(ModelException.class),
hasCause(instanceOf(PessimisticLockException.class))));
// tx1 acquires lock
tx1.runStep(session -> LockObjectsForModification.lockRealmsForModification(session, () -> session.realms().getRealm(realmId)));
// end both transactions
tx2.rollback();
tx1.commit();
// tx2 should fail as tx1 locked the realm
assertException(() -> tx2.runStep(session -> LockObjectsForModification.lockRealmsForModification(session, () -> session.realms().getRealm(realmId))),
allOf(instanceOf(ModelException.class),
hasCause(instanceOf(PessimisticLockException.class))));
// start new transaction and read again, it should be successful
tx3.begin();
tx3.runStep(session -> LockObjectsForModification.lockRealmsForModification(session, () -> session.realms().getRealm(realmId)));
tx3.commit();
// end both transactions
tx2.rollback();
tx1.commit();
// start new transaction and read again, it should be successful
tx3.begin();
tx3.runStep(session -> LockObjectsForModification.lockRealmsForModification(session, () -> session.realms().getRealm(realmId)));
tx3.commit();
} finally {
if (originalTimeoutValue == null) {
System.clearProperty(LOCK_TIMEOUT_SYSTEM_PROPERTY);
} else {
System.setProperty(LOCK_TIMEOUT_SYSTEM_PROPERTY, originalTimeoutValue);
}
reinitializeKeycloakSessionFactory();
}
}
@Test