Details
-
Bug
-
Resolution: Unresolved
-
Major
-
None
-
17.10.15
-
None
-
Unknown
-
Description
The "all" (recursive) entry of MemberGroupsCache can be cached with a stale value when a group is modified while that entry is being computed, and it then stays wrong until the entry is invalidated for another reason or evicted.
An entry is only invalidated through the references it's indexed under: its own reference when it's created, and the references of its value when setDirect/setAll is called. For the "all" entry, the groups two levels up or more are only indexed by the final setAll, after they have been read from the database.
Scenario, with M ∈ G1 ∈ G2 ∈ G3 and a thread resolving the recursive groups of M (getGroups(M, wikis, true), or GroupManager#isMember(M, X, true) when M is not in X):
- The thread creates the entry E of M (indexed under M), sets its direct groups {G1} (indexed under G1) and reads the direct groups of G2: {G3}.
- An admin removes G2 from G3. GroupCacheInvalidationListener cleans G3 and G2, which removes the entries of G2 and G1. E is not indexed under G2 or G3 yet, so it stays in the cache.
- The thread calls E.setAll(G1, G2, G3): M is now considered a member of G3 until E is invalidated or evicted.
The opposite case (a group added to another group during the resolution) caches a stale "not a member".
A related problem in AbstractGroupCache: cacheDocumentIndex holds plain HashSet instances that addToIndex (called from setDirect/setAll) and cleanIndex (called from eviction) modify without the cache lock, so they can be modified concurrently.
Possible fix: an invalidation counter in AbstractGroupCache, incremented by cleanCache/removeAll under the write lock, read before the first database read, and checked under the read lock before setDirect/setAll, the value not being cached when it changed. For the index, use concurrent sets (ConcurrentHashMap.newKeySet()).
Found during the review of https://github.com/xwiki/xwiki-platform/pull/7431 (see https://github.com/xwiki/xwiki-platform/pull/7431#discussion_r4565355131).
Attachments
Issue Links
- relates to
-
XWIKI-25732 Add GroupManager#isMember(member, group, recurse)
-
- Closed
-