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.

Sign up for GitHub

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

Synchronize user role retrieval from DB and adding it to user role cache #2899

Draft
wants to merge 3 commits into
base: 4.6.x-old
Choose a base branch
from
Draft

Synchronize user role retrieval from DB and adding it to user role cache #2899

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ba2f6da
Synchronize user role retrieval from DB and adding it to cache to avo…
DinikaSen Jan 18, 2021
43e0269
Reformat synchronized methods
DinikaSen Jan 18, 2021
579eb5e
Add debug logs
DinikaSen Jan 18, 2021
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
56 changes: 41 additions & 15 deletions ...on.user.core/src/main/java/org/wso2/carbon/user/core/common/AbstractUserStoreManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8402,22 +8402,43 @@ public List<String> doGetRoleListOfUserWithID(String userID, String filter) thro
return (List<String>) object;
}

List<String> roleList;
String username = getUserNameFromUserID(userID);
if (username != null) {
String[] roleListOfUserFromCache = getRoleListOfUserFromCache(this.tenantId, username);
if (roleListOfUserFromCache != null) {
List<String> roleList = Arrays.asList(roleListOfUserFromCache);
roleList = Arrays.asList(roleListOfUserFromCache);
if (!roleList.isEmpty()) {
return roleList;
}
}
}

return getUserRolesWithID(userID, filter);
synchronized (userID.intern()) {
if (username != null) {
String[] roleListOfUserFromCache = getRoleListOfUserFromCache(this.tenantId, username);
if (roleListOfUserFromCache != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to start synchronized block only if if (roleListOfUserFromCache == null) { right?

Copy link
Author

Choose a reason for hiding this comment

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

Here the if (roleListOfUserFromCache == null) check is done inside the synchronized block for double-checked locking purpose. There is another if (roleListOfUserFromCache == null) check in the calling method right before this method is called.

Copy link

Choose a reason for hiding this comment

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

@DinikaSen There is a pattern to follow when using synchronise block. Always advisable to follow the pattern so that code and logic is simpler and less error prone

if (! checkCondition() ) {
syncronized (lock) {
if (checkCondition() ) {exit;}

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;
}

private List<String> getUserRolesWithID(String userID, String filter) throws UserStoreException {
private String[] getUserRolesWithIDFromDatabase(String userID, String filter) throws UserStoreException {

if (log.isDebugEnabled()) {
log.debug("Retrieving user role list for userID: " + userID + " from database");
}
List<String> internalRoles = doGetInternalRoleListOfUserWithID(userID, filter);
Set<String> modifiedInternalRoles = new HashSet<>();
String[] modifiedExternalRoleList = new String[0];
Expand Down Expand Up @@ -8452,13 +8473,7 @@ private List<String> getUserRolesWithID(String userID, String filter) throws Use
}
}
}

// Add to user role cache uisng username.
String username = getUserNameFromUserID(userID);
if (username != null) {
addToUserRolesCache(this.tenantId, username, roleList);
}
return Arrays.asList(roleList);
return roleList;
}

/**
Expand All @@ -8482,11 +8497,23 @@ public final String[] doGetRoleListOfUser(String userName, String filter) throws
return roleList;
}

return getUserRoles(userName, filter);
String usernameWithTenantDomain = userName + "@" + this.getTenantDomain(this.tenantId);
synchronized (usernameWithTenantDomain.intern()) {
roleList = getRoleListOfUserFromCache(this.tenantId, userName);
Copy link

Choose a reason for hiding this comment

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

roleList = getRoleListOfUserFromCache(this.tenantId, userName); has to happen outside the synchronise block. Otherwise the synchronise block is called for each request and thus may cause unnecessary locks.

if (roleList != null && roleList.length > 0) {
return roleList;
}
roleList = getUserRolesFromDatabase(userName, filter);
addToUserRolesCache(this.tenantId, userName, roleList);
}
return roleList;
}

private String[] getUserRoles(String username, String filter) throws UserStoreException {
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];

Expand Down Expand Up @@ -8520,7 +8547,6 @@ private String[] getUserRoles(String username, String filter) throws UserStoreEx
}
}
}
addToUserRolesCache(this.tenantId, username, roleList);
return roleList;
}

Expand All @@ -8547,9 +8573,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);
}
}

Expand Down