rishabhdaim commented on code in PR #3072:
URL: https://github.com/apache/jackrabbit-oak/pull/3072#discussion_r3765652158
##########
oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/SyncConfigTracker.java:
##########
@@ -56,7 +56,7 @@ final class SyncConfigTracker extends ServiceTracker {
* @return {@code true} if dynamic membership is enabled for at least one
registered sync-handler; {@code false} otherwise.
*/
boolean isEnabled() {
- return getReferences().length > 0;
+ return getServiceReference() != null;
Review Comment:
This only makes the `isEnabled()` check itself lock-free. Every caller that
actually gets past that check —
`AutomembershipService`/`DynamicGroupMembershipService` building an
`AutoMembershipProvider`/`ExternalGroupPrincipalProvider` — immediately calls
`getAutoMembership()`, `hasDynamicGroupsEnabled()`, `getGroupAutoMembership()`,
`getAutoMembershipConfig()`, `getIdpNamesWithDynamicGroups()`, all of which
still call `getServiceReferences()` and still synchronize on the same `Tracked`
monitor.
So for real requests (anywhere `isEnabled()` is true, which is the whole
point of this code existing) the contention from the reported thread dump
doesn't actually go away — it's only fixed for the isolated `isEnabled()`-only
case this PR's test exercises. This is the same thing @pat-lego asked above :
>should `getReferences()` get the same treatment?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
