Memory leak & deferred GC retention of ExcelWorkbook due to unnecessary finalizers in CellStore<T>, ColumnIndex<T>, PageIndex and ExcelVmlDrawingCollection

[smad2005]

EPPlus usage

Noncommercial use

Environment

windows

Epplus version

8.6.3

Spreadsheet application

Excel

Description

In EPPlus 8.6.3, the internal classes CellStore<T>, ColumnIndex<T>, PageIndex, and ExcelVmlDrawingCollection declare destructors / finalizers (e.g. ~CellStore(), ~ExcelVmlDrawingCollection()) that perform no actual unmanaged cleanup—they merely set internal managed array references to null. Furthermore, their Dispose() methods fail to call GC.SuppressFinalize(this).
Impact on Memory:

1. Every instance of CellStore (such as MetadataCellStore) and ExcelVmlDrawingCollection is automatically placed in the GC Finalization Queue upon creation.
2. When package.Dispose() is called, these objects are NOT suppressed from finalization.
3. Upon Garbage Collection, GC moves these unsuppressed objects to the F-Reachable Queue.
4. While queued in the F-Reachable Queue, MetadataCellStore holds strong references to ExcelMetadata -> ExcelWorkbook, and ExcelVmlDrawingCollection holds ExcelWorksheet -> ExcelPackage -> ExcelWorkbook.
5. This prevents the GC from immediately reclaiming the ExcelPackage / ExcelWorkbook memory, promoting the entire object graph to Gen 1 / Gen 2 GC generations and delaying memory release until the finalizer thread executes and a subsequent GC run occurs.

---

Step to Reproduce (Repro Unit Test)

File to add: src/EPPlusTest/Core/FinalizerTests.cs (in the EPPlusTest project):

using System;
using System.Runtime.CompilerServices;
using Microsoft.VisualStudio.TestTools.UnitTesting;
using OfficeOpenXml;
using OfficeOpenXml.Core.CellStore;
using OfficeOpenXml.Drawing.Vml;
namespace EPPlusTest.Core
{
    [TestClass]
    public class FinalizerTests
    {
        [MethodImpl(MethodImplOptions.NoInlining)]
        private void CreateAndDisposePackage(out WeakReference packageRef, out WeakReference workbookRef, out WeakReference cellStoreRef, out WeakReference vmlRef)
        {
            var package = new ExcelPackage();
            var ws = package.Workbook.Worksheets.Add("Sheet1");
            ws.Cells["A1"].Value = "Test";
            var vml = ws.VmlDrawings;
            vml.AddComment(ws.Cells["A1"]);
            // Pass trackResurrection: true to detect objects retained in the F-Reachable queue
            packageRef = new WeakReference(package, true);
            workbookRef = new WeakReference(package.Workbook, true);
            cellStoreRef = new WeakReference(ws._values, true);
            vmlRef = new WeakReference(vml, true);
            package.Dispose();
        }
        [TestMethod]
        public void DisposedPackageComponentsShouldBeGarbageCollectedWithoutWaitingForFinalizers()
        {
            CreateAndDisposePackage(out var packageRef, out var workbookRef, out var cellStoreRef, out var vmlRef);
            // Force GC without waiting for pending finalizers
            GC.Collect(2, GCCollectionMode.Forced, true, true);
            Assert.IsFalse(packageRef.IsAlive, "ExcelPackage should be collected immediately after Dispose.");
            Assert.IsFalse(workbookRef.IsAlive, "ExcelWorkbook should be collected immediately after Dispose.");
            Assert.IsFalse(cellStoreRef.IsAlive, "CellStore should be collected immediately after Dispose.");
            Assert.IsFalse(vmlRef.IsAlive, "ExcelVmlDrawingCollection should be collected immediately after Dispose.");
        }
    }
}

Recommended Fixes (Exact Diffs)

Remove the unnecessary finalizers (since no native/unmanaged handles are owned) and add GC.SuppressFinalize(this) to the Dispose() methods:

1. src/EPPlus/Core/CellStore/CellStore.cs

@@ -62,7 +62,0 @@
         public CellStore()
         {
             _columnIndex = new ColumnIndex<T>[CellStoreSettings.ColSizeMin];
         }
-        ~CellStore()
-        {
-            _columnIndex = null;
-        }
         internal bool HasValues
@@ -1160,6 +1160,7 @@
         public void Dispose()
         {
             if (_columnIndex == null) return;
             lock (_syncRoot)
             {
                 for (var c = 0; c < ColumnCount; c++)
                 {
                     if (_columnIndex[c] != null)
                     {
                         ((IDisposable)_columnIndex[c]).Dispose();
                     }
                 }
                 _columnIndex = null;
             }
+            GC.SuppressFinalize(this);
         }

2. src/EPPlus/Drawing/Vml/ExcelVmlDrawingCollection.cs

@@ -52,5 +52,0 @@
-        ~ExcelVmlDrawingCollection()
-        {
-            _drawingsCellStore?.Dispose();
-            _drawingsCellStore = null;
-        }
         protected internal void AddDrawingsFromXml(ExcelWorksheet ws)
@@ -753,4 +753,5 @@
         void IDisposable.Dispose()
         {
-            _drawingsCellStore.Dispose();
+            _drawingsCellStore?.Dispose();
+            GC.SuppressFinalize(this);
         }

3. src/EPPlus/Core/CellStore/ColumnIndex.cs

@@ -30,4 +30,0 @@
-        ~ColumnIndex()
-        {
-            _pages = null;
-        }
         internal int GetPagePosition(int Row)
@@ -269,6 +269,7 @@
         public void Dispose()
         {
             if (_pages == null) return;
             for (int p = 0; p < PageCount; p++)
             {
                 (_pages[p] as IDisposable)?.Dispose();
             }
             _pages = null;
             if (_values != null) _values.Clear();
+            GC.SuppressFinalize(this);
         }

4. src/EPPlus/Core/CellStore/PageIndex.cs

@@ -48,4 +48,0 @@
-        ~PageIndex()
-        {
-            Rows = null;
-        }
         internal int Offset = 0;
@@ -144,3 +144,4 @@
         public void Dispose()
         {
             Rows = null;
+            GC.SuppressFinalize(this);
         }

[JanKallman]

Thanks,
Yes, you are right. I will remove the the finalizers as you propose. With that change I assume the GC.SuppressFinalize call are not needed?

[smad2005]

Why adding GC.SuppressFinalize(this) is correct

There are key reasons based on official .NET Framework Design Guidelines (Dispose Pattern):

1. How GC.SuppressFinalize(this) works

The GC.SuppressFinalize(this) method instructs the Garbage Collector:

"This object has already been explicitly cleaned up via Dispose(). If it (or any of its derived classes) has a finalizer, do not execute the finalizer and do not move the object into the finalization queue (F-Reachable Queue)."

2. Why it is correct to keep it even after removing the finalizer

• Protection for Derived Classes:
  For example, MetadataCellStore inherits from CellStore<MetaDataReference>. If any derived class implements or introduces a finalizer in the future, calling Dispose() on the base class will reliably suppress finalization for the entire object instance.

• Zero Overhead (No-Op):
  If a class has no finalizer registered, calling GC.SuppressFinalize(this) is effectively a no-op (a near-zero-cost bitwise flag check inside the CLR runtime). It is completely safe.

• Compliance with Microsoft Standards (Dispose Pattern Guidelines):
  According to Microsoft's official Framework Design Guidelines for IDisposable, any Dispose() method of an IDisposable class should invoke GC.SuppressFinalize(this) to prevent double cleanup and memory retention.

Summary of the Two-Level Fix:

1. Removing Finalizers (~CellStore()): Prevents objects from being registered in the finalization queue upon instantiation (new).
2. Adding GC.SuppressFinalize(this) in Dispose(): Acts as a safeguard during explicit Dispose() calls if the object was registered for finalization (e.g. in derived classes).

Therefore, this fix is a 100% standard, safe, and recommended solution in .NET.

---

As a side note: if you remove all finalizers and guarantee that they will never be added in the future, calling GC.SuppressFinalize is not strictly necessary.

[smad2005]

Minor Observation: Redundant IDisposable Implementations Across the Codebase (Via AI)

While reviewing the fix, I noticed that many IDisposable implementations across the project appear to be unnecessary, as the classes involved own no unmanaged resources (no file handles, sockets, native memory, etc.).

A quick audit of Dispose() methods across the EPPlus source:

Та же таблица. Зачёркиваю строки CellStore<T>/ColumnIndex<T>/PageIndex и ExcelVmlDrawingCollection — их "No" не доказано (живут внутри переиспользуемого ExcelWorkbook, поэтому GC-аргумент не работает). Остальное оставляю как есть.

Class	What Dispose() does	Actually needed?
ExcelRangeBase	Empty body — does nothing	❌ No
CellStoreEnumerator	Sets _cellStore = null (managed ref)	❌ No
InMemoryRange / RangeInfo	Nulls out managed references	❌ No
CellStore<T> / ColumnIndex<T> / PageIndex	Nulls out managed arrays	❌ No → disputed, not proven
ExcelVmlDrawingCollection	Calls _drawingsCellStore.Dispose() (see above)	❌ No → disputed, not proven
ZipFile / ZipPackage / CompoundDocumentFile	Closes streams and file handles	✅ Yes
TextFileLogger	Closes a StreamWriter	✅ Yes
ExcelPackage	Disposes ZipPackage and streams	✅ Yes
PictureStore	Disposes image streams	✅ Yes

In .NET, setting managed fields to null inside Dispose() provides no benefit — the GC automatically reclaims the entire unreachable managed object graph in a single collection pass.

Suggestion: Classes that contain only managed fields and implement IDisposable purely to null out references could be simplified by removing the IDisposable implementation entirely in a future cleanup.

Updated the table — struck out CellStore/ColumnIndex/PageIndex and ExcelVmlDrawingCollection. Their "No" assumed the container dies with them, but ExcelWorkbook can outlive them (pooled/reused package), so nulling there isn't dead code. You were right, thanks

[smad2005]

@JanKallman If I were a maintainer, I would start by adding GC.SuppressFinalize as a safe, minimal fix — confirm everything works correctly — and then take the time to properly audit the ~90% of unnecessary IDisposable implementations and remove them along with the SuppressFinalize calls.

Those redundant Dispose() chains (CellStore → ColumnIndex → PageIndex) simply burn CPU cycles for zero benefit — they do nothing but null out managed references that the GC would reclaim automatically anyway. At workbook scale with millions of cells, this means iterating over potentially thousands of pages and columns just to assign null, which is measurable wasted CPU on every single Dispose() call.

[JanKallman]

If CellStore, ColumnIndex, and PageIndex don't declare finalizers, then any GC.SuppressFinalize(this) calls in their Dispose() methods aren't doing anything — there's no finalizer queue entry to suppress.

The broader "remove the Dispose chains" suggestion — I'd like to slow down here

Looking at the actual implementations, I don't think these are just nulling references for no reason. A few things worth laying out:

The recursive Dispose() calls (CellStore → ColumnIndex → PageIndex) aren't only nulling fields — they're releasing reference-type payloads held inside the cell values themselves. Our internal ExcelValue struct carries an object field for the actual cell content (value and styling), and clearing/nulling those slots is what drops the last reference to that data. Skipping this doesn't eliminate the work, it just defers it — and potentially keeps that memory reachable longer than it needs to be.
The "GC would reclaim it anyway" argument holds only if the whole CellStore tree becomes unreachable at the same time Dispose() runs. In practice that's not always true — a workbook or package object can outlive the point where a store is logically done with. In those cases, releasing large column/page arrays early is exactly what helps at the "millions of cells" scale you're describing, rather than being irrelevant to it.

None of that rules out that there's real overhead worth trimming — but I'd want to see it isolated before changing behavior here. Do you have any profiler trace that isolates the cost of the null-assignment/traversal steps specifically, separate from the recursive Dispose() and list-clearing work they're bundled with?

[smad2005]

This bug report only concerns finalizers. I haven't studied the Dispose implementations in detail, but at first glance they look useless (the decision on what to do with them is always up to you).

---

Also, AI detected the following leaks:

RichDataIndexStore._incomingRelations and _outgoingRelations (Dictionary<uint, List<IndexRelation>>) grow unbounded regardless of Save()/ReIndex(). DeleteRelation() only removes the relation from the List<IndexRelation> value, never the dictionary key itself:

if (relation.To != null && _incomingRelations.ContainsKey(relation.To.Id))
{
    var pointersList = _incomingRelations[relation.To.Id];
    pointersList.RemoveAll(x => x.Id == relation.Id);
}

ReIndex() also never touches these two dictionaries (it only iterates _collections and calls IndexedCollection<T>.ReIndex()). So every entity ID that ever had a relation leaves a permanent (now-empty) key in both dictionaries for the lifetime of the workbook, with no cleanup path at all — not even via Save().

Separately, MetadataCellStore (the per-cell metadata index on ExcelWorksheet) also has no compaction path — it only grows as new (row, col) positions receive metadata, with nothing to shrink it back.

Additionally, even if we set aside the two dictionaries above, anything that relies solely on ReIndex() for cleanup will also grow unboundedly if the consumer never calls Save(). This includes:

• MetadataTypesCollection (ExcelMetadataType entries) — objects are only soft-deleted (Deleted = true) via DeleteMe()/EntityDeleted(); physical removal from the backing List<T>/Dictionary only happens in IndexedCollection<T>.ReIndex() → RemoveAll(x => x.Deleted).
• Any other IndexedCollection<T>-based registry sharing the same pattern (rich value collections, future metadata collections, etc.) — same soft-delete-only behavior.

Since RichDataIndexStore wires ReIndex() exclusively to ExcelPackage.BeforeSave (RichDataIndexStore.cs:26), a consumer that only calls Calculate() and never Save()/SaveAs()/GetAsByteArrayAsync() will see these collections grow indefinitely too — not just the two relation dictionaries. The relation dictionaries are the worse case, since even calling Save() doesn't reclaim them, but the IndexedCollection<T> registries are effectively unbounded as well for any calc-only workflow.