Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: resource reloading #673

Merged
merged 8 commits into from
May 12, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion ee/src/main/java/io/supertokens/ee/EEFeatureFlag.java
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import com.google.gson.*;
import io.supertokens.Main;
import io.supertokens.ProcessState;
import io.supertokens.cronjobs.Cronjobs;
import io.supertokens.cronjobs.telemetry.Telemetry;
import io.supertokens.ee.cronjobs.EELicenseCheck;
import io.supertokens.featureflag.EE_FEATURES;
Expand Down Expand Up @@ -93,7 +94,9 @@ public void updateEnabledFeaturesValueReadFromDbTime(long newTime) {
public void constructor(Main main, AppIdentifier appIdentifier) {
this.main = main;
this.appIdentifier = appIdentifier;
EELicenseCheck.getOrCreateInstance(main);

Cronjobs.addCronjob(main, EELicenseCheck.init(main, StorageLayer.getTenantsWithUniqueUserPoolId(main)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a comment above explaining that it doesn't add per constructor call even though it looks like that

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


try {
this.syncFeatureFlagWithLicenseKey();
} catch (HttpResponseException | IOException e) {
Expand Down
16 changes: 5 additions & 11 deletions ee/src/main/java/io/supertokens/ee/cronjobs/EELicenseCheck.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import io.supertokens.cronjobs.CronTask;
import io.supertokens.cronjobs.CronTaskTest;
import io.supertokens.cronjobs.Cronjobs;
import io.supertokens.cronjobs.deleteExpiredAccessTokenSigningKeys.DeleteExpiredAccessTokenSigningKeys;
import io.supertokens.ee.EEFeatureFlag;
import io.supertokens.featureflag.FeatureFlag;
import io.supertokens.pluginInterface.multitenancy.AppIdentifier;
Expand All @@ -16,21 +17,14 @@ public class EELicenseCheck extends CronTask {

public static final String RESOURCE_KEY = "io.supertokens.ee.cronjobs.EELicenseCheck";

private static EELicenseCheck instance = null;

private EELicenseCheck(Main main, List<List<TenantIdentifier>> tenantsInfo) {
super("EELicenseCheck", main, tenantsInfo, true);
}
rishabhpoddar marked this conversation as resolved.
Show resolved Hide resolved

public static synchronized EELicenseCheck getOrCreateInstance(Main main) {
if (instance != null) {
return instance;
}

instance = new EELicenseCheck(main, StorageLayer.getTenantsWithUniqueUserPoolId(main));
Cronjobs.addCronjob(main, instance);

return instance;
public static EELicenseCheck init(Main main, List<List<TenantIdentifier>> tenantsInfo) {
return (EELicenseCheck) main.getResourceDistributor()
.setResource(new TenantIdentifier(null, null, null), RESOURCE_KEY,
new EELicenseCheck(main, tenantsInfo));
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/io/supertokens/Main.java
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ private void init() throws IOException, StorageQueryException {
MultitenancyHelper.getInstance(this).loadConfig(new ArrayList<>());

// init storage layers for each unique db connection based on unique (user pool ID, connection pool ID).
MultitenancyHelper.getInstance(this).loadStorageLayer(new ArrayList<>());
MultitenancyHelper.getInstance(this).loadStorageLayer();
} catch (InvalidConfigException e) {
throw new QuitProgramException(e);
}
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/io/supertokens/ProcessState.java
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ public enum PROCESS_STATE {
PASSWORD_HASH_BCRYPT, PASSWORD_HASH_ARGON, PASSWORD_VERIFY_BCRYPT, PASSWORD_VERIFY_ARGON,
PASSWORD_VERIFY_FIREBASE_SCRYPT, ADDING_REMOTE_ADDRESS_FILTER, LICENSE_KEY_CHECK_NETWORK_CALL,
INVALID_LICENSE_KEY, SERVER_ERROR_DURING_LICENSE_KEY_CHECK_FAIL, LOADING_ALL_TENANT_CONFIG,
LOADING_ALL_TENANT_STORAGE
LOADING_ALL_TENANT_STORAGE, TENANTS_CHANGED_DURING_REFRESH_FROM_DB
}

public static class EventAndException {
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/io/supertokens/ResourceDistributor.java
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ public synchronized SingletonResource getResource(TenantIdentifier tenantIdentif
throw new TenantOrAppNotFoundException(tenantIdentifier);
}

MultitenancyHelper.getInstance(main).refreshTenantsInCoreIfRequired(true);
MultitenancyHelper.getInstance(main).refreshTenantsInCoreBasedOnChangesInCoreConfigOrIfTenantListChanged(true);

// we try again..
resource = resources.get(new KeyClass(tenantIdentifier, key));
Expand Down
4 changes: 3 additions & 1 deletion src/main/java/io/supertokens/cronjobs/Cronjobs.java
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,9 @@ public static void addCronjob(Main main, CronTask task) {
synchronized (instance.lock) {
instance.executor.scheduleWithFixedDelay(task, task.getInitialWaitTimeSeconds(),
task.getIntervalTimeSeconds(), TimeUnit.SECONDS);
instance.tasks.add(task);
if (!instance.tasks.contains(task)) {
instance.tasks.add(task);
}
Comment on lines +96 to +98
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a test for this

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

}
}

Expand Down
16 changes: 8 additions & 8 deletions src/main/java/io/supertokens/multitenancy/Multitenancy.java
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ public static boolean addNewOrUpdateAppOrTenant(Main main, TenantConfig newTenan
// the tenant being there in the tenants table. But that insertion is done in the addTenantIdInUserPool
// function below. So in order to actually refresh the resources, we have a finally block here which
// calls the forceReloadAllResources function.
tenantsThatChanged = MultitenancyHelper.getInstance(main).refreshTenantsInCoreIfRequired(false);
tenantsThatChanged = MultitenancyHelper.getInstance(main).refreshTenantsInCoreBasedOnChangesInCoreConfigOrIfTenantListChanged(false);
try {
((MultitenancyStorage) StorageLayer.getStorage(newTenant.tenantIdentifier, main))
.addTenantIdInTargetStorage(newTenant.tenantIdentifier);
Expand All @@ -220,7 +220,7 @@ public static boolean addNewOrUpdateAppOrTenant(Main main, TenantConfig newTenan
if (!creationInSharedDbSucceeded) {
try {
StorageLayer.getMultitenancyStorage(main).overwriteTenantConfig(newTenant);
tenantsThatChanged = MultitenancyHelper.getInstance(main).refreshTenantsInCoreIfRequired(false);
tenantsThatChanged = MultitenancyHelper.getInstance(main).refreshTenantsInCoreBasedOnChangesInCoreConfigOrIfTenantListChanged(false);

// we do this extra step cause if previously an attempt to add a tenant failed midway,
// such that the main tenant was added in the user pool, but did not get created
Expand Down Expand Up @@ -270,7 +270,7 @@ public static boolean deleteTenant(TenantIdentifier tenantIdentifier, Main main)
// but not from the main table.
}
boolean didExist = StorageLayer.getMultitenancyStorage(main).deleteTenantInfoInBaseStorage(tenantIdentifier);
MultitenancyHelper.getInstance(main).refreshTenantsInCoreIfRequired(true);
MultitenancyHelper.getInstance(main).refreshTenantsInCoreBasedOnChangesInCoreConfigOrIfTenantListChanged(true);
return didExist;
}

Expand All @@ -292,7 +292,7 @@ public static boolean deleteApp(AppIdentifier appIdentifier, Main main)
// but not from the main table.
}
boolean didExist = StorageLayer.getMultitenancyStorage(main).deleteAppInfoInBaseStorage(appIdentifier);
MultitenancyHelper.getInstance(main).refreshTenantsInCoreIfRequired(true);
MultitenancyHelper.getInstance(main).refreshTenantsInCoreBasedOnChangesInCoreConfigOrIfTenantListChanged(true);
return didExist;
}

Expand Down Expand Up @@ -322,7 +322,7 @@ public static boolean deleteConnectionUriDomain(String connectionUriDomain, Main
// but not from the main table.
}
boolean didExist = StorageLayer.getMultitenancyStorage(main).deleteConnectionUriDomainInfoInBaseStorage(connectionUriDomain);
MultitenancyHelper.getInstance(main).refreshTenantsInCoreIfRequired(true);
MultitenancyHelper.getInstance(main).refreshTenantsInCoreBasedOnChangesInCoreConfigOrIfTenantListChanged(true);
return didExist;
}

Expand Down Expand Up @@ -372,7 +372,7 @@ public static TenantConfig getTenantInfo(Main main, TenantIdentifier tenantIdent
}

public static TenantConfig[] getAllTenantsForApp(AppIdentifier appIdentifier, Main main) {
MultitenancyHelper.getInstance(main).refreshTenantsInCoreIfRequired(true);
MultitenancyHelper.getInstance(main).refreshTenantsInCoreBasedOnChangesInCoreConfigOrIfTenantListChanged(true);
TenantConfig[] tenants = MultitenancyHelper.getInstance(main).getAllTenants();
List<TenantConfig> tenantList = new ArrayList<>();

Expand All @@ -394,7 +394,7 @@ public static TenantConfig[] getAllAppsAndTenantsForConnectionUriDomain(String c
if (connectionUriDomain == null) {
connectionUriDomain = TenantIdentifier.DEFAULT_CONNECTION_URI;
}
MultitenancyHelper.getInstance(main).refreshTenantsInCoreIfRequired(true);
MultitenancyHelper.getInstance(main).refreshTenantsInCoreBasedOnChangesInCoreConfigOrIfTenantListChanged(true);
TenantConfig[] tenants = MultitenancyHelper.getInstance(main).getAllTenants();
List<TenantConfig> tenantList = new ArrayList<>();

Expand All @@ -413,7 +413,7 @@ public static TenantConfig[] getAllAppsAndTenantsForConnectionUriDomain(String c
}

public static TenantConfig[] getAllTenants(Main main) {
MultitenancyHelper.getInstance(main).refreshTenantsInCoreIfRequired(true);
MultitenancyHelper.getInstance(main).refreshTenantsInCoreBasedOnChangesInCoreConfigOrIfTenantListChanged(true);
return MultitenancyHelper.getInstance(main).getAllTenants();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@

package io.supertokens.multitenancy;

import com.google.gson.Gson;
import com.google.gson.JsonObject;
import io.supertokens.Main;
import io.supertokens.ProcessState;
import io.supertokens.ResourceDistributor;
import io.supertokens.config.Config;
import io.supertokens.cronjobs.Cronjobs;
Expand Down Expand Up @@ -85,7 +85,7 @@ private TenantConfig[] getAllTenantsFromDb() throws StorageQueryException {
return StorageLayer.getMultitenancyStorage(main).getAllTenants();
}

public List<TenantIdentifier> refreshTenantsInCoreIfRequired(boolean reloadAllResources) {
public List<TenantIdentifier> refreshTenantsInCoreBasedOnChangesInCoreConfigOrIfTenantListChanged(boolean reloadAllResources) {
try {
return main.getResourceDistributor().withResourceDistributorLock(() -> {
try {
Expand Down Expand Up @@ -115,6 +115,8 @@ public List<TenantIdentifier> refreshTenantsInCoreIfRequired(boolean reloadAllRe
return tenantsThatChanged;
}

rishabhpoddar marked this conversation as resolved.
Show resolved Hide resolved
ProcessState.getInstance(main).addState(ProcessState.PROCESS_STATE.TENANTS_CHANGED_DURING_REFRESH_FROM_DB, null);

// this order is important. For example, storageLayer depends on config, and cronjobs depends on
// storageLayer
if (reloadAllResources) {
Expand All @@ -123,7 +125,7 @@ public List<TenantIdentifier> refreshTenantsInCoreIfRequired(boolean reloadAllRe
// we do these two here cause they don't really depend on any table in the db, and these
// two are required for allocating any further resource for this tenant
loadConfig(tenantsThatChanged);
loadStorageLayer(tenantsThatChanged);
loadStorageLayer();
}
return tenantsThatChanged;
} catch (Exception e) {
Expand All @@ -141,7 +143,7 @@ public void forceReloadAllResources(List<TenantIdentifier> tenantsThatChanged) {
main.getResourceDistributor().withResourceDistributorLock(() -> {
try {
loadConfig(tenantsThatChanged);
loadStorageLayer(tenantsThatChanged);
loadStorageLayer();
loadFeatureFlag(tenantsThatChanged);
loadSigningKeys(tenantsThatChanged);
refreshCronjobs();
Expand All @@ -159,8 +161,8 @@ public void loadConfig(List<TenantIdentifier> tenantsThatChanged) throws IOExcep
Config.loadAllTenantConfig(main, this.tenantConfigs, tenantsThatChanged);
}

public void loadStorageLayer(List<TenantIdentifier> tenantsThatChanged) throws IOException, InvalidConfigException {
StorageLayer.loadAllTenantStorage(main, this.tenantConfigs, tenantsThatChanged);
public void loadStorageLayer() throws IOException, InvalidConfigException {
StorageLayer.loadAllTenantStorage(main, this.tenantConfigs);
}

public void loadFeatureFlag(List<TenantIdentifier> tenantsThatChanged) {
Expand Down
12 changes: 5 additions & 7 deletions src/main/java/io/supertokens/storageLayer/StorageLayer.java
Original file line number Diff line number Diff line change
Expand Up @@ -185,14 +185,12 @@ public static void initPrimary(Main main, String pluginFolderPath, JsonObject co
new StorageLayer(main, pluginFolderPath, configJson, TenantIdentifier.BASE_TENANT));
}

@TestOnly
public static void loadAllTenantStorage(Main main, TenantConfig[] tenants)
throws InvalidConfigException, IOException {
loadAllTenantStorage(main, tenants, new ArrayList<>());
}
// We decided not to include tenantsThatChanged in this function because we do not want to reload the storage
// when the db config has not change. And when db config has changed, it results in a
// different userPoolId + connectionPoolId, which in turn results in a new storage instance

public static void loadAllTenantStorage(Main main, TenantConfig[] tenants, List<TenantIdentifier> tenantsThatChanged)
throws InvalidConfigException, IOException {
ProcessState.getInstance(main).addState(ProcessState.PROCESS_STATE.LOADING_ALL_TENANT_STORAGE, null);

Map<ResourceDistributor.KeyClass, JsonObject> normalisedConfigs = Config.getNormalisedConfigsForAllTenants(
Expand All @@ -208,7 +206,7 @@ public static void loadAllTenantStorage(Main main, TenantConfig[] tenants, List<
String userPoolId = storage.getUserPoolId();
rishabhpoddar marked this conversation as resolved.
Show resolved Hide resolved
String connectionPoolId = storage.getConnectionPoolId();
String uniqueId = userPoolId + "~" + connectionPoolId;
if (idToStorageMap.get(uniqueId) != null && !tenantsThatChanged.contains(key.getTenantIdentifier())) {
if (idToStorageMap.get(uniqueId) != null) {
// this means there already exists a storage object that can be reused
// for this tenant
resourceKeyToStorageMap.put(key, idToStorageMap.get(uniqueId));
Expand Down Expand Up @@ -244,7 +242,7 @@ public static void loadAllTenantStorage(Main main, TenantConfig[] tenants, List<
String userPoolId = currStorage.getUserPoolId();
String connectionPoolId = currStorage.getConnectionPoolId();
String uniqueId = userPoolId + "~" + connectionPoolId;
rishabhpoddar marked this conversation as resolved.
Show resolved Hide resolved
if (idToExistingStorageLayerMap.containsKey(uniqueId) && !tenantsThatChanged.contains(key.getTenantIdentifier())) {
if (idToExistingStorageLayerMap.containsKey(uniqueId)) {
// we reuse the existing storage layer
resourceKeyToStorageMap.put(key, idToExistingStorageLayerMap.get(uniqueId).storage);
}
Expand Down
21 changes: 19 additions & 2 deletions src/test/java/io/supertokens/test/multitenant/ConfigTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -1253,6 +1253,8 @@ public void testThatConfigChangesReloadsConfig() throws Exception {
{
Config configBefore = Config.getInstance(t1, process.getProcess());

ProcessState.getInstance(process.getProcess()).clear();

JsonObject coreConfig = new JsonObject();
Multitenancy.addNewOrUpdateAppOrTenant(process.getProcess(), new TenantConfig(
t1,
Expand All @@ -1262,6 +1264,8 @@ public void testThatConfigChangesReloadsConfig() throws Exception {
coreConfig
), false);

assertNull(process.checkOrWaitForEvent(ProcessState.PROCESS_STATE.TENANTS_CHANGED_DURING_REFRESH_FROM_DB));

Config configAfter = Config.getInstance(t1, process.getProcess());

assertEquals(configBefore, configAfter);
Expand Down Expand Up @@ -1320,6 +1324,8 @@ public void testThatConfigChangesInAppReloadsConfigInTenant() throws Exception {
}

{
ProcessState.getInstance(process.getProcess()).clear();

Config configBefore = Config.getInstance(t1, process.getProcess());

JsonObject coreConfig = new JsonObject();
Expand All @@ -1335,7 +1341,9 @@ public void testThatConfigChangesInAppReloadsConfigInTenant() throws Exception {
new ThirdPartyConfig(true, null),
new PasswordlessConfig(true),
coreConfig
), false);
), false);

assertNull(process.checkOrWaitForEvent(ProcessState.PROCESS_STATE.TENANTS_CHANGED_DURING_REFRESH_FROM_DB));

Config configAfter = Config.getInstance(t1, process.getProcess());

Expand Down Expand Up @@ -1387,6 +1395,7 @@ public void testThatConfigChangesReloadsStorageLayer() throws Exception {
}

{
ProcessState.getInstance(process.getProcess()).clear();
Storage storageLayerBefore = StorageLayer.getStorage(t1, process.getProcess());

JsonObject coreConfig = new JsonObject();
Expand All @@ -1398,6 +1407,7 @@ public void testThatConfigChangesReloadsStorageLayer() throws Exception {
coreConfig
), false);

assertNull(process.checkOrWaitForEvent(ProcessState.PROCESS_STATE.TENANTS_CHANGED_DURING_REFRESH_FROM_DB));
Storage storageLayerAfter = StorageLayer.getStorage(t1, process.getProcess());

assertEquals(storageLayerBefore, storageLayerAfter);
Expand All @@ -1418,7 +1428,7 @@ public void testThatConfigChangesReloadsStorageLayer() throws Exception {

Storage storageLayerAfter = StorageLayer.getStorage(t1, process.getProcess());

assertNotEquals(storageLayerBefore, storageLayerAfter);
assertEquals(storageLayerBefore, storageLayerAfter);
}

{
Expand Down Expand Up @@ -1468,6 +1478,8 @@ public void testThatConfigChangesReloadsFeatureFlag() throws Exception {
}

{
ProcessState.getInstance(process.getProcess()).clear();

FeatureFlag featureFlagBefore = FeatureFlag.getInstance(process.getProcess(), t1);

JsonObject coreConfig = new JsonObject();
Expand All @@ -1479,6 +1491,7 @@ public void testThatConfigChangesReloadsFeatureFlag() throws Exception {
coreConfig
), false);

assertNull(process.checkOrWaitForEvent(ProcessState.PROCESS_STATE.TENANTS_CHANGED_DURING_REFRESH_FROM_DB));
FeatureFlag featureFlagAfter = FeatureFlag.getInstance(process.getProcess(), t1);

assertEquals(featureFlagBefore, featureFlagAfter);
Expand Down Expand Up @@ -1529,6 +1542,8 @@ public void testThatConfigChangesReloadsSigningKeys() throws Exception {
}

{
ProcessState.getInstance(process.getProcess()).clear();

AccessTokenSigningKey accessTokenSigningKeyBefore = AccessTokenSigningKey.getInstance(t1, process.getProcess());
RefreshTokenKey refreshTokenKeyBefore = RefreshTokenKey.getInstance(t1, process.getProcess());
JWTSigningKey jwtSigningKeyBefore = JWTSigningKey.getInstance(t1, process.getProcess());
Expand All @@ -1543,6 +1558,8 @@ public void testThatConfigChangesReloadsSigningKeys() throws Exception {
coreConfig
), false);

assertNull(process.checkOrWaitForEvent(ProcessState.PROCESS_STATE.TENANTS_CHANGED_DURING_REFRESH_FROM_DB));

AccessTokenSigningKey accessTokenSigningKeyAfter = AccessTokenSigningKey.getInstance(t1, process.getProcess());
RefreshTokenKey refreshTokenKeyAfter = RefreshTokenKey.getInstance(t1, process.getProcess());
JWTSigningKey jwtSigningKeyAfter = JWTSigningKey.getInstance(t1, process.getProcess());
Expand Down