Skip to content

Fix concurrent group access to prevent NullPointerException#1

Open
zaibkhan wants to merge 1 commit into
feature-group-concurrency-updatefrom
feature-group-concurrency-implementation
Open

Fix concurrent group access to prevent NullPointerException#1
zaibkhan wants to merge 1 commit into
feature-group-concurrency-updatefrom
feature-group-concurrency-implementation

Conversation

@zaibkhan

Copy link
Copy Markdown

No description provided.

Closes #40368

Signed-off-by: vramik <vramik@redhat.com>
@codoki-pr-intelligence

codoki-pr-intelligence Bot commented Sep 10, 2025

Copy link
Copy Markdown

Codoki PR Review

Summary: Guard subgroup access under concurrency, prevent NPEs
What’s good: Fix in GroupAdapter.getSubGroupsCount() defensively handles concurrent deletion by null-checking modelSupplier.get(), reducing NPE risk.
Review Status: ❌ Requires changes
Overall Priority: High

Issues (Critical & High only)

Severity Issue Why it matters
High Correctness — Potential NPE in getSubGroupsStream() under concurrent de... …/infinispan/GroupAdapter.java
Concurrent deletion can make modelSupplier.get() return null, leading to a NullPointerException here. Align the defensive pattern with getSubGroupsCount() and return an empty stream when the model is gone. Apply the same guard to the other getSubGroupsStream(...) overloads to fully harden subgroup access under concurrent modifications.

Showing top 1 issues. Critical: 0, High: 1. See inline suggestions for more.

Key Feedback (click to expand)
  • Needs improvement: Similar null-guarding is still missing from getSubGroupsStream(...) overloads, which can still throw in the same concurrent deletion scenario.
  • Testing: The new test spawns a reader thread without tracking or joining it, which can leak across tests and cause flakiness; also the tight while-loop busy-spins. Capture the thread and join after deletion, and add a small backoff (Thread.onSpinWait or sleep) inside the loop.
  • Documentation: Document that GroupModel.getSubGroupsCount() may return null under concurrent modifications so API consumers handle this safely.
  • Compatibility: Removal of groupMatchesSearchOrIsPathElement appears private-scoped; no external API break expected.
  • Performance: The test's reader thread tight loop can consume significant CPU; add a small backoff.
  • Open questions: Should subGroupCount being null be treated as 'unknown' or coerced to 0 in API responses? If 'unknown', confirm that serialization and consumers tolerate null.

Confidence: 3/5 — Needs work before merge (1 high · status: Requires changes)

React with 👍 or 👎 if you found this review useful.

List<Exception> caughtExceptions = new CopyOnWriteArrayList<>();
// read groups in a separate thread
new Thread(() -> {
while (!deletedAll.get()) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔷 Medium: The spawned reader thread is not tracked or joined, which can leak past test completion and cause flakiness; the tight while-loop also busy-spins. Capture a Thread reference, join it after setting deletedAll to true, and add a small backoff to reduce CPU usage.

Suggested change
while (!deletedAll.get()) {
managedRealm.admin().groups().groups(null, 0, Integer.MAX_VALUE, true); Thread.onSpinWait();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants