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:
**P1 · correctness/concurrency** — isEnabled() fast path does not eliminate
the reported contention on the real caller hot path
Under concurrent session/`PermissionProvider` resolution (the exact scenario
in the reported OAK-12341 thread dump), every caller that passes the
`isEnabled()` gate immediately calls other `SyncConfigTracker` methods
(`getAutoMembership`, `hasDynamicGroupsEnabled`, `getGroupAutoMembership`,
`getAutoMembershipConfig`, `getIdpNamesWithDynamicGroups`) that still call
`getServiceReferences()` directly or via the unchanged `getReferences()` helper
— each still synchronizing on the same shared `ServiceTracker$Tracked` monitor.
The cached fast path only benefits a caller that stops at the boolean check,
which isn't the reported hot path
(`AutomembershipService`/`DynamicGroupMembershipService` →
`AutoMembershipProvider`/`ExternalGroupPrincipalProvider` constructors), so
end-to-end contention is barely reduced even though the isolated
`isEnabled()`-only microbenchmark in the new test shows zero blocking. This
matches @pat-lego's open, unanswered question above about whether `getReferen
ces()` should also be fixed.
**Fix:** Either give `SyncConfigTracker` its own maintained snapshot of
tracked references (e.g. via `addingService`/`modifiedService`/`removedService`
overrides — matching what the new test's javadoc already claims but the code
doesn't implement) and have `getReferences()`,
`getIdpNamesWithDynamicGroups()`, `getAutoMembership()`,
`getGroupAutoMembership()`, `getAutoMembershipConfig()`, and
`hasDynamicGroupsEnabled()` all read from that snapshot instead of calling
`getServiceReferences()`; or explicitly scope this PR to just the `isEnabled()`
gate and file a tracked follow-up for the constructor paths.
<sub>Posted by an automated review pass.</sub>
--
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]
