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),