perf: ColumnsDB minor fixes - #12831
Conversation
|
Claude finished @alexb5dh's task in 6m 41s —— View job Review:
|
| # | Severity | Finding |
|---|---|---|
| 1 | Medium | ThrowIfDisposing in GetWithIterator narrows but doesn't close the shutdown race — and disposing column managers makes it newly reachable for columns. A reader past the check can have Return() throw ObjectDisposedException from the disposed ThreadLocal, leaving a rented native Iterator undisposed and finalized after _db.Dispose(). Shutdown-only, so possibly acceptable — but it should be acknowledged rather than presented as fixed. |
| 2 | Medium | DisposableLazy<T> ships with no tests. It's new, pure-managed, concurrency-sensitive code that needs no RocksDB instance to test, and the key invariant (dispose-without-create must not invoke the factory) is exactly the leak this PR fixes. AGENTS.md: "when fixing a bug, always add a regression test". |
| 3 | Low | Behaviour change when ReadAheadSize=0: HintReadAhead reads previously still used a pooled (non-tailing) iterator with the TryCloseReadAhead sequential fast path; they now become plain point lookups. Almost certainly intended, but it's a behaviour change, not just an allocation saving. |
| 4 | Low | <summary> on CreateLazyReadAheadIteratorManager describes the return value — belongs in <returns>. Suggestion posted. |
| 5 | Low | DisposableLazy<T> placement/visibility: a general-purpose utility with no RocksDB dependency, added as a new public type in a plugin-facing assembly. Its companion DisposeIfCreated already lives in Nethermind.Core.Extensions. |
| 6 | Low | ODE from DisposableLazy names DisposableLazy<IteratorManager>, not the DB/column — unhelpful in a shutdown log. The <remarks> also doesn't cover the created-concurrently-with-disposal window. |
| 7 | Low | Missing blank line before public string Name { get; } in ColumnDb. Suggestion posted. |
Adjacent observation (pre-existing, out of scope)
DbOnTheRocks.Dispose() calls the private InnerFlush(onlyWal), which issues rocksdb_flush on the DB handle — that flushes only the default column family. ColumnsDb.Flush overrides the public method precisely because per-CF flushing "is required for WriteFlags.DisableWAL writes: they have no WAL entry, so unless their memtable is flushed they are lost on restart" — but that override is bypassed on the shutdown path. Only bites with FlushOnExit=Full (the default is WalOnly), and it predates this PR, so it's not a blocker. Flagging it because you're in this code.
Not verified
I could not run dotnet build or the test suite — the sandbox denied the build command. The review is static only; the type change from IteratorManager to DisposableLazy<IteratorManager> touches only the three construction sites I checked (ColumnDb, DbOnTheRocks, RocksDbReader) and ColumnsDb.ColumnDbSnapshot doesn't pass an iterator manager, so I'd expect it to compile, but CI should confirm.
Review checklist
- Read PR diff
- Read surrounding code (
DbOnTheRocks,ColumnDb,ColumnsDb,RocksDbReader) - Verify no consumer disposes a
ColumnDbearly (GetColumnDbcall sites, DI ownership,ReadOnlyDb) - Analyze
IteratorManagerlock for deadlock / missed-tick / finalizer safety - Analyze
DisposableLazycreate-vs-dispose races - Check disposal ordering vs
_db.Dispose()and flush-on-exit - Build / run tests — blocked (command not permitted in this sandbox)
- Post review
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
EXPB Benchmark ComparisonRun: View workflow run superblocksScenario: Client Processing (SSE)
K6 TTFB
realblocksScenario: Client Processing (SSE)
K6 TTFB
fusakaScenario: Client Processing (SSE)
K6 TTFB
|
|
Claude finished @alexb5dh's task in 5m 59s —— View job Re-review:
|
| # | Prior severity | Status |
|---|---|---|
| 1 | Medium | Acknowledged. The comment above ThrowIfDisposing (DbOnTheRocks.cs:1161) now states plainly that the check is not atomic and that a concurrent dispose/read can still reach a disposed native DB. That is the right call — the hot Get path deliberately has no check at all, so adding synchronisation here would be the wrong trade. Documented rather than pretended-fixed is what I was asking for. |
| 2 | Medium | Resolved. DisposableLazyTests.cs covers exactly the invariants that matter, including the one that guards the leak (Dispose_does_not_create_value) and a real race test. Detail below. |
| 3 | Low | Not addressed (PR description unchanged). Only affects an explicit ReadAheadSize=0; default config is unaffected. Non-blocking. |
| 4 | Low | Not addressed (<summary> still describes the return value). Non-blocking. |
| 5 | Low | Acknowledged — DisposableLazy<T> can't be internal while RocksDbReader's ctors are public. Fair. |
| 6 | Low | Acknowledged — full type name plus stack is enough to locate the owner. <remarks> extended. |
| 7 | Low | Fixed. |
The new commit (fa3910a, DB read-options disposal) checks out
This is the one part I hadn't seen before, so I traced ownership of every ReadOptions in the assembly:
_defaultReadOptions/_hintCacheMissOptions/_readAheadReadOptionsare created byCreateReadOptions()as fresh instances and never handed out — only used atDbOnTheRocks.cs:1070and:1318internally.DbOnTheRocks._readergets its own pair via the factory overload (_ownsReadOptions = true) and is disposed separately atDispose():1619.ColumnDbSnapshotowns its shared pair and destroys it itself. So no double-free path exists, andDispose()is idempotent via theInterlocked.CompareExchangeon_isDisposing.- The added
?onDestroyReadOptionsis load-bearing:_readAheadReadOptionsis genuinely null whenReadAheadSize == 0, and it also makes the call safe ifInitthrows before the fields are assigned. - Ordering is correct, and this matters because iterators pin the read options:
ColumnsDb.ReleaseUnmanagedResourcesdisposes columns (→ each column'sIteratorManager, → its iterators) →base→_iteratorManager?.Dispose()→_db.Dispose()→ only then the threeReadOptionshandles. No iterator outlives the options it was created from. - Widening the shutdown use-after-free surface:
DbOnTheRocks.Get(:781) has noThrowIfDisposing, so a read racing disposal now passes two freed handles (_db.HandleandreadOptions.Handle) intorocksdb_get_pinnedinstead of one. Same window, same outcome (AV either way) — not a new class of hazard, and covered by theThrowIfDisposingcomment. Noting it only so it's on the record.
I also re-confirmed, against the call sites added since my last pass (BlockDiffsStore, ProcessedTransactionsDbCleaner), that nothing disposes a ColumnDb obtained from GetColumnDb — ReadOnlyColumnsDb.Dispose only disposes its ReadOnlyDb wrappers, whose Dispose is _memDb.Dispose(), and nothing registers a column as an owned DI instance. So the new DestroyReadOptions in ColumnDb.Dispose still cannot fire while the DB is live.
On the tests
Good tests — dedicated threads released by a Barrier rather than pool work items, and ExceptionDispatchInfo to preserve the failing stack. Two things I checked specifically:
Value_race_with_Dispose_never_leaks's invariant (DisposeCount == factoryCalls) is not flaky: both reachable interleavings satisfy it exactly (create-then-dispose → 1/1; dispose-then-ODE → 0/0), so it can't intermittently fail on a loaded runner.- One interleaving the suite doesn't cover, and which contradicts
Value_after_Dispose_returns_value_when_already_created: reader B can observeIsValueCreated == false, block on_lockbehind reader A's factory call, and then find_disposed == true— soValuethrowsObjectDisposedExceptioneven though a live value exists. That's the safe direction (throw rather than hand out a value about to be disposed), so no change needed; just noting the contract is "throws if creation hadn't started before disposal", not "throws if not created".
Block-scoped namespace matches the majority style in Nethermind.Db.Test (8 of 14 files), so that's consistent, not a deviation.
Remaining nits (non-blocking, take or leave)
- The reworded
<remarks>onDisposableLazy<T>says "Reading an already or newly created value skips the lock" — a newly created value does not skip the lock; the creating thread holds it. The intent ("a value whose creation started before disposal is still handed out after it") is right, just phrased as if the fast path covers both. Alsowhich creation started→whose creation started, andreject using object after disposal→reject use after disposal. RocksDbReader.Getevaluates_iteratorManager.Valuebefore callingGetWithIterator, soThrowIfDisposingruns after the lazy has already been forced. During shutdown that can construct a freshIteratorManager(with itsTimer) purely to throw a line later. Harmless —DisposableLazystill disposes it — but the check would do slightly more good one frame up.
Benchmarks
superblocks and fusaka are flat-to-better; realblocks shows +4.45% AVG / +14.35% P99, which is above the ~1.8% run-to-run CV that AGENTS.md records for flat-realblocks. Mechanically I can't construct a path to a real 4% regression from this diff: on the read path the only change is _iteratorManager.Value (two null checks against a Lazy<T> whose Value/IsValueCreated are both _state is null tests, trivially inlined) on the HintReadAhead branch only, and the default ReadAheadSize is non-zero so no behaviour changes. Comparison is also against a cached master from a different run. Worth one re-run to confirm it's noise, not worth blocking on.
Not verified
Build and test execution were blocked again — dotnet build src/Nethermind/Nethermind.Db.Test/Nethermind.Db.Test.csproj -c release requires approval in this sandbox, so DisposableLazyTests was reviewed by reading, not by running. Static analysis says it should compile: I grepped every reference to IteratorManager, _readAheadReadOptions, and DisposableLazy across src/Nethermind and the only ones are the three construction sites in this diff, so the visibility narrowing of _readAheadReadOptions to private breaks nothing. CI should confirm.
Review checklist
- Read updated diff (
63eb084..HEAD) - Verify responses to prior findings 1–7
- Audit new
fa3910aread-options disposal for double-free / UAF / ordering - Trace every
ReadOptionsowner in the assembly (DbOnTheRocks,RocksDbReader,ColumnDbSnapshot,GetViewBetween,ReadFullEnumerationBatch) - Re-verify no
GetColumnDbconsumer or DI registration disposes aColumnDb(incl. newBlockDiffsStore,ProcessedTransactionsDbCleaner) - Review
DisposableLazyTestsfor coverage and flakiness - Re-check
IteratorManagerlock (deadlock, missed tick,Timer.Disposeinside lock) - Check namespace/style conventions for the new test file
- Build / run tests — blocked (command not permitted in this sandbox)
- Post review
3 participants
Makes the read-ahead iterator pool lazy, as most columns don't use it, and prevents race condition in pool's timer during shutdown.
Adds missing disposal for columns and their readers.
Split from #12813 as merging criteria are different.
Changes
IteratorManagerlazy (so most DBs don't create it and related resources at all), and skips it entirely for a DB with read-ahead turned off.ColumnsDbdisposal.ColumnDb.Disposeexisted but was never called, so every column leaked itsIteratorManager(and the manager'sTimer) for the process lifetime.RocksDbReaderinColumnDb.Dispose. The reader owns two nativeReadOptionshandles it created itself.ThrowIfDisposingcheck toDbOnTheRocks.GetWithIterator, matching the other read entry points, so a read that starts during disposal fails instead of touching a half-torn-down pool.DisposableLazy<T>, aLazy<T>that cannot leak an undisposed value.IteratorManager's cleanup timer against disposal with a lock.OnTimercould previously clear iterators whileDisposewas disposing them, causing unmanaged access violation or other issues.Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Documentation
Requires documentation update
Requires explanation in Release Notes