From 52b354520625ef53922ba26966744946db74494d Mon Sep 17 00:00:00 2001 From: "Jeremy D. Miller" Date: Thu, 30 Jul 2026 11:26:24 -0500 Subject: [PATCH] fix(postgresql): make the Npgsql type-mapping registry safe for concurrent registration (weasel#406) NpgsqlTypeMapper.Mappings is the documented extension point for consuming code to register custom Npgsql mappings. It was a JasperFx.Core.Cache. Reads were already safe there: the Cache is backed by an immutable ImHashMap, so an enumerating reader sees a snapshot and cannot tear or throw "Collection was modified". A 4.3M-read stress run against concurrent writers produced zero failures. (This corrects the reader-side race speculated about in weasel#402.) Writes were not safe. The indexer setter is a non-atomic read-modify-write over that map, so concurrent registrations clobber each other: 8 threads x 5,000 distinct keys expected 40,000 entries, observed 7,660 -> LOST 32,340 (81%) Silently, too -- a lost registration surfaces much later as a missing or wrong type mapping with nothing pointing back at the registration. Replace the Cache with NpgsqlTypeMappingRegistry, a small ConcurrentDictionary-backed type that keeps the same surface: an indexer plus IEnumerable. Existing registration code and anything enumerating Mappings compiles and behaves unchanged; ConcurrentDictionary fixes the writes and its Values snapshot keeps reads safe. Ordering is not a concern here. GetTypeMapping breaks ties with LastOrDefault, but the Cache enumerated in ImHashMap hash order rather than insertion order anyway, so nothing depended on a defined order -- and weasel#405 removed the only doubly-claimed CLR type and added a guard against another appearing. Tests use their own registry instances rather than the static Mappings: registering tens of thousands of entries globally would persist for the rest of the run, and GetTypeMapping scans linearly, so every other test resolving an unmapped type would slow down. Postgres 802 passed on all four CI matrix legs, Core 21, SQLite 361. Co-Authored-By: Claude Opus 5 (1M context) --- .../NpgsqlTypeMappingRegistryTests.cs | 116 ++++++++++++++++++ src/Weasel.Postgresql/NpgsqlTypeMapping.cs | 61 ++++++++- 2 files changed, 175 insertions(+), 2 deletions(-) create mode 100644 src/Weasel.Postgresql.Tests/NpgsqlTypeMappingRegistryTests.cs diff --git a/src/Weasel.Postgresql.Tests/NpgsqlTypeMappingRegistryTests.cs b/src/Weasel.Postgresql.Tests/NpgsqlTypeMappingRegistryTests.cs new file mode 100644 index 0000000..e4e3fa6 --- /dev/null +++ b/src/Weasel.Postgresql.Tests/NpgsqlTypeMappingRegistryTests.cs @@ -0,0 +1,116 @@ +using System.Data; +using NpgsqlTypes; +using Shouldly; +using Xunit; + +namespace Weasel.Postgresql.Tests; + +/// +/// Exercises its own registry instances rather than the static NpgsqlTypeMapper.Mappings. +/// Registering tens of thousands of mappings into the global one would linger for the rest of +/// the run, and PostgresqlProvider.GetTypeMapping scans it linearly, so every other test that +/// resolves an unmapped type would slow to a crawl. weasel#406. +/// +public class NpgsqlTypeMappingRegistryTests +{ + private static NpgsqlTypeMappingRegistry EmptyRegistry() + { + return new NpgsqlTypeMappingRegistry(new Dictionary()); + } + + private static NpgsqlTypeMapping AnyMapping() + { + return new NpgsqlTypeMapping(NpgsqlDbType.Varchar, DbType.String, "varchar", typeof(string)); + } + + [Fact] + public void concurrent_registrations_are_not_lost() + { + // The regression: backed by a JasperFx Cache, the indexer setter was a non-atomic + // read-modify-write over an ImHashMap, and this landed 7,660 of 40,000 -- an 81% loss, + // silently. weasel#406. + const int threads = 8; + const int perThread = 2000; + var registry = EmptyRegistry(); + + Parallel.For(0, threads, t => + { + for (var i = 0; i < perThread; i++) + { + registry[(NpgsqlDbType)(t * perThread + i)] = AnyMapping(); + } + }); + + registry.Count.ShouldBe(threads * perThread); + } + + [Fact] + public void enumerating_while_registrations_land_neither_throws_nor_tears() + { + const int seeded = 500; + const int added = 5000; + + var registry = EmptyRegistry(); + for (var i = 0; i < seeded; i++) + { + registry[(NpgsqlDbType)i] = AnyMapping(); + } + + // Bounded on both sides: the writer stops after a fixed number of registrations and the + // reader stops with it, so the dictionary cannot grow without limit underneath repeated + // full enumerations. + var writer = Task.Run(() => + { + for (var i = 0; i < added; i++) + { + registry[(NpgsqlDbType)(100_000 + i)] = AnyMapping(); + } + }); + + var passes = 0; + while (!writer.IsCompleted) + { + // Each read must see a coherent snapshot: never fewer than what was there before the + // writer started, and never a null hole. + var seen = registry.ToList(); + seen.Count.ShouldBeGreaterThanOrEqualTo(seeded); + seen.ShouldAllBe(mapping => mapping != null); + passes++; + } + + writer.GetAwaiter().GetResult(); + registry.Count.ShouldBe(seeded + added); + passes.ShouldBeGreaterThan(0); + } + + [Fact] + public void seeded_mappings_are_readable_by_key_and_by_enumeration() + { + var mapping = AnyMapping(); + var registry = new NpgsqlTypeMappingRegistry(new Dictionary + { + { NpgsqlDbType.Varchar, mapping } + }); + + registry[NpgsqlDbType.Varchar].ShouldBeSameAs(mapping); + registry.TryGetValue(NpgsqlDbType.Varchar, out var found).ShouldBeTrue(); + found.ShouldBeSameAs(mapping); + registry.ShouldContain(mapping); + + registry.TryGetValue(NpgsqlDbType.Bigint, out _).ShouldBeFalse(); + } + + [Fact] + public void registering_over_an_existing_key_replaces_it() + { + var registry = EmptyRegistry(); + var first = AnyMapping(); + var second = AnyMapping(); + + registry[NpgsqlDbType.Varchar] = first; + registry[NpgsqlDbType.Varchar] = second; + + registry[NpgsqlDbType.Varchar].ShouldBeSameAs(second); + registry.Count.ShouldBe(1); + } +} diff --git a/src/Weasel.Postgresql/NpgsqlTypeMapping.cs b/src/Weasel.Postgresql/NpgsqlTypeMapping.cs index ae394c7..2dfd06f 100644 --- a/src/Weasel.Postgresql/NpgsqlTypeMapping.cs +++ b/src/Weasel.Postgresql/NpgsqlTypeMapping.cs @@ -1,4 +1,5 @@ using System.Collections; +using System.Collections.Concurrent; using System.Collections.Immutable; using System.Collections.Specialized; using System.Data; @@ -6,7 +7,6 @@ using System.Net.NetworkInformation; using System.Numerics; using System.Text.Json; -using JasperFx.Core; using NetTopologySuite.Geometries; using NpgsqlTypes; @@ -30,6 +30,63 @@ public NpgsqlTypeMapping(NpgsqlDbType? npgsqlDbType, DbType dbType, string? data public Type[] ClrTypes { get; } } +/// +/// Registry of to , keyed for +/// lookup and enumerable by mapping. Consuming code registers custom mappings through the +/// indexer, from any thread. +/// +/// +/// +/// This used to be a JasperFx.Core.Cache. Reads were already safe there — it is +/// backed by an immutable ImHashMap, so an enumerating reader sees a snapshot and +/// cannot tear. Writes were not: the indexer setter is a non-atomic read-modify-write +/// over that map, so concurrent registrations silently clobbered one another. Eight +/// threads registering 5,000 distinct keys each landed 7,660 of 40,000 — an 81% loss, +/// with nothing to indicate a mapping had gone missing. weasel#406. +/// +/// +/// fixes the writes and keeps reads +/// safe: Values hands back a snapshot, so enumeration never throws +/// mid-registration. The surface is deliberately the same as the Cache it +/// replaces — an indexer plus over the mappings — so +/// existing registration code is unaffected. +/// +/// +public class NpgsqlTypeMappingRegistry: IEnumerable +{ + private readonly ConcurrentDictionary _values; + + public NpgsqlTypeMappingRegistry(IDictionary seed) + { + _values = new ConcurrentDictionary(seed); + } + + public NpgsqlTypeMapping this[NpgsqlDbType key] + { + get => _values[key]; + set => _values[key] = value; + } + + public int Count => _values.Count; + + public bool TryGetValue(NpgsqlDbType key, out NpgsqlTypeMapping? value) + { + return _values.TryGetValue(key, out value); + } + + public IEnumerator GetEnumerator() + { + // ConcurrentDictionary.Values is a point-in-time snapshot, so a registration landing + // mid-enumeration cannot disturb this. + return _values.Values.GetEnumerator(); + } + + IEnumerator IEnumerable.GetEnumerator() + { + return GetEnumerator(); + } +} + /// /// Class defining custom NpgsqlType <=> DbType <=> CLR types /// @@ -39,7 +96,7 @@ public NpgsqlTypeMapping(NpgsqlDbType? npgsqlDbType, DbType dbType, string? data /// public class NpgsqlTypeMapper { - public static readonly Cache Mappings = new(new Dictionary + public static readonly NpgsqlTypeMappingRegistry Mappings = new(new Dictionary { // Numeric types {NpgsqlDbType.Smallint,new NpgsqlTypeMapping(NpgsqlDbType.Smallint, DbType.Int16, "smallint", typeof(short), typeof(byte),