From 278ec13cc3ca549efef92e64b9faf9f64985fa8a Mon Sep 17 00:00:00 2001 From: Arkadiusz Palinski Date: Thu, 10 Oct 2024 11:33:00 +0200 Subject: [PATCH] RavenDB-22972 Fixing NREs on getting LowLevelTransaction.Id from ScanOldest() and LowLevelTransaction.TryGetClientState() called by LuceneIndexPersistence.Clean() after we started to null _envRecord on tx dispoe. The issue was that we could dispose a transaction while we iterated over active ones. RavenDB-22995 Fixing missing disposal of IndexSearcher after recreating it to cleanup Lucene readers. Also changing the cleanup implementation so we won't create a new index searcher instance per each active transaction (it's likely that all active transaction use the same LuceneIndexState instance anyway, hence also same IndexSearcher). --- .../Documents/Indexes/IndexStateRecord.cs | 45 ++++++++++++++++++- .../Lucene/LuceneIndexPersistence.cs | 4 +- src/Voron/Impl/LowLevelTransaction.cs | 19 +++++--- 3 files changed, 60 insertions(+), 8 deletions(-) diff --git a/src/Raven.Server/Documents/Indexes/IndexStateRecord.cs b/src/Raven.Server/Documents/Indexes/IndexStateRecord.cs index 4b42384e870c..b1bec73829a6 100644 --- a/src/Raven.Server/Documents/Indexes/IndexStateRecord.cs +++ b/src/Raven.Server/Documents/Indexes/IndexStateRecord.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Immutable; using System.Diagnostics; +using System.Runtime.CompilerServices; using Corax; using Lucene.Net.Search; using Raven.Client.Documents.Linq; @@ -47,6 +48,8 @@ public sealed record CollectionEtags( public class LuceneIndexState { + private static readonly ConditionalWeakTable InstancesThatGotRecreated = new(); + public Lazy IndexSearcher; public LuceneIndexState(Func func) @@ -80,7 +83,10 @@ public LuceneIndexState() logger.Error("Failed to finalize index searcher", e); } } - catch { } + catch + { + // ignored + } Debug.Assert(false, e.ToString()); } } @@ -89,6 +95,43 @@ public void Recreate(Func createIndexSearcher) { if (IndexSearcher.IsValueCreated == false) return; + + var old = IndexSearcher.Value; + + InstancesThatGotRecreated.Add(old, new IndexSearcherDisposer(old)); // this way we ensure the old searcher will get disposed - https://www.ayende.com/blog/199169-A/externalfinalizer-adding-a-finalizer-to-3rd-party-objects + IndexSearcher = new Lazy(createIndexSearcher); } + + private class IndexSearcherDisposer(IndexSearcher searcher) + { + ~IndexSearcherDisposer() + { + try + { + using (searcher) + using (searcher.IndexReader) + { + + } + } + catch (Exception e) + { + try + { + RavenLogger logger = RavenLogManager.Instance.GetLoggerForServer(); + if (logger.IsErrorEnabled) + { + logger.Error($"Failed to finalize index searcher from {nameof(IndexSearcherDisposer)}", e); + } + } + catch + { + // ignored + } + Debug.Assert(false, e.ToString()); + } + + } + } } diff --git a/src/Raven.Server/Documents/Indexes/Persistence/Lucene/LuceneIndexPersistence.cs b/src/Raven.Server/Documents/Indexes/Persistence/Lucene/LuceneIndexPersistence.cs index ae7c8f96aeae..7f5938d60137 100644 --- a/src/Raven.Server/Documents/Indexes/Persistence/Lucene/LuceneIndexPersistence.cs +++ b/src/Raven.Server/Documents/Indexes/Persistence/Lucene/LuceneIndexPersistence.cs @@ -205,9 +205,9 @@ public override void Clean(IndexCleanup mode) _converter?.Clean(); if (mode.HasFlag(IndexCleanup.Readers)) { - foreach (LowLevelTransaction llt in _index._indexStorage.Environment().ActiveTransactions.Enumerate()) + using (var tx = _index._indexStorage.Environment().ReadTransaction()) { - if (llt.TryGetClientState(out IndexStateRecord stateRecord)) + if (tx.LowLevelTransaction.TryGetClientState(out IndexStateRecord stateRecord)) { stateRecord.LuceneIndexState.Recreate(CreateIndexSearcher); } diff --git a/src/Voron/Impl/LowLevelTransaction.cs b/src/Voron/Impl/LowLevelTransaction.cs index 3f3581a415a8..b839cef467d5 100644 --- a/src/Voron/Impl/LowLevelTransaction.cs +++ b/src/Voron/Impl/LowLevelTransaction.cs @@ -50,6 +50,7 @@ public sealed unsafe class LowLevelTransaction : IDisposable, IDisposableQueryab private readonly ImmutableDictionary _scratchPagesForReads; private List _sparsePageRanges; private readonly GetPageMethod _getPageMethod; + private readonly long _id; internal long DecompressedBufferBytes; internal TestingStuff _forTestingPurposes; @@ -149,7 +150,7 @@ public Size AdditionalMemoryUsageSize public StorageEnvironment Environment => _env; - public long Id => _envRecord.TransactionId; + public long Id => _id; public bool Committed { get; private set; } @@ -181,6 +182,7 @@ public LowLevelTransaction(LowLevelTransaction previous, TransactionPersistentCo _txHeader = TxHeaderInitializerTemplate; _env = previous._env; _journal = previous._journal; + _id = previous._id; _envRecord = previous._envRecord; _freeSpaceHandling = previous._freeSpaceHandling; _allocator = allocator ?? new ByteStringContext(SharedMultipleUseFlag.None); @@ -220,9 +222,10 @@ private LowLevelTransaction(LowLevelTransaction previous, TransactionPersistentC TxStartTime = DateTime.UtcNow; DataPager = previous.DataPager; DataPagerState = previous.DataPagerState; + _id = txId; _envRecord = previous._envRecord with { - TransactionId = txId, + TransactionId = _id, Root = previous._root.ReadHeader() }; _localTxNextPageNumber = previous._localTxNextPageNumber; @@ -297,7 +300,8 @@ public LowLevelTransaction(StorageEnvironment env, TransactionPersistentContext return; } - _envRecord = _envRecord with { TransactionId = _envRecord.TransactionId + 1 }; + _id = _envRecord.TransactionId + 1; + _envRecord = _envRecord with { TransactionId = _id }; _getPageMethod = GetPageMethod.WriteScratchFirst; _env.WriteTransactionPool.Reset(); _dirtyPages = _env.WriteTransactionPool.DirtyPagesPool; @@ -313,7 +317,12 @@ public LowLevelTransaction(StorageEnvironment env, TransactionPersistentContext public bool TryGetClientState(out T value) { - if (_envRecord.ClientState is T t) + var envRecord = _envRecord; + + if (envRecord is null) + ThrowObjectDisposed(); + + if (envRecord.ClientState is T t) { value = t; return true; @@ -807,7 +816,7 @@ public void Dispose() PagerTransactionState.InvokeDispose(_env, ref DataPagerState, ref PagerTransactionState); OnDispose?.Invoke(this); - _envRecord = null; + _envRecord = null; // RavenDB-22972 - it prevents from having a possible reference cycle via EnvironmentStateRecord.ClientState } }