Fix concurrent group access to prevent NullPointerException#1
Conversation
Closes #40368 Signed-off-by: vramik <vramik@redhat.com>
|
@coderabbitai review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis pull request refactors group caching logic by introducing lazy supplier evaluation with null-safety in GroupAdapter, adds an explicit Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/base/src/test/java/org/keycloak/tests/admin/group/GroupTest.java (1)
144-147: Remove unnecessary blank line in exception handler.The empty line 145 between the catch block and adding the exception serves no purpose.
} catch (Exception e) { - caughtExceptions.add(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 (2)
model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/entities/CachedGroup.java (1)
61-61: LGTM! Good use of @OverRide annotation.The explicit
@Overrideannotation improves code clarity and enables compile-time verification that this method correctly overrides a method from theInRealminterface.model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/GroupAdapter.java (1)
274-275: Excellent concurrent access protection!The null-safe handling prevents
NullPointerExceptionwhen the group is deleted or invalidated during concurrent access. Returningnullappropriately signals that the count is unavailable rather than throwing an exception.
| @Test | ||
| public void createMultiDeleteMultiReadMulti() { | ||
| // create multiple groups | ||
| List<String> groupUuuids = new ArrayList<>(); | ||
| IntStream.range(0, 100).forEach(groupIndex -> { | ||
| GroupRepresentation group = new GroupRepresentation(); | ||
| group.setName("Test Group " + groupIndex); | ||
| try (Response response = managedRealm.admin().groups().add(group)) { | ||
| boolean created = response.getStatusInfo().getFamily() == Response.Status.Family.SUCCESSFUL; | ||
| if (created) { | ||
| final String groupUuid = ApiUtil.getCreatedId(response); | ||
| groupUuuids.add(groupUuid); | ||
| } else { | ||
| fail("Failed to create group: " + response.getStatusInfo().getReasonPhrase()); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| AtomicBoolean deletedAll = new AtomicBoolean(false); | ||
| List<Exception> caughtExceptions = new CopyOnWriteArrayList<>(); | ||
| // read groups in a separate thread | ||
| new Thread(() -> { | ||
| while (!deletedAll.get()) { | ||
| try { | ||
| // just loading briefs | ||
| managedRealm.admin().groups().groups(null, 0, Integer.MAX_VALUE, true); | ||
| } catch (Exception e) { | ||
|
|
||
| caughtExceptions.add(e); | ||
| } | ||
| } | ||
| }).start(); | ||
|
|
||
| // delete groups | ||
| groupUuuids.forEach(groupUuid -> { | ||
| managedRealm.admin().groups().group(groupUuid).remove(); | ||
| }); | ||
| deletedAll.set(true); | ||
|
|
||
| assertThat(caughtExceptions, Matchers.empty()); | ||
| } |
There was a problem hiding this comment.
Add thread synchronization to ensure reliable test execution.
The test has several concurrency concerns:
-
Missing thread join: The background thread may still be executing when the assertion runs. Add
Thread.join()after settingdeletedAllto ensure the thread completes. -
Race condition: If deletions complete very quickly, the background thread might not execute any iterations before
deletedAllbecomestrue, making the test ineffective at verifying concurrent access. -
No verification of actual concurrent access: There's no guarantee the thread actually performed any reads during the deletion window.
Apply this refactor to fix the synchronization issues:
AtomicBoolean deletedAll = new AtomicBoolean(false);
List<Exception> caughtExceptions = new CopyOnWriteArrayList<>();
+ AtomicBoolean threadStarted = new AtomicBoolean(false);
// read groups in a separate thread
- new Thread(() -> {
+ Thread readerThread = new Thread(() -> {
+ threadStarted.set(true);
while (!deletedAll.get()) {
try {
// just loading briefs
managedRealm.admin().groups().groups(null, 0, Integer.MAX_VALUE, true);
} catch (Exception e) {
-
caughtExceptions.add(e);
}
}
- }).start();
+ });
+ readerThread.start();
+
+ // Wait for reader thread to start
+ while (!threadStarted.get()) {
+ Thread.sleep(10);
+ }
// delete groups
groupUuuids.forEach(groupUuid -> {
managedRealm.admin().groups().group(groupUuid).remove();
});
deletedAll.set(true);
+
+ // Wait for reader thread to complete
+ readerThread.join();
assertThat(caughtExceptions, Matchers.empty());Note: This requires adding throws InterruptedException to the test method signature or wrapping in a try-catch block.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/base/src/test/java/org/keycloak/tests/admin/group/GroupTest.java around
lines 118 to 158, the background reader thread is not synchronized with the main
thread causing unreliable test behavior; modify the test to create and start the
reader thread, use a synchronization primitive (CountDownLatch or Phaser) to
ensure the reader thread has started and performed at least one read before
deletions begin, after setting deletedAll = true call thread.join() (or await on
the latch) to guarantee the reader exits before assertions run, and either add
throws InterruptedException to the test signature or catch InterruptedException
around join/await; also track a simple counter/flag updated by the reader to
assert that at least one read occurred during deletions.
2 participants
Test 10
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/keycloak-coderabbit#10