DataStoreManager.getDatastore reads an unsynchronised HashMap that register mutates under a lock, so a concurrent read can return null for a type that is registered.
What was observed
DataStoreManagerTest$ConcurrentReadWriteTests.concurrentReadWriteShouldBeSafe failed twice locally during Phase 17 wave-0 work, both times while another Maven build shared the host, and passed on every re-run and in CI. The failing assertion is the one that exists for exactly this:
[a concurrent read of an already-registered type must never return null]
init-type-0 is registered before any thread starts, and one of the twenty concurrent readers still got null back for it.
Why it is not only a flaky test
In manager/DataStoreManager:
dataMap is a plain java.util.HashMap;register(DataStore) and unregister(DataStore) are static synchronized and mutate it;getDatastore(String) is not synchronised and reads it — three times per call, on the "json" fallback path.
A read that runs while a writer is rehashing that map is unsynchronised access to a structure being restructured. Returning null for a key that is present is the mildest outcome of that race; the reader also has no happens-before edge to the writer's put, so a newly registered store can be invisible to it indefinitely. The test is doing its job — it catches a real race when the machine is loaded enough to widen the window.
Suggested direction (not prescriptive)
Either make dataMap a ConcurrentHashMap and drop the synchronized on the writers, or synchronise the reader on the same monitor. The first keeps reads lock-free; the second is a two-line change. Whichever is chosen, getDatastore's three reads of the map should become one lookup so the "json" fallback cannot observe two different states of the map.
Scope
Not fixed in the pull request that found it — that one is scoped to /upm uninstall (#503, #501) and does not touch this class, whose last change was in phase 8. Filed for wave 4.
Reproducing
Run the framework test suite while the machine is loaded (a second Maven build is enough):
The failure is intermittent; several consecutive runs on an idle machine pass.
DataStoreManager.getDatastorereads an unsynchronisedHashMapthatregistermutates under a lock, so a concurrent read can returnnullfor a type that is registered.What was observed
DataStoreManagerTest$ConcurrentReadWriteTests.concurrentReadWriteShouldBeSafefailed twice locally during Phase 17 wave-0 work, both times while another Maven build shared the host, and passed on every re-run and in CI. The failing assertion is the one that exists for exactly this:init-type-0is registered before any thread starts, and one of the twenty concurrent readers still gotnullback for it.Why it is not only a flaky test
In
manager/DataStoreManager:dataMapis a plainjava.util.HashMap;register(DataStore)andunregister(DataStore)arestatic synchronizedand mutate it;getDatastore(String)is not synchronised and reads it — three times per call, on the"json"fallback path.A read that runs while a writer is rehashing that map is unsynchronised access to a structure being restructured. Returning
nullfor a key that is present is the mildest outcome of that race; the reader also has no happens-before edge to the writer'sput, so a newly registered store can be invisible to it indefinitely. The test is doing its job — it catches a real race when the machine is loaded enough to widen the window.Suggested direction (not prescriptive)
Either make
dataMapaConcurrentHashMapand drop thesynchronizedon the writers, or synchronise the reader on the same monitor. The first keeps reads lock-free; the second is a two-line change. Whichever is chosen,getDatastore's three reads of the map should become one lookup so the"json"fallback cannot observe two different states of the map.Scope
Not fixed in the pull request that found it — that one is scoped to
/upm uninstall(#503, #501) and does not touch this class, whose last change was in phase 8. Filed for wave 4.Reproducing
Run the framework test suite while the machine is loaded (a second Maven build is enough):
The failure is intermittent; several consecutive runs on an idle machine pass.