Skip to content

Fix concurrent group access to prevent NullPointerException #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
akshayutture-augment wants to merge 1 commit into
base: feature-group-concurrency-update
Choose a base branch
from
Open

Fix concurrent group access to prevent NullPointerException #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/GroupAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,8 @@ public Stream<GroupModel> getSubGroupsStream(String search, Boolean exact, Integ
@Override
public Long getSubGroupsCount() {
if (isUpdated()) return updated.getSubGroupsCount();
return getGroupModel().getSubGroupsCount();
GroupModel model = modelSupplier.get();
return model == null ? null : model.getSubGroupsCount();
}

@Override
Expand Down
1 change: 1 addition & 0 deletions ...l/infinispan/src/main/java/org/keycloak/models/cache/infinispan/entities/CachedGroup.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ public CachedGroup(Long revision, RealmModel realm, GroupModel group) {
this.type = group.getType();
}

@Override
public String getRealm() {
return realm;
}
Expand Down
10 changes: 0 additions & 10 deletions services/src/main/java/org/keycloak/utils/GroupUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,14 +98,4 @@ public static GroupRepresentation toRepresentation(GroupPermissionEvaluator grou
rep.setAccess(groupsEvaluator.getAccess(groupTree));
return rep;
}

private static boolean groupMatchesSearchOrIsPathElement(GroupModel group, String search) {
if (StringUtil.isBlank(search)) {
return true;
}
if (group.getName().contains(search)) {
return true;
}
return group.getSubGroupsStream().findAny().isPresent();
}
}
48 changes: 48 additions & 0 deletions tests/base/src/test/java/org/keycloak/tests/admin/group/GroupTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.apache.http.client.methods.CloseableHttpResponse;
import org.apache.http.client.methods.HttpGet;
import org.apache.http.impl.client.CloseableHttpClient;
import org.hamcrest.Matchers;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
import org.keycloak.admin.client.Keycloak;
Expand Down Expand Up @@ -76,6 +77,9 @@
import java.util.List;
import java.util.Map;
import java.util.UUID;
import java.util.concurrent.CopyOnWriteArrayList;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.stream.IntStream;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.anEmptyMap;
Expand All @@ -90,6 +94,7 @@
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;

/**
* @author <a href="mailto:mstrukel@redhat.com">Marko Strukelj</a>
Expand All @@ -109,6 +114,49 @@ public class GroupTest extends AbstractGroupTest {
@InjectHttpClient
CloseableHttpClient httpClient;


@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());
}
Comment on lines +118 to +158

@coderabbitai coderabbitai Bot Nov 23, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Add thread synchronization to ensure reliable test execution.

The test has several concurrency concerns:

  1. Missing thread join: The background thread may still be executing when the assertion runs. Add Thread.join() after setting deletedAll to ensure the thread completes.

  2. Race condition: If deletions complete very quickly, the background thread might not execute any iterations before deletedAll becomes true, making the test ineffective at verifying concurrent access.

  3. 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.


// KEYCLOAK-2716 Can't delete client if its role is assigned to a group
@Test
public void testClientRemoveWithClientRoleGroupMapping() {
Expand Down