Fix concurrent group access to prevent NullPointerException#10
Fix concurrent group access to prevent NullPointerException#10everettbu wants to merge 1 commit into
Conversation
Closes #40368 Signed-off-by: vramik <vramik@redhat.com>
WalkthroughThis change set introduces a null check in the Changes
Sequence Diagram(s)sequenceDiagram
participant TestThread as Test Thread
participant MainThread as Main Thread
participant GroupAPI as Group API
MainThread->>GroupAPI: Create 100 groups
MainThread->>TestThread: Start concurrent reading thread
loop While not all groups deleted
TestThread->>GroupAPI: Read all groups (brief)
end
MainThread->>GroupAPI: Delete groups sequentially
MainThread->>TestThread: Signal reading thread to stop
TestThread->>MainThread: Report exceptions (if any)
MainThread->>MainThread: Assert no exceptions occurred
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/base/src/test/java/org/keycloak/tests/admin/group/GroupTest.java (1)
118-158: Excellent concurrent test for validating the NPE fix.This test effectively validates the concurrent group access improvements by:
- Creating multiple groups to establish test data
- Running concurrent read operations while deleting groups
- Using thread-safe collections (
CopyOnWriteArrayList,AtomicBoolean) for coordination- Asserting that no exceptions occur during concurrent operations
The test directly exercises the scenario described in the PR objective and validates that the null safety improvements in
GroupAdapter.getSubGroupsCount()prevent NPEs.Consider adding a small delay in the reader thread loop to better exercise different timing windows:
while (!deletedAll.get()) { try { + Thread.sleep(1); // Small delay to vary timing // just loading briefs managedRealm.admin().groups().groups(null, 0, Integer.MAX_VALUE, true); } catch (Exception e) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/GroupAdapter.java(1 hunks)model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/entities/CachedGroup.java(1 hunks)services/src/main/java/org/keycloak/utils/GroupUtils.java(0 hunks)tests/base/src/test/java/org/keycloak/tests/admin/group/GroupTest.java(4 hunks)
💤 Files with no reviewable changes (1)
- services/src/main/java/org/keycloak/utils/GroupUtils.java
🔇 Additional comments (5)
model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/entities/CachedGroup.java (1)
61-61: Good practice: Adding explicit @OverRide annotation.The addition of the
@Overrideannotation improves code clarity by explicitly marking this method as an interface implementation, and provides compile-time safety to ensure the method signature remains consistent with theInRealminterface.model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/GroupAdapter.java (1)
274-275: Excellent fix for the NullPointerException issue.This change directly addresses the PR objective by adding null safety to prevent NPE during concurrent group access. The defensive programming approach of checking
model == nullbefore callinggetSubGroupsCount()prevents crashes when the underlying group model is null due to concurrent operations or caching issues.tests/base/src/test/java/org/keycloak/tests/admin/group/GroupTest.java (3)
28-28: New import added for test assertions.The
Matchersimport is correctly added to support the new test's assertion on the empty exceptions list.
80-82: Appropriate imports for concurrent testing.The imports for
CopyOnWriteArrayList,AtomicBoolean, andIntStreamare well-chosen for implementing thread-safe concurrent testing functionality.
97-97: Good addition of fail import.The
failimport is correctly added to support the test's error handling when group creation fails.
2 participants
Test 10
Summary by CodeRabbit
Tests
Bug Fixes
Refactor