Describe what the fixture's class-per-database actually buys - #2385
Merged
Conversation
The comment claimed IClassFixture keeps "xUnit's cross-class parallelism intact". The file separation is real, but DuckDbInitializer's lock is private static readonly, so every read and write in the suite queues on one process-wide lock regardless of which database file it targets. The classes serialize anyway -- through a lock instead of through a collection, and without a collection's ordering. What the separation does buy is the schema-build cost, which is the thing the fixture exists to amortise: ~80 DDL statements per class, running concurrently. Says explicitly that the lock must NOT be narrowed to fix this. Production creates several DuckDbInitializer instances over the same App.DatabasePath, and the static lock is what keeps them mutually exclusive -- per-instance would trade a slow suite for a real data race, which is the obvious wrong fix. Also records the practical consequence, since it already cost a nightly: scheduling pressure can starve the 5-second write-lock acquisition in GetDatabaseStateDeviationsAsync, whose maintenance block is best-effort and skips on timeout (#2374). Comment only, no behaviour change. Refs #2376
|
Reviewed. This is a comment-only change to I verified the factual claims the new comment makes against the actual code:
No correctness, parity, security, or performance concerns:
Good catch turning a stale/false invariant into an accurate one before it misled another reader the way it apparently did during #2374 triage. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #2376, reduced scope after correcting my own numbers on that issue.
I filed #2376 saying the process-wide lock was costing a 500-second suite. That figure came from the one run that FAILED — the flake. Successful runs are 190–222 s. I generalised from a single outlier, so the "restructure 42 classes into a collection" idea is not justified and I am not proposing it.
What survives is smaller and still worth fixing: the comment asserts a property the lock does not permit.
DuckDbInitializer's lock isprivate static readonly, so every read and write in the suite queues on one process-wide lock regardless of which database file it targets. The classes serialize anyway — through a lock instead of through a collection, and without a collection's ordering. The file separation is real, but what it buys is the schema-build cost running concurrently (~80 DDL statements per class), which is what the fixture exists to amortise. That is now what the comment says.A comment stating a false invariant is worse than no comment, because the next reader reasons from it — I did, while diagnosing #2374, and it cost me a wrong hypothesis before I checked the lock's declaration.
Two things deliberately included:
DuckDbInitializerinstances over the sameApp.DatabasePath(MainWindow, DatabaseStateOverridesWindow, DuckDbAlertHistoryStore), and the static lock is what keeps them mutually exclusive. Per-instance is the obvious-looking fix and would trade a slow suite for a real data race. Better to say so where someone would go looking.GetDatabaseStateDeviationsAsync, whose maintenance block is best-effort and skips on timeout (Flaky: DatabaseStateExpectedStoreTests.ResetToCurrent_RebaselinesAndClearsOverride blocked a nightly with no Lite change #2374).Comment only, no behaviour change. The
LockRecursionException→ no-op-disposable swallow noted on the issue is left untouched — withNoRecursionit fires for any nesting rather than only the leak its comment describes, but I have not tied it to an observed failure, and changing lock semantics over a comment-level concern is a bad trade.