Skip to content

Close the SettingsClassGenerator concurrency race (T7) #29

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
guy-lud merged 1 commit into from
Jul 14, 2026
Merged

Close the SettingsClassGenerator concurrency race (T7) #29

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions FIX-PLAN.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,10 @@ _Derived from the 2026-07-10 three-part review (architecture · tests · perform
- **P5 merged (#26)** — resolve config section once per type: `ConfigurationBinder` caches the `IConfigurationSection` per section name (`ConcurrentDictionary`, zero-capture `GetOrAdd`). Plan reviewed by architect+perf+security (chose internal cache over a contract change — layering); code reviewed via `/code-review`. Gated `ConfigBinderBenchmark`: **BindNoRoot 80→40 B (−50%), BindWithRoot 144→56 B (−61%)**. Suite **71/TFM**. Also gated P4's `ConvertArrayBenchmark` (was never in the CI filter). **Security review surfaced a pre-existing leak → new item S1.**
- **Perf track P0–P5 = COMPLETE + merged.** `master` @ `498fc81`.
- **S1 shipped + merged (#27).** Conversion-failure exceptions no longer carry the bound value **or** chain the value-bearing framework inner; the value-free "required value missing" case split into its own `SettingsPropertyNullException`. `master` @ `5277c60`. Detail in §S1 below.
- **C2 shipped** (branch `refactor/c2-exception-hierarchy`, PR open) — **public exception hierarchy**: `SimpleSettingsException` base, reparent all 10, promote the 4 escapees to public, flatten 3 to root namespace, leak-safe structured properties, retype the `TypeIsNotInterface` throw. Plan reviewed by `security-auditor` + `dotnet-architect` (both ENDORSE-WITH-CHANGES, both fired cleanly) + perf in-context; code by `/code-review`. Suite **82 net10** (was 76; +6). Detail in §C2 below.
- **In flight:** C2 PR open (branch `refactor/c2-exception-hierarchy`) — carries this fix-plan + handoff refresh.
- **Next:** engine tests (T4 `ValuesPopulator` / T5 `TypeConverter` / T7 generator concurrency race) **or** continue the pre-stable breaking cleanups (A5 make `SettingsHolder` internal / C1 `List<T>` support / A6 command-line quoting / A3 `Core.AspNet` / A4 dependency floor) · **A1 (HIGH)** AOT/trim story · optional P3b.
- **C2 merged (#28).** **Public exception hierarchy**: `SimpleSettingsException` base, reparent all 10, promote the 4 escapees to public, flatten 3 to root namespace, leak-safe structured properties, retype the `TypeIsNotInterface` throw. Plan reviewed by `security-auditor` + `dotnet-architect` (both ENDORSE-WITH-CHANGES, both fired cleanly) + perf in-context; code by `/code-review`. Suite **82 net10** (was 76; +6). `master` @ `13b78dd`. Detail in §C2 below.
- **T7 (generator concurrency race) shipped** (branch `test/t7-generator-concurrency`, PR open) — `SettingsClassGenerator.GenerateType` now uses double-checked locking + a single generation gate (Reflection.Emit isn't thread-safe: concurrent `DefineType` races the shared `ModuleBuilder` for both same- and distinct-interface generation). Fast cache-hit path stays lock-free. Plan reviewed by `dotnet-architect` (ENDORSE-WITH-CHANGES — lock-all required, `Lazy`-per-type rejected) + perf/security in-context; code by `/code-review`. +2 concurrency stress tests; suite **84 net10** (was 82; 5× green).
- **In flight:** T7 PR open (branch `test/t7-generator-concurrency`) — carries this fix-plan + handoff refresh.
- **Next:** engine tests T4 `ValuesPopulator` / T5 `TypeConverter` (+ optional T7 leftovers: collection not-found / binder edge cases) **or** continue the pre-stable breaking cleanups (A5 make `SettingsHolder` internal / C1 `List<T>` support / A6 command-line quoting / A3 `Core.AspNet` / A4 dependency floor) · **A1 (HIGH)** AOT/trim story · optional P3b.
- **C3 — DECIDED (option 2):** cache in the provider only; Core `SettingsBuilder.GetSettings` unchanged; no reload. See #17.
- **Held — do NOT delete (feature work coming):** D1 Validations (reconcile with the `validate-settings` branch) · D2 EqualityCompererCreator.
- Running status lives in `SESSION-HANDOFF.md`.
Expand Down Expand Up @@ -55,7 +56,7 @@ _Derived from the 2026-07-10 three-part review (architecture · tests · perform
- [ ] T4 · `ValuesPopulator` unit tests (precedence + exception wrappers)
- [ ] T5 · `TypeConverter` unit tests (null / nullable / empty-enumerable / attribute)
- [ ] T6 · Converter unit tests (array / enumerable / Uri / DateTime + `List<T>` doc test)
- [ ] T7 · `SettingsClassGenerator` caching + concurrency stress; collection not-found; binder edge cases · *(caching now covered; concurrency race still open — see P/Q4 note)*
- [x] T7 · `SettingsClassGenerator` **concurrency race closed** — double-checked locking + single generation gate (Reflection.Emit isn't thread-safe); + same-type & distinct-type concurrent stress tests · *(caching was already covered; collection not-found / binder edge-case tests remain as a minor optional follow-up)*

**Phase 5 — Performance**
- [x] P0 · Upgrade the benchmark harness (MemoryDiagnoser + phase-split + fixtures) — do first, to measure P1–P3
Expand Down
Loading
Loading