From 4390abe92e0fba5875214fdc8063b04e76d4c31d Mon Sep 17 00:00:00 2001 From: Sattvik Chakravarthy Date: Fri, 9 Feb 2024 13:02:37 +0530 Subject: [PATCH] fix: backport more changes --- CHANGELOG.md | 1 + config.yaml | 8 + devConfig.yaml | 8 + .../storage/postgresql/ConnectionPool.java | 2 + .../supertokens/storage/postgresql/Start.java | 3 +- .../postgresql/config/PostgreSQLConfig.java | 27 ++ .../postgresql/test/DbConnectionPoolTest.java | 290 ++++++++++++++---- .../test/SuperTokensSaaSSecretTest.java | 6 +- 8 files changed, 285 insertions(+), 60 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 54b8dc66..85b76e9b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [4.0.3] - 2024-02-07 - Adds tests to check connection pool behaviour. +- Adds `postgresql_idle_connection_timeout` and `postgresql_minimum_idle_connections` configs to control active connections to the database. ## [4.0.2] diff --git a/config.yaml b/config.yaml index 36459b8d..81032770 100644 --- a/config.yaml +++ b/config.yaml @@ -67,3 +67,11 @@ postgresql_config_version: 0 # (DIFFERENT_ACROSS_TENANTS | OPTIONAL | Default: "thirdparty_users") string value. Specify the name of the table # that will store the thirdparty recipe users. # postgresql_thirdparty_users_table_name + +# (DIFFERENT_ACROSS_TENANTS | OPTIONAL | Default: 60000) long value. Timeout in milliseconds for the idle connections +# to be closed. +# postgresql_idle_connection_timeout: + +# (DIFFERENT_ACROSS_TENANTS | OPTIONAL | Default: 1) integer value. Minimum number of idle connections to be kept +# active. +# postgresql_minimum_idle_connections: diff --git a/devConfig.yaml b/devConfig.yaml index 39d0d5ed..3af330ff 100644 --- a/devConfig.yaml +++ b/devConfig.yaml @@ -69,3 +69,11 @@ postgresql_password: "root" # (DIFFERENT_ACROSS_TENANTS | OPTIONAL | Default: "thirdparty_users") string value. Specify the name of the table # that will store the thirdparty recipe users. # postgresql_thirdparty_users_table_name + +# (DIFFERENT_ACROSS_TENANTS | OPTIONAL | Default: 60000) long value. Timeout in milliseconds for the idle connections +# to be closed. +# postgresql_idle_connection_timeout: + +# (DIFFERENT_ACROSS_TENANTS | OPTIONAL | Default: 1) integer value. Minimum number of idle connections to be kept +# active. +# postgresql_minimum_idle_connections: diff --git a/src/main/java/io/supertokens/storage/postgresql/ConnectionPool.java b/src/main/java/io/supertokens/storage/postgresql/ConnectionPool.java index f7dbc287..4f5cc53a 100644 --- a/src/main/java/io/supertokens/storage/postgresql/ConnectionPool.java +++ b/src/main/java/io/supertokens/storage/postgresql/ConnectionPool.java @@ -81,6 +81,8 @@ private synchronized void initialiseHikariDataSource() throws SQLException { } config.setMaximumPoolSize(userConfig.getConnectionPoolSize()); config.setConnectionTimeout(5000); + config.setIdleTimeout(userConfig.getIdleConnectionTimeout()); + config.setMinimumIdle(userConfig.getMinimumIdleConnections()); config.addDataSourceProperty("cachePrepStmts", "true"); config.addDataSourceProperty("prepStmtCacheSize", "250"); config.addDataSourceProperty("prepStmtCacheSqlLimit", "2048"); diff --git a/src/main/java/io/supertokens/storage/postgresql/Start.java b/src/main/java/io/supertokens/storage/postgresql/Start.java index ecdd72b4..f0eb802c 100644 --- a/src/main/java/io/supertokens/storage/postgresql/Start.java +++ b/src/main/java/io/supertokens/storage/postgresql/Start.java @@ -111,7 +111,8 @@ public class Start // SaaS. If the core is not running in SuperTokens SaaS, this array has no effect. private static String[] PROTECTED_DB_CONFIG = new String[]{"postgresql_connection_pool_size", "postgresql_connection_uri", "postgresql_host", "postgresql_port", "postgresql_user", "postgresql_password", - "postgresql_database_name", "postgresql_table_schema"}; + "postgresql_database_name", "postgresql_table_schema", "postgresql_idle_connection_timeout", + "postgresql_minimum_idle_connections"}; private static final Object appenderLock = new Object(); public static boolean silent = false; private ResourceDistributor resourceDistributor = new ResourceDistributor(); diff --git a/src/main/java/io/supertokens/storage/postgresql/config/PostgreSQLConfig.java b/src/main/java/io/supertokens/storage/postgresql/config/PostgreSQLConfig.java index e8bc81d6..e94950c0 100644 --- a/src/main/java/io/supertokens/storage/postgresql/config/PostgreSQLConfig.java +++ b/src/main/java/io/supertokens/storage/postgresql/config/PostgreSQLConfig.java @@ -112,6 +112,14 @@ public class PostgreSQLConfig { @ConnectionPoolProperty private String postgresql_connection_scheme = "postgresql"; + @JsonProperty + @ConnectionPoolProperty + private long postgresql_idle_connection_timeout = 60000; + + @JsonProperty + @ConnectionPoolProperty + private int postgresql_minimum_idle_connections = 1; + @IgnoreForAnnotationCheck boolean isValidAndNormalised = false; @@ -234,6 +242,14 @@ public String getThirdPartyUsersTable() { return postgresql_thirdparty_users_table_name; } + public long getIdleConnectionTimeout() { + return postgresql_idle_connection_timeout; + } + + public int getMinimumIdleConnections() { + return postgresql_minimum_idle_connections; + } + public String getThirdPartyUserToTenantTable() { return addSchemaAndPrefixToTableName("thirdparty_user_to_tenant"); } @@ -340,6 +356,17 @@ public void validateAndNormalise() throws InvalidConfigException { "'postgresql_connection_pool_size' in the config.yaml file must be > 0"); } + if (postgresql_minimum_idle_connections <= 0) { + throw new InvalidConfigException( + "'postgresql_minimum_idle_connections' must be a positive value"); + } + + if (postgresql_minimum_idle_connections > postgresql_connection_pool_size) { + throw new InvalidConfigException( + "'postgresql_minimum_idle_connections' must be less than or equal to " + + "'postgresql_connection_pool_size'"); + } + // Normalisation if (postgresql_connection_uri != null) { { // postgresql_connection_attributes diff --git a/src/test/java/io/supertokens/storage/postgresql/test/DbConnectionPoolTest.java b/src/test/java/io/supertokens/storage/postgresql/test/DbConnectionPoolTest.java index 7f4c8b7d..8ce2fbfc 100644 --- a/src/test/java/io/supertokens/storage/postgresql/test/DbConnectionPoolTest.java +++ b/src/test/java/io/supertokens/storage/postgresql/test/DbConnectionPoolTest.java @@ -16,31 +16,33 @@ package io.supertokens.storage.postgresql.test; +import static org.junit.Assert.*; + +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicLong; + +import io.supertokens.pluginInterface.multitenancy.*; +import org.junit.AfterClass; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TestRule; + import com.google.gson.JsonObject; + import io.supertokens.ProcessState; import io.supertokens.featureflag.EE_FEATURES; import io.supertokens.featureflag.FeatureFlagTestContent; import io.supertokens.multitenancy.Multitenancy; import io.supertokens.multitenancy.exception.BadPermissionException; import io.supertokens.pluginInterface.exceptions.StorageQueryException; -import io.supertokens.pluginInterface.multitenancy.*; import io.supertokens.pluginInterface.multitenancy.exceptions.TenantOrAppNotFoundException; import io.supertokens.storage.postgresql.Start; import io.supertokens.storageLayer.StorageLayer; import io.supertokens.thirdparty.ThirdParty; -import org.junit.AfterClass; -import org.junit.Before; -import org.junit.Rule; -import org.junit.Test; -import org.junit.rules.TestRule; - -import java.util.concurrent.ExecutorService; -import java.util.concurrent.Executors; -import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicInteger; -import java.util.concurrent.atomic.AtomicLong; - -import static org.junit.Assert.*; public class DbConnectionPoolTest { @Rule @@ -63,6 +65,7 @@ public void testActiveConnectionsWithTenants() throws Exception { TestingProcessManager.TestingProcess process = TestingProcessManager.start(args, false); FeatureFlagTestContent.getInstance(process.getProcess()) .setKeyValue(FeatureFlagTestContent.ENABLED_FEATURES, new EE_FEATURES[]{EE_FEATURES.MULTI_TENANCY}); + Utils.setValueInConfig("postgresql_minimum_idle_connections", "10"); process.startProcess(); assertNotNull(process.checkOrWaitForEvent(ProcessState.PROCESS_STATE.STARTED)); @@ -86,6 +89,7 @@ public void testActiveConnectionsWithTenants() throws Exception { // change connection pool size config.addProperty("postgresql_connection_pool_size", 20); + config.addProperty("postgresql_minimum_idle_connections", 20); Multitenancy.addNewOrUpdateAppOrTenant(process.getProcess(), new TenantConfig( new TenantIdentifier(null, null, "t1"), @@ -113,9 +117,212 @@ public void testActiveConnectionsWithTenants() throws Exception { public void testDownTimeWhenChangingConnectionPoolSize() throws Exception { String[] args = {"../"}; + for (int t = 0; t < 5; t++) { + TestingProcessManager.TestingProcess process = TestingProcessManager.start(args, false); + FeatureFlagTestContent.getInstance(process.getProcess()) + .setKeyValue(FeatureFlagTestContent.ENABLED_FEATURES, new EE_FEATURES[]{EE_FEATURES.MULTI_TENANCY}); + process.startProcess(); + Utils.setValueInConfig("postgresql_minimum_idle_connections", "10"); + assertNotNull(process.checkOrWaitForEvent(ProcessState.PROCESS_STATE.STARTED)); + + Start start = (Start) StorageLayer.getBaseStorage(process.getProcess()); + assertEquals(10, start.getDbActivityCount("supertokens")); + + JsonObject config = new JsonObject(); + start.modifyConfigToAddANewUserPoolForTesting(config, 1); + config.addProperty("postgresql_connection_pool_size", 300); + config.addProperty("postgresql_minimum_idle_connections", 300); + AtomicLong firstErrorTime = new AtomicLong(-1); + AtomicLong successAfterErrorTime = new AtomicLong(-1); + AtomicInteger errorCount = new AtomicInteger(0); + + Multitenancy.addNewOrUpdateAppOrTenant(process.getProcess(), new TenantConfig( + new TenantIdentifier(null, null, "t1"), + new EmailPasswordConfig(true), + new ThirdPartyConfig(true, null), + new PasswordlessConfig(true), + config + ), false); + + Thread.sleep(5000); // let the new tenant be ready + + assertEquals(300, start.getDbActivityCount("st1")); + + ExecutorService es = Executors.newFixedThreadPool(100); + + for (int i = 0; i < 10000; i++) { + int finalI = i; + es.execute(() -> { + try { + TenantIdentifier t1 = new TenantIdentifier(null, null, "t1"); + TenantIdentifierWithStorage t1WithStorage = t1.withStorage(StorageLayer.getStorage(t1, process.getProcess())); + ThirdParty.signInUp(t1WithStorage, process.getProcess(), "google", "googleid"+ finalI, "user" + + finalI + "@example.com"); + + if (firstErrorTime.get() != -1 && successAfterErrorTime.get() == -1) { + successAfterErrorTime.set(System.currentTimeMillis()); + } + } catch (StorageQueryException e) { + if (e.getMessage().contains("Connection is closed") || e.getMessage().contains("has been closed")) { + if (firstErrorTime.get() == -1) { + firstErrorTime.set(System.currentTimeMillis()); + } + } else { + errorCount.incrementAndGet(); + throw new RuntimeException(e); + } + } catch (TenantOrAppNotFoundException e) { + errorCount.incrementAndGet(); + throw new RuntimeException(e); + } catch (BadPermissionException e) { + errorCount.incrementAndGet(); + throw new RuntimeException(e); + } catch (IllegalStateException e) { + if (e.getMessage().contains("Please call initPool before getConnection")) { + if (firstErrorTime.get() == -1) { + firstErrorTime.set(System.currentTimeMillis()); + } + } else { + errorCount.incrementAndGet(); + throw e; + } + } + }); + } + + // change connection pool size + config.addProperty("postgresql_connection_pool_size", 200); + config.addProperty("postgresql_minimum_idle_connections", 200); + + Multitenancy.addNewOrUpdateAppOrTenant(process.getProcess(), new TenantConfig( + new TenantIdentifier(null, null, "t1"), + new EmailPasswordConfig(true), + new ThirdPartyConfig(true, null), + new PasswordlessConfig(true), + config + ), false); + + Thread.sleep(3000); // let the new tenant be ready + + es.shutdown(); + es.awaitTermination(2, TimeUnit.MINUTES); + + assertEquals(0, errorCount.get()); + + assertEquals(200, start.getDbActivityCount("st1")); + + // delete tenant + Multitenancy.deleteTenant(new TenantIdentifier(null, null, "t1"), process.getProcess()); + Thread.sleep(3000); // let the tenant be deleted + + assertEquals(0, start.getDbActivityCount("st1")); + + System.out.println(successAfterErrorTime.get() - firstErrorTime.get() + "ms"); + assertTrue(successAfterErrorTime.get() - firstErrorTime.get() < 250); + + if (successAfterErrorTime.get() - firstErrorTime.get() == 0) { + process.kill(); + assertNotNull(process.checkOrWaitForEvent(ProcessState.PROCESS_STATE.STOPPED)); + continue; // retry + } + + assertTrue(successAfterErrorTime.get() - firstErrorTime.get() > 0); + process.kill(); + assertNotNull(process.checkOrWaitForEvent(ProcessState.PROCESS_STATE.STOPPED)); + + return; + } + + fail(); // tried 5 times + } + + + @Test + public void testMinimumIdleConnections() throws Exception { + String[] args = {"../"}; + { + TestingProcessManager.TestingProcess process = TestingProcessManager.start(args, false); + FeatureFlagTestContent.getInstance(process.getProcess()) + .setKeyValue(FeatureFlagTestContent.ENABLED_FEATURES, new EE_FEATURES[]{EE_FEATURES.MULTI_TENANCY}); + Utils.setValueInConfig("postgresql_connection_pool_size", "20"); + Utils.setValueInConfig("postgresql_minimum_idle_connections", "10"); + Utils.setValueInConfig("postgresql_idle_connection_timeout", "30000"); + process.startProcess(); + assertNotNull(process.checkOrWaitForEvent(ProcessState.PROCESS_STATE.STARTED)); + + Thread.sleep(65000); // let the idle connections time out + + Start start = (Start) StorageLayer.getBaseStorage(process.getProcess()); + assertEquals(10, start.getDbActivityCount("supertokens")); + + process.kill(); + assertNotNull(process.checkOrWaitForEvent(ProcessState.PROCESS_STATE.STOPPED)); + } + } + + @Test + public void testMinimumIdleConnectionForTenants() throws Exception { + String[] args = {"../"}; + TestingProcessManager.TestingProcess process = TestingProcessManager.start(args, false); FeatureFlagTestContent.getInstance(process.getProcess()) .setKeyValue(FeatureFlagTestContent.ENABLED_FEATURES, new EE_FEATURES[]{EE_FEATURES.MULTI_TENANCY}); + Utils.setValueInConfig("postgresql_minimum_idle_connections", "10"); + process.startProcess(); + assertNotNull(process.checkOrWaitForEvent(ProcessState.PROCESS_STATE.STARTED)); + + Start start = (Start) StorageLayer.getBaseStorage(process.getProcess()); + assertEquals(10, start.getDbActivityCount("supertokens")); + + JsonObject config = new JsonObject(); + start.modifyConfigToAddANewUserPoolForTesting(config, 1); + + Multitenancy.addNewOrUpdateAppOrTenant(process.getProcess(), new TenantConfig( + new TenantIdentifier(null, null, "t1"), + new EmailPasswordConfig(true), + new ThirdPartyConfig(true, null), + new PasswordlessConfig(true), + config + ), false); + + Thread.sleep(1000); // let the new tenant be ready + + assertEquals(10, start.getDbActivityCount("st1")); + + // change connection pool size + config.addProperty("postgresql_connection_pool_size", 20); + config.addProperty("postgresql_minimum_idle_connections", 5); + + Multitenancy.addNewOrUpdateAppOrTenant(process.getProcess(), new TenantConfig( + new TenantIdentifier(null, null, "t1"), + new EmailPasswordConfig(true), + new ThirdPartyConfig(true, null), + new PasswordlessConfig(true), + config + ), false); + + Thread.sleep(2000); // let the new tenant be ready + + assertEquals(5, start.getDbActivityCount("st1")); + + // delete tenant + Multitenancy.deleteTenant(new TenantIdentifier(null, null, "t1"), process.getProcess()); + Thread.sleep(2000); // let the tenant be deleted + + assertEquals(0, start.getDbActivityCount("st1")); + + process.kill(); + assertNotNull(process.checkOrWaitForEvent(ProcessState.PROCESS_STATE.STOPPED)); + } + + @Test + public void testIdleConnectionTimeout() throws Exception { + String[] args = {"../"}; + + TestingProcessManager.TestingProcess process = TestingProcessManager.start(args, false); + FeatureFlagTestContent.getInstance(process.getProcess()) + .setKeyValue(FeatureFlagTestContent.ENABLED_FEATURES, new EE_FEATURES[]{EE_FEATURES.MULTI_TENANCY}); + Utils.setValueInConfig("postgresql_minimum_idle_connections", "10"); process.startProcess(); assertNotNull(process.checkOrWaitForEvent(ProcessState.PROCESS_STATE.STARTED)); @@ -125,9 +332,10 @@ public void testDownTimeWhenChangingConnectionPoolSize() throws Exception { JsonObject config = new JsonObject(); start.modifyConfigToAddANewUserPoolForTesting(config, 1); config.addProperty("postgresql_connection_pool_size", 300); - AtomicLong firstErrorTime = new AtomicLong(-1); - AtomicLong successAfterErrorTime = new AtomicLong(-1); - AtomicInteger errorCount = new AtomicInteger(0); + config.addProperty("postgresql_minimum_idle_connections", 5); + config.addProperty("postgresql_idle_connection_timeout", 30000); + + AtomicLong errorCount = new AtomicLong(0); Multitenancy.addNewOrUpdateAppOrTenant(process.getProcess(), new TenantConfig( new TenantIdentifier(null, null, "t1"), @@ -139,9 +347,9 @@ public void testDownTimeWhenChangingConnectionPoolSize() throws Exception { Thread.sleep(3000); // let the new tenant be ready - assertEquals(300, start.getDbActivityCount("st1")); + assertTrue(10 >= start.getDbActivityCount("st1")); - ExecutorService es = Executors.newFixedThreadPool(100); + ExecutorService es = Executors.newFixedThreadPool(150); for (int i = 0; i < 10000; i++) { int finalI = i; @@ -152,56 +360,29 @@ public void testDownTimeWhenChangingConnectionPoolSize() throws Exception { ThirdParty.signInUp(t1WithStorage, process.getProcess(), "google", "googleid"+ finalI, "user" + finalI + "@example.com"); - if (firstErrorTime.get() != -1 && successAfterErrorTime.get() == -1) { - successAfterErrorTime.set(System.currentTimeMillis()); - } } catch (StorageQueryException e) { - if (e.getMessage().contains("Connection is closed") || e.getMessage().contains("has been closed")) { - if (firstErrorTime.get() == -1) { - firstErrorTime.set(System.currentTimeMillis()); - } - } else { - errorCount.incrementAndGet(); - throw new RuntimeException(e); - } + errorCount.incrementAndGet(); + throw new RuntimeException(e); } catch (TenantOrAppNotFoundException e) { errorCount.incrementAndGet(); throw new RuntimeException(e); } catch (BadPermissionException e) { errorCount.incrementAndGet(); throw new RuntimeException(e); - } catch (IllegalStateException e) { - if (e.getMessage().contains("Please call initPool before getConnection")) { - if (firstErrorTime.get() == -1) { - firstErrorTime.set(System.currentTimeMillis()); - } - } else { - errorCount.incrementAndGet(); - throw e; - } } }); } - // change connection pool size - config.addProperty("postgresql_connection_pool_size", 200); - - Multitenancy.addNewOrUpdateAppOrTenant(process.getProcess(), new TenantConfig( - new TenantIdentifier(null, null, "t1"), - new EmailPasswordConfig(true), - new ThirdPartyConfig(true, null), - new PasswordlessConfig(true), - config - ), false); - - Thread.sleep(3000); // let the new tenant be ready - es.shutdown(); es.awaitTermination(2, TimeUnit.MINUTES); + assertTrue(5 < start.getDbActivityCount("st1")); + assertEquals(0, errorCount.get()); - assertEquals(200, start.getDbActivityCount("st1")); + Thread.sleep(65000); // let the idle connections time out + + assertEquals(5, start.getDbActivityCount("st1")); // delete tenant Multitenancy.deleteTenant(new TenantIdentifier(null, null, "t1"), process.getProcess()); @@ -209,9 +390,6 @@ public void testDownTimeWhenChangingConnectionPoolSize() throws Exception { assertEquals(0, start.getDbActivityCount("st1")); - System.out.println(successAfterErrorTime.get() - firstErrorTime.get() + "ms"); - assertTrue(successAfterErrorTime.get() - firstErrorTime.get() < 250); - assertTrue(successAfterErrorTime.get() - firstErrorTime.get() > 0); process.kill(); assertNotNull(process.checkOrWaitForEvent(ProcessState.PROCESS_STATE.STOPPED)); } diff --git a/src/test/java/io/supertokens/storage/postgresql/test/SuperTokensSaaSSecretTest.java b/src/test/java/io/supertokens/storage/postgresql/test/SuperTokensSaaSSecretTest.java index 17b6e437..6693acb1 100644 --- a/src/test/java/io/supertokens/storage/postgresql/test/SuperTokensSaaSSecretTest.java +++ b/src/test/java/io/supertokens/storage/postgresql/test/SuperTokensSaaSSecretTest.java @@ -45,12 +45,12 @@ public class SuperTokensSaaSSecretTest { private final String[] PROTECTED_DB_CONFIG = new String[]{"postgresql_connection_pool_size", "postgresql_connection_uri", "postgresql_host", "postgresql_port", "postgresql_user", - "postgresql_password", - "postgresql_database_name", "postgresql_table_schema"}; + "postgresql_password", "postgresql_database_name", "postgresql_table_schema", + "postgresql_minimum_idle_connections", "postgresql_idle_connection_timeout"}; private final Object[] PROTECTED_DB_CONFIG_VALUES = new Object[]{11, "postgresql://root:root@localhost:5432/supertokens?currentSchema=myschema", "localhost", 5432, "root", - "root", "supertokens", "myschema"}; + "root", "supertokens", "myschema", 5, 120000}; @Rule public TestRule watchman = Utils.getOnFailure();