OAK-12341: Thread Contention authentication.external.impl.principal.SyncConfigTracker - #3072
OAK-12341: Thread Contention authentication.external.impl.principal.SyncConfigTracker#3072jsedding wants to merge 3 commits into
Conversation
…SyncConfigTracker - speed up unit test
…SyncConfigTracker - fix contention on isEnabled - add ServiceTracker generics
Commit-Check ❌ |
|
|
Wouldnt the issue still be present in this scenario, we really dont want to call getServiceReferences, in this scenario we still call it and thats what leads to the blocking operation. In the case of a large burst of requests this lookup can cause hundreds of threads to become a FIFO like queue. In my PR what I propose is what we let the OSGi framework push the services to us and we track them avoiding the lookup completely. |
|
@pat-lego I am calling BTW, I included your test case and my change passes it. I like how you're checking for contention! 👍 |
Yes sorry I should be more specific, is there ever a reason we should also fix |
rishabhdaim
changed the title
| */ | ||
| boolean isEnabled() { | ||
| return getReferences().length > 0; | ||
| return getServiceReference() != null; |
There was a problem hiding this 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?
| + "the shared Tracked monitor, as observed in the OAK-12341 thread dump", blockedMs > 0); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
This javadoc doesn't match what the fix actually does — there's no CopyOnWriteArrayList and no addingService/removedService override anywhere in SyncConfigTracker. The real mechanism is the inherited ServiceTracker.getServiceReference() (singular), which caches the resolved reference internally and only falls back to the synchronized getServiceReferences() scan on a cache miss — like @jsedding described above. Worth fixing so a future reader doesn't go looking for a cache field that isn't there.
|
|
||
| long[] threads = threadIds.stream().mapToLong(Long::longValue).toArray(); | ||
| long blockedMs = 0; | ||
| for (int i = 0; i < 1000; i++) { |
There was a problem hiding this comment.
This assertion (blockedMs > 0) depends on a single spinner thread resubmitting one-shot tasks through a bounded queue, rather than all THREAD_COUNT threads hammering concurrently — real overlap only happens once the queue fills, which isn't guaranteed on a fast or CPU-constrained CI box. Might be worth having each worker loop directly instead of resubmitting through pool.execute(), and polling against a wall-clock timeout instead of a fixed 1000 iterations, so this doesn't flake.
There was a problem hiding this comment.
Polling against a wall clock was in place before. One assertion checks that the threads are not blocked, so it always waits for the wall clock duration. If we can set the wall clock to 50ms or less, that's fine.
|
|
rishabhdaim
changed the title
|
Closing in favour of #3069. Apparently, the contention does not only affect the |
3 participants
I propose this fix as an alternative to PR #3069. I took the unit-test from the original PR, made it fast, and implemented a fix that relies on an internally cached field of
ServiceTracker.cc @pat-lego