Fix concurrent group access to prevent NullPointerException#1
Fix concurrent group access to prevent NullPointerException#1ShashankFC wants to merge 1 commit into
Conversation
Closes #40368 Signed-off-by: vramik <vramik@redhat.com>
|
@CodeAnt-AI: review |
|
CodeAnt AI is running the review. |
codeant-ai
Bot
added
the
size:M
Sequence DiagramThe PR fixes a null-safety bug in cached group access and adds a concurrent test that creates many groups, reads them repeatedly in a background thread while the main thread deletes them, and asserts no exceptions (NPEs) occur. The diagram shows the high-level concurrent create/read/delete flow exercised by the test and the cache access point that was hardened. sequenceDiagram
participant TestRunner
participant AdminAPI
participant GroupCache
participant BackendStore
TestRunner->>AdminAPI: Create many groups (loop)
AdminAPI->>BackendStore: Persist groups
TestRunner->>AdminAPI: Start background reader thread
loop while deletions pending
AdminAPI->>GroupCache: List groups (concurrent read)
GroupCache->>BackendStore: Fetch group data (modelSupplier.get())
BackendStore-->>GroupCache: Group data (may be null during concurrent delete)
GroupCache-->>AdminAPI: Return group list (null-safe)
end
TestRunner->>AdminAPI: Delete groups (main thread)
AdminAPI->>BackendStore: Remove groups
TestRunner-->>TestRunner: Assert no exceptions collected (no NPEs)
Generated by CodeAnt AI |
Nitpicks 🔍
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| 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); |
There was a problem hiding this comment.
Suggestion: The concurrency test starts a background thread to read groups but never joins it, so the test can complete (and assert that no exceptions occurred) before the reader thread actually runs or finishes, making the test flaky and potentially missing concurrent-access failures it is supposed to detect. [logic error]
Severity Level: Major ⚠️
- ⚠️ Concurrency regression test may miss group cache failures.
- ⚠️ CI may report green despite existing concurrent group bug.| 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); | |
| Thread reader = 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); | |
| } | |
| } | |
| }); | |
| reader.start(); | |
| // delete groups | |
| groupUuuids.forEach(groupUuid -> { | |
| managedRealm.admin().groups().group(groupUuid).remove(); | |
| }); | |
| deletedAll.set(true); | |
| try { | |
| reader.join(); | |
| } catch (InterruptedException e) { | |
| Thread.currentThread().interrupt(); | |
| fail("Reader thread was interrupted"); | |
| } |
Steps of Reproduction ✅
1. Run the JUnit test method `createMultiDeleteMultiReadMulti()` in
`tests/base/src/test/java/org/keycloak/tests/admin/group/GroupTest.java:119-158` (e.g.,
via `mvn test
-Dtest=org.keycloak.tests.admin.group.GroupTest#createMultiDeleteMultiReadMulti`).
2. The test creates and deletes many groups on the main thread at `GroupTest.java:151-154`
while starting a background reader thread at `GroupTest.java:139-149` that repeatedly
calls `managedRealm.admin().groups().groups(...)`.
3. The main thread sets `deletedAll` to `true` at `GroupTest.java:155` and immediately
asserts `caughtExceptions` is empty at `GroupTest.java:157` without joining the reader
thread, so the assertion can run before the reader thread has executed or finished its
loop.
4. If the reader thread encounters a concurrent-access bug and catches an exception later
in its loop at `GroupTest.java:140-147`, it adds it to `caughtExceptions` after the
assertion has already passed, causing the test to report success even though concurrent
group reads are failing.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/base/src/test/java/org/keycloak/tests/admin/group/GroupTest.java
**Line:** 139:155
**Comment:**
*Logic Error: The concurrency test starts a background thread to read groups but never joins it, so the test can complete (and assert that no exceptions occurred) before the reader thread actually runs or finishes, making the test flaky and potentially missing concurrent-access failures it is supposed to detect.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished running the review. |
2 participants
User description
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
CodeAnt-AI Description
Prevent NullPointerException when groups are accessed concurrently and add a concurrency test
What Changed
Impact
✅ Fewer NullPointerExceptions when groups are accessed concurrently✅ Safer group deletion while other requests read group lists✅ Earlier detection of concurrency regressions through a targeted integration test💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.