From ba2f6da91547f54e2811edb8eb57448df80d1a14 Mon Sep 17 00:00:00 2001 From: Dinika Date: Mon, 18 Jan 2021 14:25:23 +0530 Subject: [PATCH 1/3] Synchronize user role retrieval from DB and adding it to cache to avoid multiple threads trying to get user roles from DB for the same user concurrently. --- .../core/common/AbstractUserStoreManager.java | 136 ++++++++++-------- 1 file changed, 79 insertions(+), 57 deletions(-) diff --git a/core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/AbstractUserStoreManager.java b/core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/AbstractUserStoreManager.java index cf926a70a75..dd5f411e7e1 100644 --- a/core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/AbstractUserStoreManager.java +++ b/core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/AbstractUserStoreManager.java @@ -8418,47 +8418,61 @@ public List doGetRoleListOfUserWithID(String userID, String filter) thro private List getUserRolesWithID(String userID, String filter) throws UserStoreException { - List internalRoles = doGetInternalRoleListOfUserWithID(userID, filter); - Set modifiedInternalRoles = new HashSet<>(); - String[] modifiedExternalRoleList = new String[0]; - - if (readGroupsEnabled && doCheckExistingUserWithID(userID)) { - List roles = new ArrayList<>(); - String[] externalRoles = doGetExternalRoleListOfUserWithID(userID, "*"); - roles.addAll(Arrays.asList(externalRoles)); - if (isSharedGroupEnabled()) { - String[] sharedRoles = doGetSharedRoleListOfUserWithID(userID, null, "*"); - if (sharedRoles != null) { - roles.addAll(Arrays.asList(sharedRoles)); + List userRoleList; + String username = getUserNameFromUserID(userID); + synchronized (userID.intern()) { + if (username != null) { + String[] roleListOfUserFromCache = getRoleListOfUserFromCache(this.tenantId, username); + if (roleListOfUserFromCache != null) { + List roleList = Arrays.asList(roleListOfUserFromCache); + if (!roleList.isEmpty()) { + return roleList; + } } } - modifiedExternalRoleList = UserCoreUtil.addDomainToNames(roles.toArray(new String[0]), getMyDomainName()); - // Get the associated internal roles of the groups. - if (isRoleAndGroupSeparationEnabled()) { - Set rolesOfGroups = getUniqueSet(getHybridRoleListOfGroups(roles, getMyDomainName())); - modifiedInternalRoles.addAll(rolesOfGroups); + List internalRoles = doGetInternalRoleListOfUserWithID(userID, filter); + Set modifiedInternalRoles = new HashSet<>(); + String[] modifiedExternalRoleList = new String[0]; + + if (readGroupsEnabled && doCheckExistingUserWithID(userID)) { + List roles = new ArrayList<>(); + String[] externalRoles = doGetExternalRoleListOfUserWithID(userID, "*"); + roles.addAll(Arrays.asList(externalRoles)); + if (isSharedGroupEnabled()) { + String[] sharedRoles = doGetSharedRoleListOfUserWithID(userID, null, "*"); + if (sharedRoles != null) { + roles.addAll(Arrays.asList(sharedRoles)); + } + } + modifiedExternalRoleList = UserCoreUtil.addDomainToNames(roles.toArray(new String[0]), getMyDomainName()); + + // Get the associated internal roles of the groups. + if (isRoleAndGroupSeparationEnabled()) { + Set rolesOfGroups = getUniqueSet(getHybridRoleListOfGroups(roles, getMyDomainName())); + modifiedInternalRoles.addAll(rolesOfGroups); + } } - } - modifiedInternalRoles.addAll(internalRoles); - String[] roleList = UserCoreUtil.combine(modifiedExternalRoleList, new ArrayList<>(modifiedInternalRoles)); + modifiedInternalRoles.addAll(internalRoles); + String[] roleList = UserCoreUtil.combine(modifiedExternalRoleList, new ArrayList<>(modifiedInternalRoles)); + userRoleList = Arrays.asList(roleList); - for (UserOperationEventListener userOperationEventListener : UMListenerServiceComponent - .getUserOperationEventListeners()) { - if (userOperationEventListener instanceof AbstractUserOperationEventListener) { - if (!((AbstractUserOperationEventListener) userOperationEventListener) - .doPostGetRoleListOfUserWithID(userID, filter, roleList, this)) { - break; + for (UserOperationEventListener userOperationEventListener : UMListenerServiceComponent + .getUserOperationEventListeners()) { + if (userOperationEventListener instanceof AbstractUserOperationEventListener) { + if (!((AbstractUserOperationEventListener) userOperationEventListener) + .doPostGetRoleListOfUserWithID(userID, filter, roleList, this)) { + break; + } } } - } - // Add to user role cache uisng username. - String username = getUserNameFromUserID(userID); - if (username != null) { - addToUserRolesCache(this.tenantId, username, roleList); + // Add to user role cache using username. + if (username != null) { + addToUserRolesCache(this.tenantId, username, roleList); + } } - return Arrays.asList(roleList); + return userRoleList; } /** @@ -8487,40 +8501,48 @@ public final String[] doGetRoleListOfUser(String userName, String filter) throws private String[] getUserRoles(String username, String filter) throws UserStoreException { - String[] internalRoles = doGetInternalRoleListOfUser(username, filter); - String[] modifiedExternalRoleList = new String[0]; + String[] roleList; + String usernameWithTenantDomain = username + "@" + this.getTenantDomain(this.tenantId); + synchronized (usernameWithTenantDomain.intern()) { + roleList = getRoleListOfUserFromCache(this.tenantId, username); + if (roleList != null && roleList.length > 0) { + return roleList; + } + String[] internalRoles = doGetInternalRoleListOfUser(username, filter); + String[] modifiedExternalRoleList = new String[0]; - if (readGroupsEnabled && doCheckExistingUser(username)) { - String[] externalRoles = doGetExternalRoleListOfUser(username, "*"); - List roles = Arrays.asList(externalRoles); - if (isSharedGroupEnabled()) { - String[] sharedRoles = doGetSharedRoleListOfUser(username, null, "*"); - if (sharedRoles != null) { - roles.addAll(Arrays.asList(sharedRoles)); + if (readGroupsEnabled && doCheckExistingUser(username)) { + String[] externalRoles = doGetExternalRoleListOfUser(username, "*"); + List roles = Arrays.asList(externalRoles); + if (isSharedGroupEnabled()) { + String[] sharedRoles = doGetSharedRoleListOfUser(username, null, "*"); + if (sharedRoles != null) { + roles.addAll(Arrays.asList(sharedRoles)); + } } - } - modifiedExternalRoleList = UserCoreUtil - .addDomainToNames(roles.toArray(new String[0]), getMyDomainName()); + modifiedExternalRoleList = UserCoreUtil + .addDomainToNames(roles.toArray(new String[0]), getMyDomainName()); - // Get the associated internal roles of the groups. - if (isRoleAndGroupSeparationEnabled()) { - Set rolesOfGroups = getUniqueSet(getHybridRoleListOfGroups(roles, getMyDomainName())); - internalRoles = UserCoreUtil.combine(internalRoles, new ArrayList<>(rolesOfGroups)); + // Get the associated internal roles of the groups. + if (isRoleAndGroupSeparationEnabled()) { + Set rolesOfGroups = getUniqueSet(getHybridRoleListOfGroups(roles, getMyDomainName())); + internalRoles = UserCoreUtil.combine(internalRoles, new ArrayList<>(rolesOfGroups)); + } } - } - String[] roleList = UserCoreUtil.combine(internalRoles, Arrays.asList(modifiedExternalRoleList)); + roleList = UserCoreUtil.combine(internalRoles, Arrays.asList(modifiedExternalRoleList)); - for (UserOperationEventListener userOperationEventListener : UMListenerServiceComponent - .getUserOperationEventListeners()) { - if (userOperationEventListener instanceof AbstractUserOperationEventListener) { - if (!((AbstractUserOperationEventListener) userOperationEventListener) - .doPostGetRoleListOfUser(username, filter, roleList, this)) { - break; + for (UserOperationEventListener userOperationEventListener : UMListenerServiceComponent + .getUserOperationEventListeners()) { + if (userOperationEventListener instanceof AbstractUserOperationEventListener) { + if (!((AbstractUserOperationEventListener) userOperationEventListener) + .doPostGetRoleListOfUser(username, filter, roleList, this)) { + break; + } } } + addToUserRolesCache(this.tenantId, username, roleList); } - addToUserRolesCache(this.tenantId, username, roleList); return roleList; } From 43e0269a5d6f578083a6d8d505a7fa88f51f5eef Mon Sep 17 00:00:00 2001 From: Dinika Date: Mon, 18 Jan 2021 19:26:06 +0530 Subject: [PATCH 2/3] Reformat synchronized methods --- .../core/common/AbstractUserStoreManager.java | 154 +++++++++--------- 1 file changed, 76 insertions(+), 78 deletions(-) diff --git a/core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/AbstractUserStoreManager.java b/core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/AbstractUserStoreManager.java index dd5f411e7e1..ebd2e17d57a 100644 --- a/core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/AbstractUserStoreManager.java +++ b/core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/AbstractUserStoreManager.java @@ -8402,77 +8402,75 @@ public List doGetRoleListOfUserWithID(String userID, String filter) thro return (List) object; } + List roleList; String username = getUserNameFromUserID(userID); if (username != null) { String[] roleListOfUserFromCache = getRoleListOfUserFromCache(this.tenantId, username); if (roleListOfUserFromCache != null) { - List roleList = Arrays.asList(roleListOfUserFromCache); + roleList = Arrays.asList(roleListOfUserFromCache); if (!roleList.isEmpty()) { return roleList; } } } - return getUserRolesWithID(userID, filter); - } - - private List getUserRolesWithID(String userID, String filter) throws UserStoreException { - - List userRoleList; - String username = getUserNameFromUserID(userID); synchronized (userID.intern()) { if (username != null) { String[] roleListOfUserFromCache = getRoleListOfUserFromCache(this.tenantId, username); if (roleListOfUserFromCache != null) { - List roleList = Arrays.asList(roleListOfUserFromCache); + roleList = Arrays.asList(roleListOfUserFromCache); if (!roleList.isEmpty()) { return roleList; } } } + String[] roleListOfUserFromDatabase = getUserRolesWithIDFromDatabase(userID, filter); + roleList = Arrays.asList(roleListOfUserFromDatabase); + // Add to user role cache using username. + if (username != null) { + addToUserRolesCache(this.tenantId, username, roleListOfUserFromDatabase); + } + } + return roleList; + } - List internalRoles = doGetInternalRoleListOfUserWithID(userID, filter); - Set modifiedInternalRoles = new HashSet<>(); - String[] modifiedExternalRoleList = new String[0]; + private String[] getUserRolesWithIDFromDatabase(String userID, String filter) throws UserStoreException { - if (readGroupsEnabled && doCheckExistingUserWithID(userID)) { - List roles = new ArrayList<>(); - String[] externalRoles = doGetExternalRoleListOfUserWithID(userID, "*"); - roles.addAll(Arrays.asList(externalRoles)); - if (isSharedGroupEnabled()) { - String[] sharedRoles = doGetSharedRoleListOfUserWithID(userID, null, "*"); - if (sharedRoles != null) { - roles.addAll(Arrays.asList(sharedRoles)); - } - } - modifiedExternalRoleList = UserCoreUtil.addDomainToNames(roles.toArray(new String[0]), getMyDomainName()); + List internalRoles = doGetInternalRoleListOfUserWithID(userID, filter); + Set modifiedInternalRoles = new HashSet<>(); + String[] modifiedExternalRoleList = new String[0]; - // Get the associated internal roles of the groups. - if (isRoleAndGroupSeparationEnabled()) { - Set rolesOfGroups = getUniqueSet(getHybridRoleListOfGroups(roles, getMyDomainName())); - modifiedInternalRoles.addAll(rolesOfGroups); + if (readGroupsEnabled && doCheckExistingUserWithID(userID)) { + List roles = new ArrayList<>(); + String[] externalRoles = doGetExternalRoleListOfUserWithID(userID, "*"); + roles.addAll(Arrays.asList(externalRoles)); + if (isSharedGroupEnabled()) { + String[] sharedRoles = doGetSharedRoleListOfUserWithID(userID, null, "*"); + if (sharedRoles != null) { + roles.addAll(Arrays.asList(sharedRoles)); } } - modifiedInternalRoles.addAll(internalRoles); - String[] roleList = UserCoreUtil.combine(modifiedExternalRoleList, new ArrayList<>(modifiedInternalRoles)); - userRoleList = Arrays.asList(roleList); + modifiedExternalRoleList = UserCoreUtil.addDomainToNames(roles.toArray(new String[0]), getMyDomainName()); - for (UserOperationEventListener userOperationEventListener : UMListenerServiceComponent - .getUserOperationEventListeners()) { - if (userOperationEventListener instanceof AbstractUserOperationEventListener) { - if (!((AbstractUserOperationEventListener) userOperationEventListener) - .doPostGetRoleListOfUserWithID(userID, filter, roleList, this)) { - break; - } - } + // Get the associated internal roles of the groups. + if (isRoleAndGroupSeparationEnabled()) { + Set rolesOfGroups = getUniqueSet(getHybridRoleListOfGroups(roles, getMyDomainName())); + modifiedInternalRoles.addAll(rolesOfGroups); } + } + modifiedInternalRoles.addAll(internalRoles); + String[] roleList = UserCoreUtil.combine(modifiedExternalRoleList, new ArrayList<>(modifiedInternalRoles)); - // Add to user role cache using username. - if (username != null) { - addToUserRolesCache(this.tenantId, username, roleList); + for (UserOperationEventListener userOperationEventListener : UMListenerServiceComponent + .getUserOperationEventListeners()) { + if (userOperationEventListener instanceof AbstractUserOperationEventListener) { + if (!((AbstractUserOperationEventListener) userOperationEventListener) + .doPostGetRoleListOfUserWithID(userID, filter, roleList, this)) { + break; + } } } - return userRoleList; + return roleList; } /** @@ -8496,52 +8494,52 @@ public final String[] doGetRoleListOfUser(String userName, String filter) throws return roleList; } - return getUserRoles(userName, filter); - } - - private String[] getUserRoles(String username, String filter) throws UserStoreException { - - String[] roleList; - String usernameWithTenantDomain = username + "@" + this.getTenantDomain(this.tenantId); + String usernameWithTenantDomain = userName + "@" + this.getTenantDomain(this.tenantId); synchronized (usernameWithTenantDomain.intern()) { - roleList = getRoleListOfUserFromCache(this.tenantId, username); + roleList = getRoleListOfUserFromCache(this.tenantId, userName); if (roleList != null && roleList.length > 0) { return roleList; } - String[] internalRoles = doGetInternalRoleListOfUser(username, filter); - String[] modifiedExternalRoleList = new String[0]; + roleList = getUserRolesFromDatabase(userName, filter); + addToUserRolesCache(this.tenantId, userName, roleList); + } + return roleList; + } - if (readGroupsEnabled && doCheckExistingUser(username)) { - String[] externalRoles = doGetExternalRoleListOfUser(username, "*"); - List roles = Arrays.asList(externalRoles); - if (isSharedGroupEnabled()) { - String[] sharedRoles = doGetSharedRoleListOfUser(username, null, "*"); - if (sharedRoles != null) { - roles.addAll(Arrays.asList(sharedRoles)); - } - } - modifiedExternalRoleList = UserCoreUtil - .addDomainToNames(roles.toArray(new String[0]), getMyDomainName()); + private String[] getUserRolesFromDatabase(String username, String filter) throws UserStoreException { + + String[] internalRoles = doGetInternalRoleListOfUser(username, filter); + String[] modifiedExternalRoleList = new String[0]; - // Get the associated internal roles of the groups. - if (isRoleAndGroupSeparationEnabled()) { - Set rolesOfGroups = getUniqueSet(getHybridRoleListOfGroups(roles, getMyDomainName())); - internalRoles = UserCoreUtil.combine(internalRoles, new ArrayList<>(rolesOfGroups)); + if (readGroupsEnabled && doCheckExistingUser(username)) { + String[] externalRoles = doGetExternalRoleListOfUser(username, "*"); + List roles = Arrays.asList(externalRoles); + if (isSharedGroupEnabled()) { + String[] sharedRoles = doGetSharedRoleListOfUser(username, null, "*"); + if (sharedRoles != null) { + roles.addAll(Arrays.asList(sharedRoles)); } } + modifiedExternalRoleList = UserCoreUtil + .addDomainToNames(roles.toArray(new String[0]), getMyDomainName()); - roleList = UserCoreUtil.combine(internalRoles, Arrays.asList(modifiedExternalRoleList)); + // Get the associated internal roles of the groups. + if (isRoleAndGroupSeparationEnabled()) { + Set rolesOfGroups = getUniqueSet(getHybridRoleListOfGroups(roles, getMyDomainName())); + internalRoles = UserCoreUtil.combine(internalRoles, new ArrayList<>(rolesOfGroups)); + } + } - for (UserOperationEventListener userOperationEventListener : UMListenerServiceComponent - .getUserOperationEventListeners()) { - if (userOperationEventListener instanceof AbstractUserOperationEventListener) { - if (!((AbstractUserOperationEventListener) userOperationEventListener) - .doPostGetRoleListOfUser(username, filter, roleList, this)) { - break; - } + String[] roleList = UserCoreUtil.combine(internalRoles, Arrays.asList(modifiedExternalRoleList)); + + for (UserOperationEventListener userOperationEventListener : UMListenerServiceComponent + .getUserOperationEventListeners()) { + if (userOperationEventListener instanceof AbstractUserOperationEventListener) { + if (!((AbstractUserOperationEventListener) userOperationEventListener) + .doPostGetRoleListOfUser(username, filter, roleList, this)) { + break; } } - addToUserRolesCache(this.tenantId, username, roleList); } return roleList; } @@ -8569,9 +8567,9 @@ public final String[] getRoleListOfUserFromDatabase(String username, String filt // According to implementation, getRoleListOfUser method would return everyone role name for all users. return new String[]{realmConfig.getEveryOneRoleName()}; } - return getUserRolesWithID(userID, filter).toArray(new String[0]); + return getUserRolesWithIDFromDatabase(userID, filter); } else { - return getUserRoles(username, filter); + return getUserRolesFromDatabase(username, filter); } } From 579eb5e9fd29aae28cc84e89193fa58cbc2cf9c4 Mon Sep 17 00:00:00 2001 From: Dinika Date: Mon, 18 Jan 2021 20:31:30 +0530 Subject: [PATCH 3/3] Add debug logs --- .../carbon/user/core/common/AbstractUserStoreManager.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/AbstractUserStoreManager.java b/core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/AbstractUserStoreManager.java index ebd2e17d57a..de4e3a361b7 100644 --- a/core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/AbstractUserStoreManager.java +++ b/core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/AbstractUserStoreManager.java @@ -8436,6 +8436,9 @@ public List doGetRoleListOfUserWithID(String userID, String filter) thro private String[] getUserRolesWithIDFromDatabase(String userID, String filter) throws UserStoreException { + if (log.isDebugEnabled()) { + log.debug("Retrieving user role list for userID: " + userID + " from database"); + } List internalRoles = doGetInternalRoleListOfUserWithID(userID, filter); Set modifiedInternalRoles = new HashSet<>(); String[] modifiedExternalRoleList = new String[0]; @@ -8508,6 +8511,9 @@ public final String[] doGetRoleListOfUser(String userName, String filter) throws private String[] getUserRolesFromDatabase(String username, String filter) throws UserStoreException { + if (log.isDebugEnabled()) { + log.debug("Retrieving user role list for user: " + username + " from database"); + } String[] internalRoles = doGetInternalRoleListOfUser(username, filter); String[] modifiedExternalRoleList = new String[0];