fix(postgresql): make the Npgsql type-mapping registry safe for concurrent registration (weasel#406) - #412
Merged
Conversation
…rrent 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<NpgsqlTypeMapping>. 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) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #406.
NpgsqlTypeMapper.Mappingsis the documented extension point for consuming code to register custom Npgsql mappings:It was a
JasperFx.Core.Cache.Reads were already fine — writes were not
Reads:
Cacheis backed by an immutableImHashMap, so an enumerating reader sees a snapshot and cannot tear or throwCollection was modified. A 4.3M-read stress run against concurrent writers produced zero failures.Writes: the indexer setter is a non-atomic read-modify-write over that map, so concurrent registrations clobber each other:
Silently. A lost registration surfaces much later as a missing or wrong type mapping, with nothing pointing back at the registration that vanished.
Change
Replace the
CachewithNpgsqlTypeMappingRegistry— a smallConcurrentDictionary-backed type that keeps the same surface: an indexer plusIEnumerable<NpgsqlTypeMapping>. Existing registration code and anything enumeratingMappingscompiles and behaves unchanged.ConcurrentDictionaryfixes the writes, and itsValuessnapshot keeps reads safe.Ordering is not a concern
GetTypeMappingbreaks ties withLastOrDefault, so it's worth being explicit that this doesn't disturb resolution:Cacheenumerated in ImHashMap hash order, not insertion order, so nothing depended on a defined order to begin with.IPNetwork) and addedno_clr_type_is_claimed_by_more_than_one_mappingto stop another appearing.With no CLR type claimed twice, which element
LastOrDefaultlands on is immaterial.Tests
Four new tests, exercising their own registry instances rather than the static
Mappings— registering tens of thousands of entries globally would persist for the rest of the run, andGetTypeMappingscans linearly, so every other test resolving an unmapped type would slow to a crawl. (Measured: a registry grown to ~16M entries dropped a reader to 506 reads in 8 seconds.)concurrent_registrations_are_not_lost— 8 × 2,000; would have lost ~80% beforeenumerating_while_registrations_land_neither_throws_nor_tearsseeded_mappings_are_readable_by_key_and_by_enumerationregistering_over_an_existing_key_replaces_itVerification
Postgres 802 passed on all four matrix legs (net9.0/net10.0 × case-sensitive true/false), Core 21, SQLite 361.
🤖 Generated with Claude Code