Synchronize user role retrieval from DB and adding it to user role cache #2899
Conversation
…id multiple threads trying to get user roles from DB for the same user concurrently.
| synchronized (userID.intern()) { | ||
| if (username != null) { | ||
| String[] roleListOfUserFromCache = getRoleListOfUserFromCache(this.tenantId, username); | ||
| if (roleListOfUserFromCache != null) { |
There was a problem hiding this comment.
Better to start synchronized block only if if (roleListOfUserFromCache == null) { right?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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;}
DinikaSen
requested review from
ashensw,
gayashanbc,
JKAUSHALYA,
madurangasiriwardena and
tharindu-b-hewage
as code owners
|
PR builder started |
|
PR builder completed |
| return getUserRoles(userName, filter); | ||
| String usernameWithTenantDomain = userName + "@" + this.getTenantDomain(this.tenantId); | ||
| synchronized (usernameWithTenantDomain.intern()) { | ||
| roleList = getRoleListOfUserFromCache(this.tenantId, userName); |
There was a problem hiding this comment.
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.
| synchronized (userID.intern()) { | ||
| if (username != null) { | ||
| String[] roleListOfUserFromCache = getRoleListOfUserFromCache(this.tenantId, username); | ||
| if (roleListOfUserFromCache != null) { |
There was a problem hiding this comment.
@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;}
Purpose
This PR introduces thread synchronization to the process of 'user role retrieval from DB and adding it to user role cache' to avoid multiple threads querying the user roles from DB for the same user concurrently, and update the user role cache with the same value retrieved from DB concurrently.
Resolves : wso2/product-is#10869