Skip to content

Commit

Permalink
RavenDB-22972 Dequeued EnvirnmentStateRecord elements should be expli…
Browse files Browse the repository at this point in the history
…ctly nulled because they might hold references of objects which have finalizers (ClientState)
  • Loading branch information
arekpalinski committed Oct 22, 2024
1 parent 889cecd commit 18407e3
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 12 deletions.
5 changes: 5 additions & 0 deletions src/Voron/EnvironmentStateRecord.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,8 @@ public record EnvironmentStateRecord(
(JournalFile Current, long Last4KWritePosition) Journal,
object ClientState,
List<long> SparsePageRanges);

internal sealed class EnvironmentStateRecordHolder
{
public EnvironmentStateRecord EnvStateRecord;
}
30 changes: 18 additions & 12 deletions src/Voron/StorageEnvironment.cs
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ public static IDisposable GetStaticContext(out ByteStringContext ctx)

internal TestingStuff _forTestingPurposes;
private EnvironmentStateRecord _currentStateRecord;
private readonly ConcurrentQueue<EnvironmentStateRecord> _transactionsToFlush = new();
private readonly ConcurrentQueue<EnvironmentStateRecordHolder> _transactionsToFlush = new();

internal EnvironmentStateRecord CurrentStateRecord => _currentStateRecord;

Expand Down Expand Up @@ -1340,11 +1340,11 @@ public InMemoryStorageState GetInMemoryStorageState(LowLevelTransaction tx)
},
TransactionsToFlush = _transactionsToFlush.ToList().Select(x => new InMemoryStorageState.EnvironmentStateRecordDetails
{
TransactionId = x.TransactionId,
ScratchPagesTable = GetScratchTableSummary(x.ScratchPagesTable),
NextPageNumber = x.NextPageNumber,
WrittenToJournalNumber = x.WrittenToJournalNumber,
ClientStateType = x.ClientState?.GetType().ToString()
TransactionId = x.EnvStateRecord.TransactionId,
ScratchPagesTable = GetScratchTableSummary(x.EnvStateRecord.ScratchPagesTable),
NextPageNumber = x.EnvStateRecord.NextPageNumber,
WrittenToJournalNumber = x.EnvStateRecord.WrittenToJournalNumber,
ClientStateType = x.EnvStateRecord.ClientState?.GetType().ToString()
}).ToList(),
FlushState = new InMemoryStorageState.FlushStateDetails
{
Expand Down Expand Up @@ -1661,12 +1661,12 @@ private void UpdateStateOnCommit(LowLevelTransaction tx)

// we don't _have_ to make it using interlocked, but let's publish it immediately
Interlocked.Exchange(ref _currentStateRecord, updatedState);

// We only want to flush to data pager transactions that have been flushed to the journal.
// Transactions that _haven't_ been flushed are mostly book-keeping (updating scratch table, etc)
if (tx.WrittenToJournalNumber >= 0)
{
_transactionsToFlush.Enqueue(updatedState);
_transactionsToFlush.Enqueue(new EnvironmentStateRecordHolder { EnvStateRecord = updatedState });
}
}

Expand All @@ -1687,16 +1687,24 @@ internal ApplyLogsToDataFileState TryGetLatestEnvironmentStateToFlush(long uptoT
while (true)
{
if (_transactionsToFlush.TryPeek(out var maybe) == false ||
maybe.TransactionId >= uptoTxIdExclusive)
maybe.EnvStateRecord.TransactionId >= uptoTxIdExclusive)
{
if (found == false)
return null;
Debug.Assert(record is not null);
return new ApplyLogsToDataFileState(scratchBuffers, sparseRegions, record);
}
if (_transactionsToFlush.TryDequeue(out record) == false)

if (_transactionsToFlush.TryDequeue(out EnvironmentStateRecordHolder recordHolder) == false)
throw new InvalidOperationException("Failed to get transaction to flush after already peeked successfully");

record = recordHolder.EnvStateRecord;

// single thread is reading from this, so we can be sure that peek + take gets the same value
Debug.Assert(ReferenceEquals(record, maybe.EnvStateRecord));

recordHolder.EnvStateRecord = null; // this way we ensure that _transactionsToFlush won't hold reference in its internal slots preventing GC on EnvironmentStateRecord.ClientState object

if (record.SparsePageRanges != null)
{
sparseRegions.AddRange(record.SparsePageRanges);
Expand All @@ -1708,8 +1716,6 @@ internal ApplyLogsToDataFileState TryGetLatestEnvironmentStateToFlush(long uptoT
scratchBuffers.Add(pageFromScratch);
}

// single thread is reading from this, so we can be sure that peek + take gets the same value
Debug.Assert(ReferenceEquals(record, maybe));
found = true;
}
}
Expand Down

0 comments on commit 18407e3

Please sign in to comment.