diff --git a/docs/changelog/133681.yaml b/docs/changelog/133681.yaml new file mode 100644 index 0000000000000..6231eec2a5235 --- /dev/null +++ b/docs/changelog/133681.yaml @@ -0,0 +1,6 @@ +pr: 133681 +summary: Remove `DocumentSubsetBitsetCache` locking +area: Authorization +type: bug +issues: + - 132842 diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/DocumentSubsetBitsetCache.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/DocumentSubsetBitsetCache.java index 1177ff68c34c4..5c969b3545f03 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/DocumentSubsetBitsetCache.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/DocumentSubsetBitsetCache.java @@ -31,26 +31,21 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.unit.ByteSizeValue; import org.elasticsearch.common.util.concurrent.ConcurrentCollections; -import org.elasticsearch.common.util.concurrent.ReleasableLock; import org.elasticsearch.core.Nullable; import org.elasticsearch.core.TimeValue; import org.elasticsearch.lucene.util.BitSets; import org.elasticsearch.lucene.util.MatchAllBitSet; -import org.elasticsearch.threadpool.ThreadPool; import java.io.Closeable; import java.io.IOException; import java.util.List; import java.util.Map; import java.util.Objects; -import java.util.Optional; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ExecutionException; -import java.util.concurrent.ExecutorService; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicLong; -import java.util.concurrent.locks.ReentrantReadWriteLock; /** * This is a cache for {@link BitSet} instances that are used with the {@link DocumentSubsetReader}. @@ -81,6 +76,8 @@ */ public final class DocumentSubsetBitsetCache implements IndexReader.ClosedListener, Closeable, Accountable { + private static final Logger logger = LogManager.getLogger(DocumentSubsetBitsetCache.class); + /** * The TTL defaults to 2 hours. We default to a large cache size ({@link #CACHE_SIZE_SETTING}), and aggressively * expire unused entries so that the cache does not hold on to memory unnecessarily. @@ -102,40 +99,15 @@ public final class DocumentSubsetBitsetCache implements IndexReader.ClosedListen private static final BitSet NULL_MARKER = new FixedBitSet(0); - private static final Logger logger = LogManager.getLogger(DocumentSubsetBitsetCache.class); - - /** - * When a {@link BitSet} is evicted from {@link #bitsetCache}, we need to also remove it from {@link #keysByIndex}. - * We use a {@link ReentrantReadWriteLock} to control atomicity here - the "read" side represents potential insertions to the - * {@link #bitsetCache}, the "write" side represents removals from {@link #keysByIndex}. - * The risk (that {@link Cache} does not provide protection for) is that an entry is removed from the cache, and then immediately - * re-populated, before we process the removal event. To protect against that we need to check the state of the {@link #bitsetCache} - * but we need exclusive ("write") access while performing that check and updating the values in {@link #keysByIndex}. - */ - private final ReleasableLock cacheEvictionLock; - private final ReleasableLock cacheModificationLock; - private final ExecutorService cleanupExecutor; - private final long maxWeightBytes; private final Cache bitsetCache; private final Map> keysByIndex; private final AtomicLong cacheFullWarningTime; - public DocumentSubsetBitsetCache(Settings settings, ThreadPool threadPool) { - this(settings, threadPool.executor(ThreadPool.Names.GENERIC)); - } - /** * @param settings The global settings object for this node - * @param cleanupExecutor An executor on which the cache cleanup tasks can be run. Due to the way the cache is structured internally, - * it is sometimes necessary to run an asynchronous task to synchronize the internal state. */ - protected DocumentSubsetBitsetCache(Settings settings, ExecutorService cleanupExecutor) { - final ReentrantReadWriteLock readWriteLock = new ReentrantReadWriteLock(); - this.cacheEvictionLock = new ReleasableLock(readWriteLock.writeLock()); - this.cacheModificationLock = new ReleasableLock(readWriteLock.readLock()); - this.cleanupExecutor = cleanupExecutor; - + public DocumentSubsetBitsetCache(Settings settings) { final TimeValue ttl = CACHE_TTL_SETTING.get(settings); this.maxWeightBytes = CACHE_SIZE_SETTING.get(settings).getBytes(); this.bitsetCache = CacheBuilder.builder() @@ -150,8 +122,8 @@ protected DocumentSubsetBitsetCache(Settings settings, ExecutorService cleanupEx } @Override - public void onClose(IndexReader.CacheKey ownerCoreCacheKey) { - final Set keys = keysByIndex.remove(ownerCoreCacheKey); + public void onClose(IndexReader.CacheKey indexKey) { + final Set keys = keysByIndex.remove(indexKey); if (keys != null) { // Because this Set has been removed from the map, and the only update to the set is performed in a // Map#compute call, it should not be possible to get a concurrent modification here. @@ -163,24 +135,17 @@ public void onClose(IndexReader.CacheKey ownerCoreCacheKey) { * Cleanup (synchronize) the internal state when an object is removed from the primary cache */ private void onCacheEviction(RemovalNotification notification) { - final BitsetCacheKey bitsetKey = notification.getKey(); - final IndexReader.CacheKey indexKey = bitsetKey.index; - if (keysByIndex.getOrDefault(indexKey, Set.of()).contains(bitsetKey) == false) { - // If the bitsetKey isn't in the lookup map, then there's nothing to synchronize - return; - } - // We push this to a background thread, so that it reduces the risk of blocking searches, but also so that the lock management is - // simpler - this callback is likely to take place on a thread that is actively adding something to the cache, and is therefore - // holding the read ("update") side of the lock. It is not possible to upgrade a read lock to a write ("eviction") lock, but we - // need to acquire that lock here. - cleanupExecutor.submit(() -> { - try (ReleasableLock ignored = cacheEvictionLock.acquire()) { - // it's possible for the key to be back in the cache if it was immediately repopulated after it was evicted, so check - if (bitsetCache.get(bitsetKey) == null) { - // key is no longer in the cache, make sure it is no longer in the lookup map either. - Optional.ofNullable(keysByIndex.get(indexKey)).ifPresent(set -> set.remove(bitsetKey)); - } - } + final BitsetCacheKey cacheKey = notification.getKey(); + final IndexReader.CacheKey indexKey = cacheKey.indexKey; + // the key is *probably* no longer in the cache, so make sure it is no longer in the lookup map. + // note: rather than locking (which destroys our throughput), we're erring on the side of tidying the keysByIndex + // structure even if some other racing thread has already added a new bitset into the cache for this same key. + // the keysByIndex structure is used in onClose (our notification from lucene that a segment has become inaccessible), + // so we might end up failing to *eagerly* invalidate a bitset -- the consequence of that would be temporarily higher + // memory use (the bitset will not be accessed, and it will still be invalidated eventually for size or ttl reasons). + keysByIndex.computeIfPresent(indexKey, (ignored, keys) -> { + keys.remove(cacheKey); + return keys.isEmpty() ? null : keys; }); } @@ -231,41 +196,39 @@ public BitSet getBitSet(final Query query, final LeafReaderContext context) thro final IndexReader.CacheKey indexKey = coreCacheHelper.getKey(); final BitsetCacheKey cacheKey = new BitsetCacheKey(indexKey, query); - try (ReleasableLock ignored = cacheModificationLock.acquire()) { - final BitSet bitSet = bitsetCache.computeIfAbsent(cacheKey, ignore1 -> { - // This ensures all insertions into the set are guarded by ConcurrentHashMap's atomicity guarantees. - keysByIndex.compute(indexKey, (ignore2, set) -> { - if (set == null) { - set = ConcurrentCollections.newConcurrentSet(); - } - set.add(cacheKey); - return set; - }); - final BitSet result = computeBitSet(query, context); - if (result == null) { - // A cache loader is not allowed to return null, return a marker object instead. - return NULL_MARKER; - } - final long bitSetBytes = result.ramBytesUsed(); - if (bitSetBytes > this.maxWeightBytes) { - logger.warn( - "built a DLS BitSet that uses [{}] bytes; the DLS BitSet cache has a maximum size of [{}] bytes;" - + " this object cannot be cached and will need to be rebuilt for each use;" - + " consider increasing the value of [{}]", - bitSetBytes, - maxWeightBytes, - CACHE_SIZE_SETTING.getKey() - ); - } else if (bitSetBytes + bitsetCache.weight() > maxWeightBytes) { - maybeLogCacheFullWarning(); + final BitSet bitSet = bitsetCache.computeIfAbsent(cacheKey, ignore1 -> { + // This ensures all insertions into the set are guarded by ConcurrentHashMap's atomicity guarantees. + keysByIndex.compute(indexKey, (ignore2, keys) -> { + if (keys == null) { + keys = ConcurrentCollections.newConcurrentSet(); } - return result; + keys.add(cacheKey); + return keys; }); - if (bitSet == NULL_MARKER) { - return null; - } else { - return bitSet; + final BitSet result = computeBitSet(query, context); + if (result == null) { + // A cache loader is not allowed to return null, return a marker object instead. + return NULL_MARKER; } + final long bitSetBytes = result.ramBytesUsed(); + if (bitSetBytes > this.maxWeightBytes) { + logger.warn( + "built a DLS BitSet that uses [{}] bytes; the DLS BitSet cache has a maximum size of [{}] bytes;" + + " this object cannot be cached and will need to be rebuilt for each use;" + + " consider increasing the value of [{}]", + bitSetBytes, + maxWeightBytes, + CACHE_SIZE_SETTING.getKey() + ); + } else if (bitSetBytes + bitsetCache.weight() > maxWeightBytes) { + maybeLogCacheFullWarning(); + } + return result; + }); + if (bitSet == NULL_MARKER) { + return null; + } else { + return bitSet; } } @@ -323,11 +286,11 @@ public Map usageStats() { } private static class BitsetCacheKey { - final IndexReader.CacheKey index; + final IndexReader.CacheKey indexKey; final Query query; - private BitsetCacheKey(IndexReader.CacheKey index, Query query) { - this.index = index; + private BitsetCacheKey(IndexReader.CacheKey indexKey, Query query) { + this.indexKey = indexKey; this.query = query; } @@ -340,41 +303,59 @@ public boolean equals(Object other) { return false; } final BitsetCacheKey that = (BitsetCacheKey) other; - return Objects.equals(this.index, that.index) && Objects.equals(this.query, that.query); + return Objects.equals(this.indexKey, that.indexKey) && Objects.equals(this.query, that.query); } @Override public int hashCode() { - return Objects.hash(index, query); + return Objects.hash(indexKey, query); } @Override public String toString() { - return getClass().getSimpleName() + "(" + index + "," + query + ")"; + return getClass().getSimpleName() + "(" + indexKey + "," + query + ")"; } } /** - * This method verifies that the two internal data structures ({@link #bitsetCache} and {@link #keysByIndex}) are consistent with one - * another. This method is only called by tests. + * This test-only method verifies that the two internal data structures ({@link #bitsetCache} and {@link #keysByIndex}) are consistent + * with one another. */ + // visible for testing void verifyInternalConsistency() { - this.bitsetCache.keys().forEach(bck -> { - final Set set = this.keysByIndex.get(bck.index); - if (set == null) { - throw new IllegalStateException( - "Key [" + bck + "] is in the cache, but there is no entry for [" + bck.index + "] in the lookup map" - ); - } - if (set.contains(bck) == false) { + verifyInternalConsistencyCacheToKeys(); + verifyInternalConsistencyKeysToCache(); + } + + /** + * This test-only method iterates over the {@link #bitsetCache} and checks that {@link #keysByIndex} is consistent with it. + */ + // visible for testing + void verifyInternalConsistencyCacheToKeys() { + bitsetCache.keys().forEach(cacheKey -> { + final Set keys = keysByIndex.get(cacheKey.indexKey); + if (keys == null || keys.contains(cacheKey) == false) { throw new IllegalStateException( - "Key [" + bck + "] is in the cache, but the lookup entry for [" + bck.index + "] does not contain that key" + "Key [" + cacheKey + "] is in the cache, but the lookup entry for [" + cacheKey.indexKey + "] does not contain that key" ); } }); - this.keysByIndex.values().stream().flatMap(Set::stream).forEach(bck -> { - if (this.bitsetCache.get(bck) == null) { - throw new IllegalStateException("Key [" + bck + "] is in the lookup map, but is not in the cache"); + } + + /** + * This test-only method iterates over the {@link #keysByIndex} and checks that {@link #bitsetCache} is consistent with it. + */ + // visible for testing + void verifyInternalConsistencyKeysToCache() { + keysByIndex.forEach((indexKey, keys) -> { + if (keys == null || keys.isEmpty()) { + throw new IllegalStateException("The lookup entry for [" + indexKey + "] is null or empty"); + } else { + keys.forEach(cacheKey -> { + if (bitsetCache.get(cacheKey) == null) { + throw new IllegalStateException("Key [" + cacheKey + "] is in the lookup map, but is not in the cache"); + } + }); } }); } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/DocumentSubsetBitsetCacheTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/DocumentSubsetBitsetCacheTests.java index 5369c95ad6fa7..db78d8eb33ede 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/DocumentSubsetBitsetCacheTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/DocumentSubsetBitsetCacheTests.java @@ -43,10 +43,6 @@ import org.elasticsearch.test.ESTestCase; import org.elasticsearch.test.IndexSettingsModule; import org.elasticsearch.test.MockLog; -import org.hamcrest.Matchers; -import org.junit.After; -import org.junit.Before; -import org.mockito.Mockito; import java.io.Closeable; import java.io.IOException; @@ -65,12 +61,13 @@ import static java.util.Collections.emptyList; import static java.util.Collections.emptyMap; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.greaterThan; import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.lessThan; import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.notNullValue; import static org.hamcrest.Matchers.nullValue; import static org.hamcrest.Matchers.sameInstance; -import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -80,17 +77,6 @@ public class DocumentSubsetBitsetCacheTests extends ESTestCase { // This value is based on the internal implementation details of lucene's FixedBitSet // If the implementation changes, this can be safely updated to match the new ram usage for a single bitset private static final long EXPECTED_BYTES_PER_BIT_SET = 56; - private ExecutorService singleThreadExecutor; - - @Before - public void setUpExecutor() { - singleThreadExecutor = Executors.newSingleThreadExecutor(); - } - - @After - public void cleanUpExecutor() { - singleThreadExecutor.shutdown(); - } public void testSameBitSetIsReturnedForIdenticalQuery() throws Exception { final DocumentSubsetBitsetCache cache = newCache(Settings.EMPTY); @@ -103,7 +89,7 @@ public void testSameBitSetIsReturnedForIdenticalQuery() throws Exception { final BitSet bitSet2 = cache.getBitSet(query2, leafContext); assertThat(bitSet2, notNullValue()); - assertThat(bitSet2, Matchers.sameInstance(bitSet1)); + assertThat(bitSet2, sameInstance(bitSet1)); }); } @@ -272,7 +258,7 @@ public void testCacheRespectsAccessTimeExpiry() throws Exception { assertThat(bitSet2, notNullValue()); // Loop until the cache has less than 2 items, which mean that something we evicted - assertThat(cache.entryCount(), Matchers.lessThan(2)); + assertThat(cache.entryCount(), lessThan(2)); }, 100, TimeUnit.MILLISECONDS); @@ -288,42 +274,28 @@ public void testIndexLookupIsClearedWhenBitSetIsEvicted() throws Exception { .put(DocumentSubsetBitsetCache.CACHE_SIZE_SETTING.getKey(), maxCacheBytes + "b") .build(); - final ExecutorService executor = mock(ExecutorService.class); - final AtomicReference runnableRef = new AtomicReference<>(); - when(executor.submit(any(Runnable.class))).thenAnswer(inv -> { - final Runnable r = (Runnable) inv.getArguments()[0]; - runnableRef.set(r); - return null; - }); - - final DocumentSubsetBitsetCache cache = new DocumentSubsetBitsetCache(settings, executor); + final DocumentSubsetBitsetCache cache = new DocumentSubsetBitsetCache(settings); assertThat(cache.entryCount(), equalTo(0)); assertThat(cache.ramBytesUsed(), equalTo(0L)); runTestOnIndex((searchExecutionContext, leafContext) -> { + cache.verifyInternalConsistency(); + final Query query1 = QueryBuilders.termQuery("field-1", "value-1").toQuery(searchExecutionContext); final BitSet bitSet1 = cache.getBitSet(query1, leafContext); assertThat(bitSet1, notNullValue()); + cache.verifyInternalConsistency(); final Query query2 = QueryBuilders.termQuery("field-2", "value-2").toQuery(searchExecutionContext); final BitSet bitSet2 = cache.getBitSet(query2, leafContext); assertThat(bitSet2, notNullValue()); - - // BitSet1 has been evicted now, run the cleanup... - final Runnable runnable1 = runnableRef.get(); - assertThat(runnable1, notNullValue()); - runnable1.run(); cache.verifyInternalConsistency(); - // Check that the original bitset is no longer in the cache (a new instance is returned) assertThat(cache.getBitSet(query1, leafContext), not(sameInstance(bitSet1))); - - // BitSet2 has been evicted now, run the cleanup... - final Runnable runnable2 = runnableRef.get(); - assertThat(runnable2, not(sameInstance(runnable1))); - runnable2.run(); cache.verifyInternalConsistency(); }); + + cache.verifyInternalConsistency(); } public void testCacheUnderConcurrentAccess() throws Exception { @@ -337,23 +309,12 @@ public void testCacheUnderConcurrentAccess() throws Exception { .put(DocumentSubsetBitsetCache.CACHE_SIZE_SETTING.getKey(), maxCacheBytes + "b") .build(); + final DocumentSubsetBitsetCache cache = new DocumentSubsetBitsetCache(settings); + assertThat(cache.entryCount(), equalTo(0)); + assertThat(cache.ramBytesUsed(), equalTo(0L)); + final ExecutorService threads = Executors.newFixedThreadPool(concurrentThreads + 1); - final ExecutorService cleanupExecutor = Mockito.mock(ExecutorService.class); - when(cleanupExecutor.submit(any(Runnable.class))).thenAnswer(inv -> { - final Runnable runnable = (Runnable) inv.getArguments()[0]; - return threads.submit(() -> { - // Sleep for a small (random) length of time. - // This increases the likelihood that cache could have been modified between the eviction & the cleanup - Thread.sleep(randomIntBetween(1, 10)); - runnable.run(); - return null; - }); - }); try { - final DocumentSubsetBitsetCache cache = new DocumentSubsetBitsetCache(settings, cleanupExecutor); - assertThat(cache.entryCount(), equalTo(0)); - assertThat(cache.ramBytesUsed(), equalTo(0L)); - runTestOnIndices(numberOfIndices, contexts -> { final CountDownLatch start = new CountDownLatch(concurrentThreads); final CountDownLatch end = new CountDownLatch(concurrentThreads); @@ -394,12 +355,12 @@ public void testCacheUnderConcurrentAccess() throws Exception { threads.shutdown(); assertTrue("Cleanup thread did not complete in expected time", threads.awaitTermination(3, TimeUnit.SECONDS)); - cache.verifyInternalConsistency(); + cache.verifyInternalConsistencyKeysToCache(); // Due to cache evictions, we must get more bitsets than fields - assertThat(uniqueBitSets.size(), Matchers.greaterThan(FIELD_COUNT)); + assertThat(uniqueBitSets.size(), greaterThan(FIELD_COUNT)); // Due to cache evictions, we must have seen more bitsets than the cache currently holds - assertThat(uniqueBitSets.size(), Matchers.greaterThan(cache.entryCount())); + assertThat(uniqueBitSets.size(), greaterThan(cache.entryCount())); // Even under concurrent pressure, the cache should hit the expected size assertThat(cache.entryCount(), is(maxCacheCount)); assertThat(cache.ramBytesUsed(), is(maxCacheBytes)); @@ -407,62 +368,41 @@ public void testCacheUnderConcurrentAccess() throws Exception { } finally { threads.shutdown(); } + + cache.verifyInternalConsistencyKeysToCache(); } - public void testCleanupWorksWhenIndexIsClosing() throws Exception { + public void testCleanupWorksWhenIndexIsClosed() throws Exception { // Enough to hold slightly more than 1 bit-set in the cache final long maxCacheBytes = EXPECTED_BYTES_PER_BIT_SET + EXPECTED_BYTES_PER_BIT_SET / 2; final Settings settings = Settings.builder() .put(DocumentSubsetBitsetCache.CACHE_SIZE_SETTING.getKey(), maxCacheBytes + "b") .build(); - final ExecutorService threads = Executors.newFixedThreadPool(1); - final ExecutorService cleanupExecutor = Mockito.mock(ExecutorService.class); - final CountDownLatch cleanupReadyLatch = new CountDownLatch(1); - final CountDownLatch cleanupCompleteLatch = new CountDownLatch(1); - final CountDownLatch indexCloseLatch = new CountDownLatch(1); - final AtomicReference cleanupException = new AtomicReference<>(); - when(cleanupExecutor.submit(any(Runnable.class))).thenAnswer(inv -> { - final Runnable runnable = (Runnable) inv.getArguments()[0]; - return threads.submit(() -> { - try { - cleanupReadyLatch.countDown(); - assertTrue("index close did not completed in expected time", indexCloseLatch.await(1, TimeUnit.SECONDS)); - runnable.run(); - } catch (Throwable e) { - logger.warn("caught error in cleanup thread", e); - cleanupException.compareAndSet(null, e); - } finally { - cleanupCompleteLatch.countDown(); - } - return null; - }); - }); - final DocumentSubsetBitsetCache cache = new DocumentSubsetBitsetCache(settings, cleanupExecutor); + final DocumentSubsetBitsetCache cache = new DocumentSubsetBitsetCache(settings); assertThat(cache.entryCount(), equalTo(0)); assertThat(cache.ramBytesUsed(), equalTo(0L)); - try { - runTestOnIndex((searchExecutionContext, leafContext) -> { - final Query query1 = QueryBuilders.termQuery("field-1", "value-1").toQuery(searchExecutionContext); - final BitSet bitSet1 = cache.getBitSet(query1, leafContext); - assertThat(bitSet1, notNullValue()); + runTestOnIndex((searchExecutionContext, leafContext) -> { + final Query query1 = QueryBuilders.termQuery("field-1", "value-1").toQuery(searchExecutionContext); + final BitSet bitSet1 = cache.getBitSet(query1, leafContext); + assertThat(bitSet1, notNullValue()); + cache.verifyInternalConsistency(); - // Second query should trigger a cache eviction - final Query query2 = QueryBuilders.termQuery("field-2", "value-2").toQuery(searchExecutionContext); - final BitSet bitSet2 = cache.getBitSet(query2, leafContext); - assertThat(bitSet2, notNullValue()); + // Second query should trigger a cache eviction + final Query query2 = QueryBuilders.termQuery("field-2", "value-2").toQuery(searchExecutionContext); + final BitSet bitSet2 = cache.getBitSet(query2, leafContext); + assertThat(bitSet2, notNullValue()); + cache.verifyInternalConsistency(); - final IndexReader.CacheKey indexKey = leafContext.reader().getCoreCacheHelper().getKey(); - assertTrue("cleanup did not trigger in expected time", cleanupReadyLatch.await(1, TimeUnit.SECONDS)); - cache.onClose(indexKey); - indexCloseLatch.countDown(); - assertTrue("cleanup did not complete in expected time", cleanupCompleteLatch.await(1, TimeUnit.SECONDS)); - assertThat("caught error in cleanup thread: " + cleanupException.get(), cleanupException.get(), nullValue()); - }); - } finally { - threads.shutdown(); - } + final IndexReader.CacheKey indexKey = leafContext.reader().getCoreCacheHelper().getKey(); + cache.onClose(indexKey); + cache.verifyInternalConsistency(); + + // closing an index results in the associated entries being removed from the cache (at least when single threaded) + assertThat(cache.entryCount(), equalTo(0)); + assertThat(cache.ramBytesUsed(), equalTo(0L)); + }); } public void testCacheIsPerIndex() throws Exception { @@ -492,7 +432,7 @@ public void accept(SearchExecutionContext searchExecutionContext, LeafReaderCont runTestOnIndex(consumer); } - public void testCacheClearEntriesWhenIndexIsClosed() throws Exception { + public void testCacheClearsEntriesWhenIndexIsClosed() throws Exception { final DocumentSubsetBitsetCache cache = newCache(Settings.EMPTY); assertThat(cache.entryCount(), equalTo(0)); assertThat(cache.ramBytesUsed(), equalTo(0L)); @@ -504,9 +444,13 @@ public void testCacheClearEntriesWhenIndexIsClosed() throws Exception { final BitSet bitSet = cache.getBitSet(query, leafContext); assertThat(bitSet, notNullValue()); } + cache.verifyInternalConsistency(); assertThat(cache.entryCount(), not(equalTo(0))); assertThat(cache.ramBytesUsed(), not(equalTo(0L))); }); + cache.verifyInternalConsistency(); + + // closing an index results in the associated entries being removed from the cache (at least when single threaded) assertThat(cache.entryCount(), equalTo(0)); assertThat(cache.ramBytesUsed(), equalTo(0L)); } @@ -650,7 +594,7 @@ private void runTestOnIndices(int numberIndices, CheckedConsumer createComponents( components.add(privilegeStore); final ReservedRolesStore reservedRolesStore = new ReservedRolesStore(Set.copyOf(INCLUDED_RESERVED_ROLES_SETTING.get(settings))); - dlsBitsetCache.set(new DocumentSubsetBitsetCache(settings, threadPool)); + dlsBitsetCache.set(new DocumentSubsetBitsetCache(settings)); final FieldPermissionsCache fieldPermissionsCache = new FieldPermissionsCache(settings); RoleDescriptor.setFieldPermissionsCache(fieldPermissionsCache); diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/IndicesAndAliasesResolverTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/IndicesAndAliasesResolverTests.java index 6099b7351cf76..6d9926072b530 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/IndicesAndAliasesResolverTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/IndicesAndAliasesResolverTests.java @@ -58,7 +58,6 @@ import org.elasticsearch.protocol.xpack.graph.GraphExploreRequest; import org.elasticsearch.search.internal.ShardSearchRequest; import org.elasticsearch.test.ESTestCase; -import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.transport.EmptyRequest; import org.elasticsearch.transport.NoSuchRemoteClusterException; import org.elasticsearch.transport.TransportRequest; @@ -255,7 +254,7 @@ public void setup() { fieldPermissionsCache, mock(ApiKeyService.class), mock(ServiceAccountService.class), - new DocumentSubsetBitsetCache(Settings.EMPTY, mock(ThreadPool.class)), + new DocumentSubsetBitsetCache(Settings.EMPTY), RESTRICTED_INDICES, EsExecutors.DIRECT_EXECUTOR_SERVICE, rds -> {} diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStoreTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStoreTests.java index ed173d8e2b127..404468dfa9b86 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStoreTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStoreTests.java @@ -3195,7 +3195,7 @@ private RoleProviders buildRolesProvider( } private DocumentSubsetBitsetCache buildBitsetCache() { - return new DocumentSubsetBitsetCache(Settings.EMPTY, mock(ThreadPool.class)); + return new DocumentSubsetBitsetCache(Settings.EMPTY); } private static class InMemoryRolesProvider implements BiConsumer, ActionListener> {