Initialize the property mapping collection exactly once

Address the review remark on PropertyMappingsInternal: the lock-free
CompareExchange pattern allowed racing threads to each run the property
mapper and build their own PropertyMappingCollection, with only one of
the builds ever being published. Since the mapper may be user-supplied
and the collection is the mutable object callers manipulate through
PropertyMappings, initialization now takes a lock on the cold path so
the mapper runs exactly once.

The warm path is unchanged - a single Volatile.Read with no lock and no
allocation (re-verified at 0 B per access on a warmed-up mapping). The
other lazy members keep the lock-free pattern; their factories are
idempotent and produce values that are never mutated afterwards, so a
duplicate run is unobservable there. The comment on the member now
spells out this distinction.

Adds a test racing 64 threads on first access, asserting the mapper
runs exactly once and all threads observe the same collection.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PtLPvgYW89N5hTebTKPQBk

Author: claude

src/Hl7.Fhir.Base/Introspection/ClassMapping.cs

@@ -187,6 +187,7 @@ internal static bool TryCreate(ModelInspector parent, Type type, [NotNullWhen(tr
     // This list is created lazily. This not only improves initial startup time of
     // applications but also ensures circular references between types will not cause loops.
     private PropertyMappingCollection? _mappings;
+    private readonly object _mappingsLock = new();
 
     // Note: this member - like the other lazily initialized members below, and the equivalent
     // members in PropertyMapping and PropertyMappingCollection - deliberately does not use
@@ -197,11 +198,21 @@ internal static bool TryCreate(ModelInspector parent, Type type, [NotNullWhen(tr
     // cached by the compiler in a static field, so the LazyInitializer calls elsewhere in this
     // assembly cost nothing per access and are left alone.)
     //
-    // The pattern used instead has the same semantics as LazyInitializer for reference types: the
-    // factory may run more than once when threads race, but only a single instance is ever
-    // published and every caller receives that one instance. The Volatile.Read on the fast path
-    // provides the same acquire semantics, so a caller that observes the field also observes the
-    // fully constructed object on weakly ordered architectures.
+    // On the warm path all of these members are a single Volatile.Read, whose acquire semantics
+    // guarantee that a caller observing the field also observes the fully constructed object on
+    // weakly ordered architectures. The cold paths differ:
+    //
+    // This member builds the mutable collection that callers may manipulate through
+    // PropertyMappings, and it runs the (possibly user-supplied) property mapper to do so. Racing
+    // threads must therefore not each run the mapper and build their own copy - the lock, taken
+    // only until the field is first published, makes initialization run exactly once.
+    //
+    // The other lazy members (CreateInstance/CreateList below, and the equivalents in
+    // PropertyMapping and PropertyMappingCollection) have idempotent factories producing values
+    // that are never mutated afterwards, so they keep LazyInitializer's lock-free semantics
+    // instead: when threads race, the factory may run more than once, but only a single result is
+    // ever published (via Interlocked.CompareExchange) and every caller receives that one
+    // instance - a duplicate run is unobservable there.
     private PropertyMappingCollection PropertyMappingsInternal
     {
         get
@@ -210,12 +221,18 @@ private PropertyMappingCollection PropertyMappingsInternal
 
             PropertyMappingCollection createCollection()
             {
-                var properties = propertyMapper(this).ToList();
-                if(properties.FirstOrDefault(m => m.DeclaringClass != this) is {} errorMapping)
-                    throw new InvalidOperationException($"PropertyMapping '{errorMapping.Name}' is already used for another ClassMapping '{errorMapping.DeclaringClass.Name}'.");
-
-                var created = new PropertyMappingCollection(properties);
-                return Interlocked.CompareExchange(ref _mappings, created, null) ?? created;
+                lock (_mappingsLock)
+                {
+                    if (_mappings is { } existing) return existing;
+
+                    var properties = propertyMapper(this).ToList();
+                    if(properties.FirstOrDefault(m => m.DeclaringClass != this) is {} errorMapping)
+                        throw new InvalidOperationException($"PropertyMapping '{errorMapping.Name}' is already used for another ClassMapping '{errorMapping.DeclaringClass.Name}'.");
+
+                    var created = new PropertyMappingCollection(properties);
+                    Volatile.Write(ref _mappings, created);
+                    return created;
+                }
             }
         }
     }

src/Hl7.Fhir.Support.Tests/Introspection/ClassMappingTest.cs

@@ -313,6 +313,33 @@ public void LazyMembersAreSafeToReadConcurrently()
             lists.Should().OnlyHaveUniqueItems();
             lists.Should().AllSatisfy(l => l.Count.Should().Be(0));
         }
+
+        [TestMethod]
+        public void PropertyMapperRunsExactlyOnceUnderConcurrentFirstAccess()
+        {
+            // The property mapper may be user-supplied and builds the mutable PropertyMappings
+            // collection, so racing threads must not each run it and build their own copy: the
+            // first access initializes it exactly once and all threads see that single result.
+            const int threads = 64;
+            var mapperRuns = 0;
+
+            var mapping = new ClassMapping(ModelInspector.Base, "test", typeof(FhirBoolean), _ =>
+            {
+                System.Threading.Interlocked.Increment(ref mapperRuns);
+                // Keep the factory busy for a while so racing threads pile up on the
+                // still-uninitialized field instead of hitting the warm path.
+                System.Threading.Thread.Sleep(50);
+                return [];
+            });
+
+            var collections = new ICollection<PropertyMapping>[threads];
+            var result = Parallel.For(0, threads, new ParallelOptions { MaxDegreeOfParallelism = threads },
+                i => collections[i] = mapping.PropertyMappings);
+
+            result.IsCompleted.Should().BeTrue();
+            mapperRuns.Should().Be(1);
+            collections.Should().AllSatisfy(c => c.Should().BeSameAs(collections[0]));
+        }
     }