Fix concurrent group access to prevent NullPointerException #1
Conversation
Closes #40368 Signed-off-by: vramik <[email protected]>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Greptile OverviewConfidence Score: 1/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant Reader as Reader Thread
participant Deleter as Deleter Thread
participant GA as GroupAdapter
participant MS as modelSupplier
participant Cache as CacheSession
participant DB as Database
Note over Reader,DB: Concurrent Group Access Scenario
Deleter->>Cache: groups().group(id).remove()
Cache->>DB: Delete group from database
DB-->>Cache: Group deleted
Reader->>GA: getSubGroupsCount()
GA->>GA: Check isUpdated() = false
GA->>MS: modelSupplier.get()
MS->>Cache: getGroupById(realm, id)
Cache->>DB: Query deleted group
DB-->>Cache: null (not found)
Cache-->>MS: null
MS-->>GA: null
alt Fixed: getSubGroupsCount()
GA-->>Reader: null or 0L (prevents NPE)
end
alt Unfixed: getSubGroupsStream() variants
GA->>GA: model.getSubGroupsStream(...)
Note right of GA: NPE thrown here!
GA--xReader: NullPointerException
end
|
There was a problem hiding this comment.
Additional Comments (3)
-
model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/GroupAdapter.java, line 256 (link)logic: NPE can still occur here during concurrent deletion - same issue as
getSubGroupsCount(). WhenmodelSupplier.get()returns null (group deleted), this will throw NPE. -
model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/GroupAdapter.java, line 262 (link)logic: NPE can still occur here during concurrent deletion - same issue as
getSubGroupsCount(). WhenmodelSupplier.get()returns null (group deleted), this will throw NPE. -
model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/GroupAdapter.java, line 268 (link)logic: NPE can still occur here during concurrent deletion - same issue as
getSubGroupsCount(). WhenmodelSupplier.get()returns null (group deleted), this will throw NPE.
4 files reviewed, 5 comments
| if (isUpdated()) return updated.getSubGroupsCount(); | ||
| return getGroupModel().getSubGroupsCount(); | ||
| GroupModel model = modelSupplier.get(); | ||
| return model == null ? null : model.getSubGroupsCount(); |
There was a problem hiding this comment.
logic: Violates interface contract - GroupModel.getSubGroupsCount() javadoc states "Never returns {@code null}". Should return 0L instead.
| return model == null ? null : model.getSubGroupsCount(); | |
| return model == null ? 0L : model.getSubGroupsCount(); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/GroupAdapter.java
Line: 275:275
Comment:
**logic:** Violates interface contract - `GroupModel.getSubGroupsCount()` javadoc states "Never returns {@code null}". Should return `0L` instead.
```suggestion
return model == null ? 0L : model.getSubGroupsCount();
```
How can I resolve this? If you propose a fix, please make it concise.| 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(); |
There was a problem hiding this comment.
logic: Race condition - thread may not start before groups are deleted. Add a small delay or synchronization to ensure the reader thread is actually running before deletion starts.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/base/src/test/java/org/keycloak/tests/admin/group/GroupTest.java
Line: 139:149
Comment:
**logic:** Race condition - thread may not start before groups are deleted. Add a small delay or synchronization to ensure the reader thread is actually running before deletion starts.
How can I resolve this? If you propose a fix, please make it concise.
Test 10
Replicated from ai-code-review-evaluation/keycloak-greptile#10