Repository navigation
Fix lazy-singleton double init and publish registries as immutable snapshots (#16) - #21
Merged
Merged
Conversation
…apshots (IABTechLab#16) ExtractorRegistryFactory, TransformerRegistryFactory, LocalCacheRegistryFactory and ExperimentManagerFactory checked a non-volatile field outside the lock and never re-checked it inside, so concurrent first callers each built an instance and the last one won. A component holding an earlier instance then read a registry the setup task never filled. Make the fields volatile and re-check inside the synchronized block, matching AWSServiceClientFactory. The extractor and transformer registries are filled by ModelFeatureOperatorRegistrySetupTask on a scheduler thread after init() returns, while request threads already read them through a plain HashMap. Registry now holds its records as an unmodifiable snapshot in a volatile field. register() is synchronized, copies the current snapshot, adds the entry and publishes the copy, so readers always see a complete map without a lock. get() does one lookup on a hit instead of containsKey plus get; containsKey runs only on a miss, so a name registered with a null value still returns null as before. This avoids the per-lookup cost of ConcurrentHashMap on the request path. Behavior change for code outside this repo: getRecords() used to return the live backing map; it now returns the current unmodifiable snapshot, as its javadoc already said, so mutating it throws UnsupportedOperationException and a kept reference does not see later registrations. The constructor copies the map it is given. Nothing in this repo relied on either. Snapshots use Collections.unmodifiableMap over a private HashMap rather than Map.copyOf, which rejects null keys and values and randomizes iteration order per JVM. Add LazySingletonConcurrencyTest for each lazy getter and RegistryTest for the snapshot contract and for register() racing get() and iteration. ExtractorRegistryTest read the map it passed to the constructor; it now asserts through the registry. Fixes IABTechLab#16
mpc00620
force-pushed
the
fix-16-singleton-init
branch
from
October 20, 2026 21:41
8585bd1 to
5d1fb97
Compare
kchiang6997
approved these changes
Oct 27, 2026
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.
Fixes #16.
Two fixes:
Re-check lazy singletons under the lock.
ExtractorRegistryFactory,TransformerRegistryFactory,LocalCacheRegistryFactoryandExperimentManagerFactorychecked a non-volatile field outside the lock and never re-checked it inside, so concurrent first callers each built an instance and the last one won. A component holding an earlier instance then read a registry the setup task never filled. The fields are nowvolatileand re-checked inside thesynchronizedblock, the same patternAWSServiceClientFactoryalready uses.Publish registries as immutable snapshots. The extractor and transformer registries are filled by
ModelFeatureOperatorRegistrySetupTaskon a scheduler thread up to a second afterinit()returns, while request threads already read them through a plainHashMap.ConcurrentHashMapwould fix that but adds a few ns to every lookup on the request path, so insteadRegistrykeeps its records as an unmodifiable snapshot in avolatilefield.register()issynchronized, copies the snapshot, adds the entry and publishes the copy.get()does one lookup instead ofcontainsKeyplusget(containsKeyruns only on a miss, so a name registered with a null value still returns null). The factories are back onHashMap.No public or protected signature changes. One behavior change for code outside this repo:
getRecords()used to return the live backing map. It now returns the current unmodifiable snapshot, as its javadoc already said, so mutating it throwsUnsupportedOperationExceptionand a kept reference does not see later registrations. The constructor copies the map it is given. Nothing in this repo relied on either. The snapshot usesCollections.unmodifiableMapover a privateHashMaprather thanMap.copyOf, becauseMap.copyOfrejects nulls and randomizes iteration order per JVM.Tests
LazySingletonConcurrencyTestreleases 16 threads together on a fresh factory, 50 rounds per getter. On the old code it failed in 174-200 of 200 runs per getter. With the fix it passed 20 of 20 runs.RegistryTest(new) checks the snapshot contract and runsregister()while other threads callget()and iterategetRecords(). Against the oldRegistry, 5 of its 8 tests fail. Four are deterministic:getRecords()is mutable, an earlier snapshot changes afterregister(), the constructor shares the caller's map, and iteratinggetRecords()while registering throwsConcurrentModificationException. The threaded test also fails, withConcurrentModificationException, in every run we observed. All 8 pass with the fix, 10 of 10 repeated runs. The visibility race itself (a reader seeing a stale or partly built map) cannot be reproduced deterministically. The fix relies on publishing a map that is never mutated through avolatilefield.ExtractorRegistryTestread the map it passed to the constructor to checkregister(). It now asserts through the registry../gradlew clean buildis green: 554 tests, checkstyle and spotbugs clean apart from existing warnings.Benchmark (JMH 1.37, JDK 17, Graviton2 aarch64)
Registry.get, 1 thread (ns)Registry.get, 8 threads (ns)evaluate()map input, 1 model (us)evaluate()map input, 2 models (us)Per lookup the snapshot is about 30% cheaper than main. For
evaluate()all three are within noise of each other. For 2 models the snapshot's point estimate is slightly above main in two separate runs (+1.5% and +3.8%), but both confidence intervals include zero. The micro saving is under 2% of an evaluate, which is smaller than the run-to-run spread on this host.