Fix concurrent group access to prevent NullPointerException#10
Open
ShashankFC wants to merge 1 commit into
Open
Fix concurrent group access to prevent NullPointerException#10ShashankFC wants to merge 1 commit into
ShashankFC wants to merge 1 commit into
Conversation
Closes #40368 Signed-off-by: vramik <vramik@redhat.com>
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis pull request refactors the Keycloak group caching layer by modifying null-safe retrieval in GroupAdapter, adding override annotation clarity in CachedGroup, removing an unused private utility method from GroupUtils, and introducing a new concurrent stress test for group operations. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~18 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 2
🤖 Fix all issues with AI agents
In
@model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/GroupAdapter.java:
- Around line 274-275: In GroupAdapter, make the getSubGroupsStream overloads
null-safe like getSubGroupsCount: call modelSupplier.get() into a local
GroupModel variable, check for null, and if null return an appropriate empty
result (e.g., Stream.empty()) instead of invoking methods on a null reference;
otherwise delegate to model.getSubGroupsStream(...). Ensure you update all three
overloads that currently call modelSupplier.get().getSubGroupsStream(...) so
they use this pattern.
In @tests/base/src/test/java/org/keycloak/tests/admin/group/GroupTest.java:
- Around line 136-158: The reader thread created for calling
managedRealm.admin().groups().groups(...) must be joined before asserting on
caughtExceptions to avoid the race; change the anonymous new Thread(...) to
assign it to a variable (e.g., readerThread), start it, and after
deletedAll.set(true) call readerThread.join() (optionally with a timeout) before
assertThat(caughtExceptions, Matchers.empty()); this ensures the reader loop has
finished and any exceptions have been recorded.
🧹 Nitpick comments (1)
tests/base/src/test/java/org/keycloak/tests/admin/group/GroupTest.java (1)
118-134: Test setup is appropriate for concurrent stress testing.The group creation loop with proper error handling and the use of thread-safe collections (
CopyOnWriteArrayList,AtomicBoolean) are good choices for this concurrency test. Consider adding cleanup for created groups in case assertions fail mid-test (e.g., usingmanagedRealm.cleanup().add(...)after each successful creation).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/GroupAdapter.javamodel/infinispan/src/main/java/org/keycloak/models/cache/infinispan/entities/CachedGroup.javaservices/src/main/java/org/keycloak/utils/GroupUtils.javatests/base/src/test/java/org/keycloak/tests/admin/group/GroupTest.java
💤 Files with no reviewable changes (1)
- services/src/main/java/org/keycloak/utils/GroupUtils.java
🧰 Additional context used
🧬 Code graph analysis (1)
tests/base/src/test/java/org/keycloak/tests/admin/group/GroupTest.java (2)
tests/utils/src/main/java/org/keycloak/tests/utils/matchers/Matchers.java (1)
Matchers(41-166)docs/documentation/tests/src/test/java/org/keycloak/documentation/test/utils/HttpUtils.java (1)
Response(108-146)
🔇 Additional comments (1)
model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/entities/CachedGroup.java (1)
61-64: LGTM!Adding the
@Overrideannotation improves code clarity and enables compile-time verification that this method correctly implements the interface contract.
Comment on lines
+274
to
+275
| GroupModel model = modelSupplier.get(); | ||
| return model == null ? null : model.getSubGroupsCount(); |
There was a problem hiding this comment.
Null-safety fix is correct, but other similar methods lack the same protection.
The null check here correctly prevents NPE when the group is deleted concurrently. However, the getSubGroupsStream overloads at lines 256, 262, and 268 call modelSupplier.get() directly without null checks:
return modelSupplier.get().getSubGroupsStream(search, firstResult, maxResults);These would still throw NPE during concurrent deletion. Consider applying the same null-safe pattern consistently.
🔧 Suggested fix for consistency
@Override
public Stream<GroupModel> getSubGroupsStream(String search, Integer firstResult, Integer maxResults) {
if (isUpdated()) return updated.getSubGroupsStream(search, firstResult, maxResults);
- return modelSupplier.get().getSubGroupsStream(search, firstResult, maxResults);
+ GroupModel model = modelSupplier.get();
+ return model == null ? Stream.empty() : model.getSubGroupsStream(search, firstResult, maxResults);
}
@Override
public Stream<GroupModel> getSubGroupsStream(Integer firstResult, Integer maxResults) {
if (isUpdated()) return updated.getSubGroupsStream(firstResult, maxResults);
- return modelSupplier.get().getSubGroupsStream(firstResult, maxResults);
+ GroupModel model = modelSupplier.get();
+ return model == null ? Stream.empty() : model.getSubGroupsStream(firstResult, maxResults);
}
@Override
public Stream<GroupModel> getSubGroupsStream(String search, Boolean exact, Integer firstResult, Integer maxResults) {
if (isUpdated()) return updated.getSubGroupsStream(search, exact, firstResult, maxResults);
- return modelSupplier.get().getSubGroupsStream(search, exact, firstResult, maxResults);
+ GroupModel model = modelSupplier.get();
+ return model == null ? Stream.empty() : model.getSubGroupsStream(search, exact, firstResult, maxResults);
}🤖 Prompt for AI Agents
In
@model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/GroupAdapter.java
around lines 274 - 275, In GroupAdapter, make the getSubGroupsStream overloads
null-safe like getSubGroupsCount: call modelSupplier.get() into a local
GroupModel variable, check for null, and if null return an appropriate empty
result (e.g., Stream.empty()) instead of invoking methods on a null reference;
otherwise delegate to model.getSubGroupsStream(...). Ensure you update all three
overloads that currently call modelSupplier.get().getSubGroupsStream(...) so
they use this pattern.
Comment on lines
+136
to
+158
| 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.
Race condition: reader thread is not joined before assertion.
The test sets deletedAll.set(true) and immediately asserts on caughtExceptions, but the reader thread may still be executing. This can cause flaky results or miss exceptions from the final loop iteration.
🔧 Proposed fix
+ Thread readerThread = new 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();
+ });
+ readerThread.start();
// delete groups
groupUuuids.forEach(groupUuid -> {
managedRealm.admin().groups().group(groupUuid).remove();
});
deletedAll.set(true);
+ try {
+ readerThread.join(5000); // Wait up to 5 seconds for reader to finish
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ fail("Reader thread interrupted");
+ }
+
assertThat(caughtExceptions, Matchers.empty());🤖 Prompt for AI Agents
In @tests/base/src/test/java/org/keycloak/tests/admin/group/GroupTest.java
around lines 136 - 158, The reader thread created for calling
managedRealm.admin().groups().groups(...) must be joined before asserting on
caughtExceptions to avoid the race; change the anonymous new Thread(...) to
assign it to a variable (e.g., readerThread), start it, and after
deletedAll.set(true) call readerThread.join() (optionally with a timeout) before
assertThat(caughtExceptions, Matchers.empty()); this ensures the reader loop has
finished and any exceptions have been recorded.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Test 10
Summary by CodeRabbit
Release Notes
Tests
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/keycloak-coderabbit#10