Fix transaction finalizer causing LiteDB.LiteException/System.ObjectDisposedException

[ejdre-vestas]

I believe that when #1906 was introduced, a bug was also added.

When the transaction is finalized, the code is calling _monitor.ReleaseTransaction, and will eventually do:

LiteDB/LiteDB/Engine/Services/TransactionMonitor.cs

Lines 116 to 119 in 48d89fd

	if (keepLocked == false)
	{
	_locker.ExitTransaction();
	}

The finalizers always run on a specific thread controlled by the Garbage Collector, so the thread that is releasing the transaction will never have acquired a lock.

I was running version 5.0.15, which is before this band-aid fix #2280 (two of the referenced issues also contain stack traces starting on a Finalize), so I was able to consistently get an exception anytime the Finalize was called.

Even though the exception is hidden on newer versions, it still makes sense to me to have this fixed.
Edit: The new version will still throw an exception in another place, so this PR is still required to fix problems calling finalizers.
2 exceptions still come up without this fix:

Unhandled exception. System.ObjectDisposedException: Cannot access a disposed object.
Object name: 'System.Threading.ThreadLocal`1[[LiteDB.Engine.TransactionService, LiteDB, Version=6.0.0.0, Culture=neutral, PublicKeyToken=null]]'.
   at System.Threading.ThreadLocal`1.GetValueSlow()
   at LiteDB.Engine.TransactionMonitor.ReleaseTransaction(TransactionService transaction) in /home/edureis95/repo/LiteDB/LiteDB/Engine/Services/TransactionMonitor.cs:line 145
   at LiteDB.Engine.TransactionService.Dispose(Boolean dispose) in /home/edureis95/repo/LiteDB/LiteDB/Engine/Services/TransactionService.cs:line 446
   at LiteDB.Engine.TransactionService.Finalize() in /home/edureis95/repo/LiteDB/LiteDB/Engine/Services/TransactionService.cs:line 79
   at System.GC.RunFinalizers()

Unhandled exception. LiteDB.LiteException: current thread must contains transaction parameter
   at LiteDB.Constants.ENSURE(Boolean conditional, String message) in /home/edureis95/repo/LiteDB/LiteDB/Utils/Constants.cs:line 146
   at LiteDB.Engine.TransactionMonitor.ReleaseTransaction(TransactionService transaction) in /home/edureis95/repo/LiteDB/LiteDB/Engine/Services/TransactionMonitor.cs:line 145
   at LiteDB.Engine.TransactionService.Dispose(Boolean dispose) in /home/edureis95/repo/LiteDB/LiteDB/Engine/Services/TransactionService.cs:line 446
   at LiteDB.Engine.TransactionService.Finalize() in /home/edureis95/repo/LiteDB/LiteDB/Engine/Services/TransactionService.cs:line 79
   at System.GC.RunFinalizers()

[Copilot]

Pull Request Overview

This PR refactors the transaction disposal logic by extracting a new RemoveTransaction method and removes unused imports. The primary goal is to provide a way to dispose and remove transactions from the monitor without releasing thread locks, separating concerns between transaction cleanup and lock management.

• Extracted RemoveTransaction method from ReleaseTransaction to separate transaction disposal from lock release
• Updated TransactionService.Dispose to call RemoveTransaction instead of ReleaseTransaction when called from finalizer
• Removed unused imports (System.Collections.Concurrent, System.Runtime.InteropServices, and System.Threading from TransactionService)

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
LiteDB/Engine/Services/TransactionService.cs	Updated finalizer path to call RemoveTransaction instead of ReleaseTransaction; removed unused imports
LiteDB/Engine/Services/TransactionMonitor.cs	Extracted RemoveTransaction method from ReleaseTransaction; removed unused imports; minor whitespace fixes

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Calling transaction.Dispose() within RemoveTransaction creates infinite recursion. The finalizer calls Dispose(false), which calls RemoveTransaction, which calls Dispose() again. This will result in a stack overflow. The RemoveTransaction method should not call transaction.Dispose() since it's already being called from within the transaction's own Dispose(bool) method.

Suggested change

	transaction.Dispose();
	// transaction.Dispose(); // Removed to prevent infinite recursion

[ejdre-vestas]

I did not change this behavior, but I think this comment is wrong anyway.
Calling the Dispose here will do

this.Dispose(true);
GC.SuppressFinalize(this);

The dispose boolean is true, and the finalizer will never be called because that is being suppressed.

[Copilot]

This assignment to keepLocked is useless, since its value is never read.

[ejdre-vestas]

It's used in the original ReleaseTransaction method.

[JKamsker]

Thank you for that! Please target your pr at dev and provide atleast one regression test if possible.

[JKamsker]

@codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Chef's kiss.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[ejdre-vestas]

Thank you for that! Please target your pr at dev and provide atleast one regression test if possible.

I have added the test, and if you switch the code back to call _monitor.ReleaseTransaction(this);, the test will fail with UnhandledException mentioned in the PR description. I know it's annoying that this is unhandled, but the GC thread "swallows" the exception, so you cannot catch it.

[ejdre-vestas]

Even though I made this fix, I think the overall best change is to remove the finalizer completely.

Please someone correct me if I'm wrong but the conclusion that led to this implementation (written in #1772 (comment)), is not valid.

The author wanted to remove a TransactionService from the TransactionMonitor when a thread ends. From his sentence, he believed that the thread ending would trigger the finalizer. However, that's not how GC does things. The finalizer will only ever be called when there are no longer any strong references to an object, and TransactionMonitor holds a strong reference to all TransactionServices. So, essentially, the finalizer will never be called unless the TransactionService is removed from TransactionMonitor (or both are ready to be collected by the GC). If the whole point of the finalizer is to perform this removal, you can see where I'm getting at; it will never do what it was supposed to.

@JKamsker please let me know how I should proceed.

[JKamsker]

Thanks for your contribution. Unfortunately this exceeds my current knowledge of this topic - so all i can do is to trust you and the supporting argumentation of my AI 😅 - i will merge it

For my future reference:

• Supporting .NET docs line up with (most of) the author’s argument:

  - Finalizers run on GC’s schedule, not on “thread end.” Object.Finalize is called after the GC discovers an object is inaccessible (unreachable), and the runtime makes
    no guarantee about the thread or ordering. (learn.microsoft.com
    (https://learn.microsoft.com/en-us/dotnet/fundamentals/runtime-libraries/system-object-finalize?utm_source=openai))
  - If a finalizer throws, it can tear down the process. The .NET docs explicitly say an exception escaping Finalize terminates the process (unless a host overrides pol
    icy). (learn.microsoft.com (https://learn.microsoft.com/en-us/dotnet/fundamentals/runtime-libraries/system-object-finalize?utm_source=openai))
  - You shouldn’t touch managed objects in a finalizer. The C# guide warns managed objects may already be disposed / invalid during finalization. (learn.microsoft.com
    (https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/finalizers))
  - ThreadLocal<T>.Value throws once disposed. If TransactionMonitor._slot (a ThreadLocal<TransactionService>) has been disposed/finalized, reading .Value throws Object
    DisposedException. (learn.microsoft.com (https://learn.microsoft.com/en-us/dotnet/api/system.threading.threadlocal-1.value?view=net-8.0))

  Reasoning about LiteDB’s specific failure mode:

  1. Why calling ReleaseTransaction() from a finalizer is fundamentally unsafe
      - ReleaseTransaction() is thread-affine in behavior: it may call _locker.ExitTransaction() and it checks/clears _slot.Value for the current thread.
      - A finalizer runs on a runtime-controlled thread and not “the thread that acquired the lock / owns the slot”, so those invariants can fail (e.g., “exit lock I
        never entered”, or _slot.Value != transaction), producing exactly the kinds of exceptions seen in the PR description.
      - Because exceptions escaping a finalizer can terminate the process, this is a high-severity bug, not just “noise”. (learn.microsoft.com
        (https://learn.microsoft.com/en-us/dotnet/fundamentals/runtime-libraries/system-object-finalize?utm_source=openai))
  2. Why the ObjectDisposedException scenario is plausible
      - TransactionMonitor._slot is a managed object with its own lifetime (and finalization/Dispose). Finalizer ordering isn’t guaranteed, and the C# docs warn managed
        members may already be disposed during finalization. (learn.microsoft.com
        (https://learn.microsoft.com/en-us/dotnet/fundamentals/runtime-libraries/system-object-finalize?utm_source=openai))
      - If _slot is disposed before TransactionService finalizes, then ReleaseTransaction() touching _slot.Value can throw ObjectDisposedException exactly as documented.
        (learn.microsoft.com (https://learn.microsoft.com/en-us/dotnet/api/system.threading.threadlocal-1.value?view=net-8.0))
  3. Is the author’s “the original finalizer motivation is invalid” claim correct?
      - The premise “thread ends ⇒ finalizer runs” is incorrect per the GC model: finalization is driven by reachability, not thread lifecycle. (learn.microsoft.com
        (https://learn.microsoft.com/en-us/dotnet/fundamentals/runtime-libraries/system-object-finalize?utm_source=openai))
      - And yes, if TransactionMonitor strongly references every TransactionService in _transactions, then as long as the monitor is reachable, those transactions are a
        lso reachable and won’t be finalized (because they’re still in the GC reachability graph). (learn.microsoft.com
        (https://learn.microsoft.com/en-us/dotnet/standard/garbage-collection/fundamentals))
      - So the author’s skepticism about the original justification is well-founded.

  Net: the PR’s change (introducing RemoveTransaction() and using it from the finalizer path) is consistent with .NET guidance: it removes the most obviously thread-dep
  endent operations from the finalizer path, reducing chances of a fatal finalizer exception. The author’s broader point (“maybe remove the finalizer entirely”) is also
  reasonable, because the docs strongly discourage managed-object work in finalizers and because finalizers can’t fix leaks caused by strong references anyway. (learn.m
  icrosoft.com (https://learn.microsoft.com/en-us/dotnet/fundamentals/runtime-libraries/system-object-finalize?utm_source=openai))

[JKamsker]

Thanks alot for your contribution!