From 29f2481fd2bc2903e1ea7cb6f21c8f098d43542c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joaqu=C3=ADn?= Date: Wed, 7 Aug 2024 16:33:13 +0200 Subject: [PATCH] feat(engine): add property and check for transaction isolation level on datasource initialisation Related to https://github.com/camunda/camunda-bpm-platform/issues/4516 --- .../engine/impl/cfg/ConfigurationLogger.java | 15 +++ .../cfg/ProcessEngineConfigurationImpl.java | 72 ++++++++++-- .../cfg/ProcessEngineConfigurationTest.java | 105 ++++++++++++++++++ .../camunda.cfg.customIsolationLevel.xml | 52 +++++++++ 4 files changed, 236 insertions(+), 8 deletions(-) create mode 100644 engine/src/test/resources/camunda.cfg.customIsolationLevel.xml diff --git a/engine/src/main/java/org/camunda/bpm/engine/impl/cfg/ConfigurationLogger.java b/engine/src/main/java/org/camunda/bpm/engine/impl/cfg/ConfigurationLogger.java index 0fdd3b86db0..de8f27e7532 100644 --- a/engine/src/main/java/org/camunda/bpm/engine/impl/cfg/ConfigurationLogger.java +++ b/engine/src/main/java/org/camunda/bpm/engine/impl/cfg/ConfigurationLogger.java @@ -137,4 +137,19 @@ public NotValidException logErrorNoTTLConfigured() { + "* Set a default historyTimeToLive as a global process engine configuration\n" + "* (Not recommended) Deactivate the enforceTTL config to disable this check")); } + + public void logNullIsolationLevel(String defaultIsolationLevel) { + logInfo("019", + "Property transactionIsolationLevel has a null or empty value and the global database configuration is different from '{}'." + + "This may produce deadlocks or other unexpected behaviors.", + defaultIsolationLevel); + } + + public void logNonOptimalIsolationLevel(String propertyValue, String defaultIsolationLevel) { + logWarn("020", + "Property transactionIsolationLevel has value '{}' which is different from '{}'. " + + "This may produce deadlocks or other unexpected behaviors.", + propertyValue, + defaultIsolationLevel); + } } diff --git a/engine/src/main/java/org/camunda/bpm/engine/impl/cfg/ProcessEngineConfigurationImpl.java b/engine/src/main/java/org/camunda/bpm/engine/impl/cfg/ProcessEngineConfigurationImpl.java index 14cbb3deca3..43a718d1800 100644 --- a/engine/src/main/java/org/camunda/bpm/engine/impl/cfg/ProcessEngineConfigurationImpl.java +++ b/engine/src/main/java/org/camunda/bpm/engine/impl/cfg/ProcessEngineConfigurationImpl.java @@ -26,8 +26,6 @@ import java.nio.charset.Charset; import java.sql.Connection; import java.sql.DatabaseMetaData; -import java.sql.PreparedStatement; -import java.sql.ResultSet; import java.sql.SQLException; import java.text.ParseException; import java.util.ArrayList; @@ -47,6 +45,7 @@ import javax.sql.DataSource; import org.apache.ibatis.builder.xml.XMLConfigBuilder; import org.apache.ibatis.datasource.pooled.PooledDataSource; +import org.apache.ibatis.datasource.unpooled.UnpooledDataSource; import org.apache.ibatis.mapping.Environment; import org.apache.ibatis.session.Configuration; import org.apache.ibatis.session.ExecutorType; @@ -130,7 +129,6 @@ import org.camunda.bpm.engine.impl.cfg.multitenancy.TenantCommandChecker; import org.camunda.bpm.engine.impl.cfg.multitenancy.TenantIdProvider; import org.camunda.bpm.engine.impl.cfg.standalone.StandaloneTransactionContextFactory; -import org.camunda.bpm.engine.impl.cmd.HistoryCleanupCmd; import org.camunda.bpm.engine.impl.cmmn.CaseServiceImpl; import org.camunda.bpm.engine.impl.cmmn.deployer.CmmnDeployer; import org.camunda.bpm.engine.impl.cmmn.entity.repository.CaseDefinitionManager; @@ -395,7 +393,7 @@ */ public abstract class ProcessEngineConfigurationImpl extends ProcessEngineConfiguration { - protected final static ConfigurationLogger LOG = ConfigurationLogger.CONFIG_LOGGER; + protected static ConfigurationLogger LOG = ConfigurationLogger.CONFIG_LOGGER; public static final String DB_SCHEMA_UPDATE_CREATE = "create"; public static final String DB_SCHEMA_UPDATE_DROP_CREATE = "drop-create"; @@ -413,11 +411,19 @@ public abstract class ProcessEngineConfigurationImpl extends ProcessEngineConfig public static final int DEFAULT_INVOCATIONS_PER_BATCH_JOB = 1; + public static final String TRANSACTION_ISOLATION_LEVEL_DEFAULT = "readCommitted"; + + public static SqlSessionFactory cachedSqlSessionFactory; + protected static final Map DEFAULT_BEANS_MAP = new HashMap<>(); protected static final String PRODUCT_NAME = "Camunda BPM Runtime"; - public static SqlSessionFactory cachedSqlSessionFactory; + protected static final Map TRANSACTION_ISOLATION_LEVEL_MAPPING = Map.of( + "readCommitted", Connection.TRANSACTION_READ_COMMITTED, + "readUncommitted", Connection.TRANSACTION_READ_UNCOMMITTED, + "repeatableRead", Connection.TRANSACTION_REPEATABLE_READ, + "serializable", Connection.TRANSACTION_SERIALIZABLE); // SERVICES ///////////////////////////////////////////////////////////////// @@ -439,7 +445,7 @@ public abstract class ProcessEngineConfigurationImpl extends ProcessEngineConfig // Command executor and interceptor stack /** - * the configurable list which will be {@link #initInterceptorChain(java.util.List) processed} to build the {@link #commandExecutorTxRequired} + * the configurable list which will be {@link #initInterceptorChain(List) processed} to build the {@link #commandExecutorTxRequired} */ protected List customPreCommandInterceptorsTxRequired; protected List customPostCommandInterceptorsTxRequired; @@ -606,6 +612,7 @@ public abstract class ProcessEngineConfigurationImpl extends ProcessEngineConfig protected boolean enableScriptEngineLoadExternalResources = false; protected boolean enableScriptEngineNashornCompatibility = false; protected boolean configureScriptEngineHostAccess = true; + protected String transactionIsolationLevel = TRANSACTION_ISOLATION_LEVEL_DEFAULT; /** * When set to false, the following behavior changes: @@ -749,7 +756,7 @@ public abstract class ProcessEngineConfigurationImpl extends ProcessEngineConfig protected DmnHistoryEventProducer dmnHistoryEventProducer; /** - * As an instance of {@link org.camunda.bpm.engine.impl.history.handler.CompositeHistoryEventHandler} + * As an instance of {@link CompositeHistoryEventHandler} * it contains all the provided history event handlers that process history events. */ protected HistoryEventHandler historyEventHandler; @@ -1261,7 +1268,7 @@ public void initHistoryCleanup() { //validate number of threads if (historyCleanupDegreeOfParallelism < 1 || historyCleanupDegreeOfParallelism > MAX_THREADS_NUMBER) { throw LOG.invalidPropertyValue("historyCleanupDegreeOfParallelism", String.valueOf(historyCleanupDegreeOfParallelism), - String.format("value for number of threads for history cleanup should be between 1 and %s", HistoryCleanupCmd.MAX_THREADS_NUMBER)); + String.format("value for number of threads for history cleanup should be between 1 and %s", MAX_THREADS_NUMBER)); } if (historyCleanupBatchWindowStartTime != null) { @@ -1708,12 +1715,52 @@ protected void initDataSource() { ((PooledDataSource) dataSource).forceCloseAll(); } } + configureDefaultIsolationLevel(); if (databaseType == null) { initDatabaseType(); } } + protected void configureDefaultIsolationLevel() { + if (transactionIsolationLevel == null || transactionIsolationLevel.isBlank()) { + int currentIsolationLevel = getCurrentTransactionIsolationLevel(); + if (currentIsolationLevel != Connection.TRANSACTION_READ_COMMITTED) { + LOG.logNullIsolationLevel(TRANSACTION_ISOLATION_LEVEL_DEFAULT); + } + } else if (!TRANSACTION_ISOLATION_LEVEL_MAPPING.containsKey(transactionIsolationLevel)) { + throw LOG.invalidPropertyValue("transactionIsolationLevel", transactionIsolationLevel); + } else { + final int isolationLevel = TRANSACTION_ISOLATION_LEVEL_MAPPING.get(transactionIsolationLevel); + if (isolationLevel != Connection.TRANSACTION_READ_COMMITTED) { + LOG.logNonOptimalIsolationLevel(transactionIsolationLevel, TRANSACTION_ISOLATION_LEVEL_DEFAULT); + } + setDefaultTransactionIsolationLevel(isolationLevel); + } + } + + protected Integer getCurrentTransactionIsolationLevel() { + try (Connection connection = dataSource.getConnection()) { + return connection.getTransactionIsolation(); + } catch (SQLException e) { + throw LOG.databaseConnectionAccessException(e); + } + } + + protected void setDefaultTransactionIsolationLevel(int isolationLevel) { + if (dataSource instanceof PooledDataSource) { + ((PooledDataSource) dataSource).setDefaultTransactionIsolationLevel(isolationLevel); + } else if (dataSource instanceof UnpooledDataSource){ + ((UnpooledDataSource) dataSource).setDefaultTransactionIsolationLevel(isolationLevel); + } else { + try (Connection connection = dataSource.getConnection()) { + connection.setTransactionIsolation(isolationLevel); + } catch (SQLException e) { + throw LOG.databaseConnectionAccessException(e); + } + } + } + protected static Properties databaseTypeMappings = getDefaultDatabaseTypeMappings(); protected static final String MY_SQL_PRODUCT_NAME = "MySQL"; protected static final String MARIA_DB_PRODUCT_NAME = "MariaDB"; @@ -3584,6 +3631,15 @@ public void setBeans(Map beans) { this.beans = beans; } + public String getTransactionIsolationLevel() { + return transactionIsolationLevel; + } + + public ProcessEngineConfigurationImpl setTransactionIsolationLevel(String transactionIsolationLevel) { + this.transactionIsolationLevel = transactionIsolationLevel; + return this; + } + @Override public ProcessEngineConfigurationImpl setClassLoader(ClassLoader classLoader) { super.setClassLoader(classLoader); diff --git a/engine/src/test/java/org/camunda/bpm/engine/impl/cfg/ProcessEngineConfigurationTest.java b/engine/src/test/java/org/camunda/bpm/engine/impl/cfg/ProcessEngineConfigurationTest.java index c357f81c7c1..9c89c5bfcdf 100644 --- a/engine/src/test/java/org/camunda/bpm/engine/impl/cfg/ProcessEngineConfigurationTest.java +++ b/engine/src/test/java/org/camunda/bpm/engine/impl/cfg/ProcessEngineConfigurationTest.java @@ -17,11 +17,39 @@ package org.camunda.bpm.engine.impl.cfg; import static org.assertj.core.api.Assertions.*; +import static org.camunda.bpm.engine.impl.ProcessEngineLogger.CONFIG_LOGGER; +import static org.camunda.bpm.engine.impl.cfg.ProcessEngineConfigurationImpl.TRANSACTION_ISOLATION_LEVEL_DEFAULT; +import static org.junit.Assert.assertEquals; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.sql.Connection; import org.camunda.bpm.engine.ProcessEngineConfiguration; +import org.camunda.bpm.engine.ProcessEngineException; +import org.junit.AfterClass; +import org.junit.Before; import org.junit.Test; public class ProcessEngineConfigurationTest { + private ProcessEngineConfigurationImpl engineConfiguration; + private ConfigurationLogger logger; + + @Before + public void setUp() { + this.engineConfiguration = (ProcessEngineConfigurationImpl) ProcessEngineConfiguration.createProcessEngineConfigurationFromResourceDefault(); + this.logger = mock(ConfigurationLogger.class); + ProcessEngineConfigurationImpl.LOG = logger; + } + + @AfterClass + public static void cleanUp() { + ProcessEngineConfigurationImpl.LOG = CONFIG_LOGGER; + } + @Test public void shouldEnableStandaloneTasksByDefault() { // when @@ -39,4 +67,81 @@ public void shouldEnableImplicitUpdatesDetectionByDefault() { // then assertThat(engineConfiguration.isImplicitVariableUpdateDetectionEnabled()).isTrue(); } + + @Test + public void validIsolationLevelPropertyIsSetCorrectly() { + //given + engineConfiguration.setTransactionIsolationLevel("serializable"); + + //when + engineConfiguration.initDataSource(); + + //then + assertEquals(Connection.TRANSACTION_SERIALIZABLE, engineConfiguration.getCurrentTransactionIsolationLevel().intValue()); + verify(logger).logNonOptimalIsolationLevel("serializable", TRANSACTION_ISOLATION_LEVEL_DEFAULT); + } + + @Test + public void validIsolationLevelPropertyFromFileIsSetCorrectly() { + //given + ProcessEngineConfigurationImpl engineConfiguration = (ProcessEngineConfigurationImpl) ProcessEngineConfiguration.createProcessEngineConfigurationFromResource("camunda.cfg.customIsolationLevel.xml"); + + //when + engineConfiguration.initDataSource(); + + //then + assertEquals(Connection.TRANSACTION_SERIALIZABLE, engineConfiguration.getCurrentTransactionIsolationLevel().intValue()); + verify(logger).logNonOptimalIsolationLevel("serializable", TRANSACTION_ISOLATION_LEVEL_DEFAULT); + } + + @Test + public void unsetIsolationLevelPropertyDefaultsToExpectedValue() { + //when + engineConfiguration.initDataSource(); + + //then + assertEquals("readCommitted", engineConfiguration.getTransactionIsolationLevel()); + assertEquals(Connection.TRANSACTION_READ_COMMITTED, engineConfiguration.getCurrentTransactionIsolationLevel().intValue()); + verify(logger, times(0)).logNonOptimalIsolationLevel(anyString(), anyString()); + } + + @Test + public void nullIsolationLevelPropertyIsHandledCorrectly() { + //given + engineConfiguration.setTransactionIsolationLevel(null); + + //when + engineConfiguration.initDataSource(); + + //then + assertEquals(Connection.TRANSACTION_READ_COMMITTED, engineConfiguration.getCurrentTransactionIsolationLevel().intValue()); + verify(logger, times(0)).logNullIsolationLevel(TRANSACTION_ISOLATION_LEVEL_DEFAULT); + } + + @Test + public void blankIsolationLevelPropertyIsHandledCorrectly() { + //given + engineConfiguration.setTransactionIsolationLevel(" "); + + //when + engineConfiguration.initDataSource(); + + //then + assertEquals(Connection.TRANSACTION_READ_COMMITTED, engineConfiguration.getCurrentTransactionIsolationLevel().intValue()); + verify(logger, times(0)).logNullIsolationLevel(TRANSACTION_ISOLATION_LEVEL_DEFAULT); + } + + @Test + public void invalidIsolationLevelPropertyIsHandledCorrectly() { + //given + String propertyName = "transactionIsolationLevel"; + String propertyValue = "invalid"; + ProcessEngineException expectedException = CONFIG_LOGGER.invalidPropertyValue(propertyName, propertyValue); + engineConfiguration.setTransactionIsolationLevel(propertyValue); + when(logger.invalidPropertyValue(anyString(), anyString())).thenReturn(expectedException); + + //when then + assertThatThrownBy(() -> engineConfiguration.initDataSource()).isEqualTo(expectedException); + verify(logger).invalidPropertyValue(propertyName, propertyValue); + } } diff --git a/engine/src/test/resources/camunda.cfg.customIsolationLevel.xml b/engine/src/test/resources/camunda.cfg.customIsolationLevel.xml new file mode 100644 index 00000000000..12a5422baf1 --- /dev/null +++ b/engine/src/test/resources/camunda.cfg.customIsolationLevel.xml @@ -0,0 +1,52 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +