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

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:

Every instance of CellStore (such as MetadataCellStore) and ExcelVmlDrawingCollection is automatically placed in the GC Finalization Queue upon creation.
When package.Dispose() is called, these objects are NOT suppressed from finalization.
Upon Garbage Collection, GC moves these unsuppressed objects to the F-Reachable Queue.
While queued in the F-Reachable Queue, MetadataCellStore holds strong references to ExcelMetadata -> ExcelWorkbook, and ExcelVmlDrawingCollection holds ExcelWorksheet -> ExcelPackage -> ExcelWorkbook.
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:

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);
         }

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);
         }

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);
         }

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);
         }

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?

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:

Removing Finalizers (~CellStore()): Prevents objects from being registered in the finalization queue upon instantiation (new).
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.

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

@JanKallman

@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.

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?

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.

Thanks, I will look at the issues you point out.

#2456-Removed Finalizers from the cell store and the ExcelVmlDrawingC…

#2456-Removed finalizers on cell store classes. (#2477)

Feature/drawing renderer refactor (#2480)

Navigation Menu

Repository navigation

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

EPPlus usage

Environment

Epplus version

Spreadsheet application

Description

Step to Reproduce (Repro Unit Test)

Recommended Fixes (Exact Diffs)

Why adding `GC.SuppressFinalize(this)` is correct

1. How `GC.SuppressFinalize(this)` works

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

Summary of the Two-Level Fix:

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

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

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

Description

EPPlus usage

Environment

Epplus version

Spreadsheet application

Description

Step to Reproduce (Repro Unit Test)

Recommended Fixes (Exact Diffs)

Activity

JanKallman commented on Aug 6, 2026

smad2005 commented on Aug 6, 2026

Why adding GC.SuppressFinalize(this) is correct

1. How GC.SuppressFinalize(this) works

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

Summary of the Two-Level Fix:

smad2005 commented on Aug 6, 2026

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

smad2005 commented on Aug 6, 2026

JanKallman commented on Aug 7, 2026

smad2005 commented on Aug 7, 2026

JanKallman commented on Aug 10, 2026

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Why adding `GC.SuppressFinalize(this)` is correct

1. How `GC.SuppressFinalize(this)` works

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