Allow configuring global default quantities

[angularsen]

Extracted from #1544 because choosing the global quantity catalog is an independent setup capability, not a requirement of fractional QuantityValue. Keeping it separate makes the singleton initialization and compatibility implications explicit and allows this commit to be deferred without blocking the isolated factory APIs.

Changes:

• Lazily create UnitsNetSetup.Default from the existing setup builder.
• Add ConfigureDefaults for selecting the global catalog before first use.
• Synchronize configuration and creation so concurrent first access cannot observe a partially configured builder.
• Reject configuration after the singleton has been created.

Tests:

• Add a dedicated test assembly so global singleton state is isolated from the existing suite.
• Verify configured catalog selection, default cache/parser wiring, excluded units, and rejection of reconfiguration.

[claude]

Review

Nice, focused PR — the singleton/thread-safety reasoning is careful and the Lazy<T>-based design is correct, including the subtle reentrancy case (a configuration callback that itself touches UnitsNetSetup.Default triggers BuildDefault() reentrantly under the same lock, gets caught by the second IsValueCreated check, and fails loudly instead of silently building with the wrong builder). Good attention to detail there.

Breaking changes

None. Default remains a get-only property with identical read semantics; the old static constructor is replaced by lazy initialization but observable behavior for existing callers is unchanged. ConfigureDefaults is purely additive.

Style/conventions

• DefaultConfiguration (the Lazy<UnitsNetSetup> field) and DefaultConfigurationBuilder (the nested builder type) are easy to confuse at a glance — worth a pass to make the names more distinct (e.g. DefaultSetupLazy/_lazyDefault).
• Consider cross-linking the doc comments: Default's remarks don't mention ConfigureDefaults as the way to customize it before first use, and vice versa. A <seealso> on each would help discoverability.

Generated code

No generator or generated-code changes in this PR — out of scope for that part of the review.

Code quality / potential issues

• CI gap: .github/workflows/net48-compatibility.yml hardcodes an explicit list of test projects (UnitsNet.Tests, UnitsNet.NumberExtensions.Tests, UnitsNet.NumberExtensions.CS14.Tests, UnitsNet.Serialization.JsonNet.Tests). The new UnitsNet.GlobalSetup.Tests project conditionally targets net48 on Windows but isn't in that list, so it won't actually run under net48 CI even though it's built for it. Worth adding it there, or dropping the net48 TFM from the new csproj if that coverage isn't intended yet.
• Double locking: Lazy<T>'s default mode (ExecutionAndPublication) already synchronizes factory execution internally, and ConfigureDefaults/BuildDefault additionally take DefaultConfigurationLock. This is correct (not a bug) but the two locks somewhat duplicate each other's job — a comment explaining why the outer lock is still needed (to make the builder swap + creation check atomic) would help future readers, since it's not obvious from the code alone why Lazy's own thread-safety isn't sufficient.

Test coverage

• The single test in UnitsNetSetupGlobalConfigurationTests is thorough for the happy path (configures, verifies quantities/cache/parser wiring, excluded units, rejection of reconfiguration), matching the PR description.
• Given this exercises genuine process-wide static state, it'd be worth a comment at the top of the test class (or file) noting that this assembly must stay single-test / non-parallel, since any additional test that touches UnitsNetSetup.Default or ConfigureDefaults will interact with this one through shared static state regardless of xunit's parallelization settings. That constraint isn't obvious to a future contributor adding a second test here.
• Not covered: calling UnitsNetSetup.Default first (forcing default construction) and then calling ConfigureDefaults, which per the code should throw InvalidOperationException — this is the "used before configured" path and is arguably the more common real-world mistake vs. double-configuration.
• No concurrency test for the race between Default and ConfigureDefaults on separate threads, though the manual code-review of the locking above suggests it's correct.

Performance

No concerns — the lock/lazy overhead only applies to first access of Default/ConfigureDefaults, not to steady-state use.

Security

None identified.

[claude]

Review: Allow configuring global default quantities (#1692)

Solid, well-scoped change with good isolated test coverage for the singleton edge cases. One concurrency issue worth fixing before merge; a few smaller notes below.

🐛 Potential deadlock in ConfigureDefaults (UnitsNet/CustomCode/UnitsNetSetup.cs)

ConfigureDefaults acquires DefaultConfigurationLock and, while still holding it, calls DefaultConfiguration.Value (line ~149). Lazy<T> constructed via new Lazy<T>(factory) defaults to LazyThreadSafetyMode.ExecutionAndPublication, which holds its own internal monitor for the duration of the factory call — and the factory (BuildDefault) itself acquires DefaultConfigurationLock.

That gives two different lock-acquisition orders on two different code paths:

• First access to Default (no prior ConfigureDefaults call): Lazy-internal-lock → DefaultConfigurationLock (via Default → Lazy.Value → BuildDefault → lock(DefaultConfigurationLock)).
• ConfigureDefaults: DefaultConfigurationLock → Lazy-internal-lock (via lock(DefaultConfigurationLock) → ... → DefaultConfiguration.Value).

If a thread calls UnitsNetSetup.Default for the first time on one thread while another thread concurrently calls ConfigureDefaults, you can get:

1. Thread A enters Lazy.Value, takes the Lazy-internal-lock, starts running BuildDefault.
2. Before A acquires DefaultConfigurationLock, Thread B enters ConfigureDefaults and acquires DefaultConfigurationLock first.
3. A now blocks waiting for DefaultConfigurationLock (held by B).
4. B finishes configuring and calls DefaultConfiguration.Value, which blocks waiting for the Lazy-internal-lock (held by A).

→ classic lock-order inversion, both threads hang forever. This is exactly the kind of race ConfigureDefaults is meant to guard against (calling it concurrently with incidental early use), so it seems worth closing.

A simple fix: don't call DefaultConfiguration.Value while still holding DefaultConfigurationLock. Release the lock after swapping _defaultConfigurationBuilder, then call .Value outside the lock:

lock (DefaultConfigurationLock)
{
    if (DefaultConfiguration.IsValueCreated) throw ...
    var builder = new DefaultConfigurationBuilder();
    configuration(builder);
    if (DefaultConfiguration.IsValueCreated) throw ...
    _defaultConfigurationBuilder = builder;
}
return DefaultConfiguration.Value;

This preserves the atomicity you need (builder swap still guarded, "created while being configured" check still works for the same-thread-reentrancy case) without nesting the two locks in opposite orders across threads.

(Note: the second IsValueCreated check inside the lock is a nice touch for catching a caller who reentrantly touches UnitsNetSetup.Default from inside their own configuration callback on the same thread — worth keeping.)

Style

• DefaultConfigurationLock (PascalCase, no underscore) is inconsistent with _defaultConfigurationBuilder right below it and with the rest of the codebase's private field convention (_camelCase, e.g. QuantityParser._unitParser, UnitKey._unitEnumType). Minor, but easy to fix.

API / breaking changes

• No breaking changes — ConfigureDefaults is purely additive, and Default keeps its public signature/behavior (still lazily produces the same built-in catalog if never configured).
• Minor behavioral nuance worth being aware of: previously Default was populated in a static UnitsNetSetup() cctor, so an exception during construction would be cached forever as a TypeInitializationException for the AppDomain. With Lazy<T> in ExecutionAndPublication mode, a failed factory call is not cached — every subsequent access retries BuildDefault(). Probably a net improvement, but worth calling out since it's a subtle change in failure semantics if a custom catalog's Build() can throw.

Tests

• Good use of separate assemblies (UnitsNet.GlobalSetup.Tests, UnitsNet.GlobalSetup.DefaultFirst.Tests) to isolate the process-wide singleton — and the comments explaining why each has exactly one test are helpful.
• ConfigureDefaults_AfterDefaultIsCreated_ThrowsInvalidOperationException and ConfigureDefaults_BeforeFirstUse_ConfiguresAndFreezesDefaultSetup cover the main happy/sad paths well (rejection after creation, catalog restriction taking effect, wiring of UnitAbbreviations/UnitParser to the configured instance).
• No test for the concurrent-access race above — understandably hard to make deterministic in a unit test, but given the singleton is explicitly designed to be safe for "concurrent first access," it'd be good to at least have a stress test (e.g. spin N threads hammering Default while one calls ConfigureDefaults) even if it only catches the bug probabilistically under CI.
• net48-compatibility.yml was correctly updated to include both new test projects, and UnitsNet.slnx was updated too — consistent with existing patterns.

No new quantities/units, no generated-code changes, and no security concerns in this PR.

[claude]

Review: Allow configuring global default quantities (PR #1692)

Reviewed against AGENTS.md conventions. Small, well-scoped PR (extracted from #1544).

POTENTIAL DEADLOCK (highest priority):
UnitsNetSetup.ConfigureDefaults and the lazy UnitsNetSetup.Default getter can deadlock when invoked concurrently from two threads, due to lock-order inversion.

In UnitsNet/CustomCode/UnitsNetSetup.cs, BuildDefault() acquires DefaultConfigurationLock from inside the Lazy factory callback, i.e. while the Lazy instance's own internal execution lock is already held. Meanwhile ConfigureDefaults() acquires DefaultConfigurationLock first, then at the end calls DefaultConfiguration.Value, which needs to enter that same Lazy internal lock.

Scenario: Thread A calls UnitsNetSetup.Default for the first time (or any of the many forwarding call sites such as Quantity.Parse, UnitAbbreviationsCache.Default, UnitParser.Default). This enters Lazy.Value, acquiring the Lazy internal lock, then calls BuildDefault(), which blocks waiting for DefaultConfigurationLock. Concurrently, Thread B calls ConfigureDefaults(...): it acquires DefaultConfigurationLock, runs the user callback, sets _defaultConfigurationBuilder, then calls DefaultConfiguration.Value, which blocks waiting for the Lazy internal lock held by Thread A. Thread A now waits on a lock held by B, and B waits on a lock held by A: deadlock.

This is exactly the race ConfigureDefaults is meant to guard against (the IsValueCreated checks exist specifically to reject configuration after first use), so it is a realistic startup race rather than a purely theoretical one, e.g. one thread starts formatting or parsing a quantity while another thread is still calling ConfigureDefaults during app initialization.

Possible fix directions: avoid calling DefaultConfiguration.Value while still holding DefaultConfigurationLock inside ConfigureDefaults, or avoid re-entering a lock inside BuildDefault at all (e.g. use Volatile.Read/Volatile.Write on the builder field instead of a full lock, since Lazy already guarantees single execution of the factory).

OTHER OBSERVATIONS:

Correctness (non-blocking path): the double IsValueCreated check in ConfigureDefaults, before and after invoking the caller's configuration callback, correctly catches reentrant calls to Default from within that callback (via the reentrant lock keyword), which would otherwise silently return a stale value built from the old builder. Good design, just undermined by the deadlock above.

Behavior change (minor, non-breaking): switching from a static constructor to Lazy changes failure semantics subtly. Previously a failure during static init raised TypeInitializationException and poisoned all static members of the type; now only Default itself fails repeatedly (Lazy caches exceptions by default) while Create and ConfigureDefaults remain usable. Likely a positive change, but worth calling out as intentional since it changes exception type and scope for anyone currently catching TypeInitializationException.

Test coverage: good use of two separate test assemblies, UnitsNet.GlobalSetup.Tests and UnitsNet.GlobalSetup.DefaultFirst.Tests, to work around Default being a permanent process-wide singleton that cannot be reset between tests. The XML doc remarks explaining why each assembly has a single test is a nice touch. Missing: no coverage for the concurrent-access race described above (understandably hard to make a reliable, non-flaky test for, but worth a comment or follow-up issue), and no test for ConfigureDefaults where the configuration callback itself throws.

Style and conventions: XML docs, exception types, and the builder pattern match existing conventions in the file. The new csproj files match the boilerplate of sibling test projects such as UnitsNet.NumberExtensions.Tests. The net48-compatibility.yml workflow was correctly updated to include both new test projects.

Breaking changes: none in the public API. ConfigureDefaults is purely additive, and the Default property's signature is unchanged.

Generated code and new quantities: not applicable, this PR does not touch Common/UnitDefinitions or generated code.

Overall this is a well-designed, minimal API addition with a good test isolation strategy. The deadlock is the one issue worth resolving, or explicitly ruling out with reasoning, before merging.

[claude]

Review: Allow configuring global default quantities (#1692)

Scope: this PR only touches UnitsNetSetup (global default singleton config) plus new test assemblies. No unit/quantity JSON changes, no code-gen changes, so the criteria for new quantities/units and generated-code guidance don't apply here.

Breaking changes

None. UnitsNetSetup.Default keeps its public signature (public static UnitsNetSetup Default { get; } becomes an expression-bodied property, which is source/binary compatible). ConfigureDefaults is purely additive. The only semantic shift is that Default creation moves from eager (static constructor, at type load) to lazy (first access) — worth a one-line callout in release notes since it changes when Quantity.DefaultProvider.Quantities and UnitConverter.CreateDefault() run, but it's not an API break.

Potential bug: race window between ConfigureDefaults and first Default access

BuildDefault() (UnitsNet/CustomCode/UnitsNetSetup.cs:102-108) takes DefaultConfigurationLock, builds the setup, and releases the lock before returning to Lazy<T>. Lazy<T>.IsValueCreated only flips to true afterwards, once Lazy<T> itself publishes the result under its own internal lock. That leaves a small window where:

• Thread A is inside Lazy<UnitsNetSetup>.Value (already released DefaultConfigurationLock, not yet published).
• Thread B calls ConfigureDefaults(...), acquires the now-free DefaultConfigurationLock, and reads DefaultConfiguration.IsValueCreated == false (both checks, before and after invoking the user's configuration callback).
• Thread B overwrites _defaultConfigurationBuilder and calls DefaultConfiguration.Value, which blocks on Lazy<T>'s internal lock (still held by Thread A) and then returns Thread A's already-computed (old) value — not Thread B's configuration.

Net effect: ConfigureDefaults silently returns a UnitsNetSetup that doesn't reflect the caller's configuration and does not throw, even though the doc and PR description explicitly promise "reject configuration after the singleton has been created" / "concurrent first access cannot observe a partially configured builder." It's a narrow window (needs Default and ConfigureDefaults racing on separate threads at startup), so unlikely to bite in practice, but it defeats the stated goal of this PR and would be hard to diagnose if hit (no exception, wrong quantities silently in effect).

Suggested fix: don't rely on Lazy<T>.IsValueCreated for the guard — set your own bool (e.g. _defaultCreated) to true inside BuildDefault() while still holding DefaultConfigurationLock, and check that flag in ConfigureDefaults instead. That keeps the "already created" check and the actual creation atomic under the same lock.

Code quality / style

• Matches the file's existing conventions (is null argument guards, XML doc with <seealso> cross-refs). Good.
• Minor nit: DefaultConfigurationLock / DefaultConfiguration use PascalCase for private static fields while _defaultConfigurationBuilder uses _camelCase — both styles exist elsewhere in the codebase, but mixing them in the same handful of lines reads a little inconsistently.
• The comment on DefaultConfigurationLock ("synchronizes ... already synchronized by Lazy") is the right idea but, per above, doesn't fully hold in practice — worth revisiting once the fix above is in.

Test coverage

Nice approach: putting ConfigureDefaults-before-first-use and ConfigureDefaults-after-first-use into two separate single-test assemblies (UnitsNet.GlobalSetup.Tests, UnitsNet.GlobalSetup.DefaultFirst.Tests) is a reasonable way to work around the fact that UnitsNetSetup.Default can never be reset within a process. Both are wired into UnitsNet.slnx and the net48 compatibility workflow.

One gap: neither test can realistically exercise the race window above — that's expected (timing-dependent races are hard to unit test), but if the fix above is applied, it'd be worth a comment noting the invariant is enforced by construction (single lock owns both check-and-create) rather than relying on a test to prove it.

Other

• .github/workflows/net48-compatibility.yml: new projects added to the explicit test list, consistent with the existing pattern.
• UnitsNet.slnx: both new projects registered correctly.

Overall this is a well-scoped, additive change with good test isolation for the singleton-state problem; the main thing worth addressing before merge is the check/create race described above.

---

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 96%. Comparing base (670ba36) to head (633ff0c).

Files with missing lines	Patch %	Lines
UnitsNet/CustomCode/UnitsNetSetup.cs	94%	1 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff           @@
##           master   #1692   +/-   ##
======================================
- Coverage      96%     96%   -1%     
======================================
  Files         455     455           
  Lines       29540   29553   +13     
======================================
+ Hits        28502   28514   +12     
- Misses       1038    1039    +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.