From e1e330a454202074cab35b075baf92c5a648f0ad Mon Sep 17 00:00:00 2001 From: Brett Wooldridge Date: Mon, 7 Sep 2026 13:07:53 +0900 Subject: [PATCH] fix: one index instance and one map wrapper per name, and a drop that cannot run twice The first read of a non-unique index after a store reopens runs the lazy layout migration in SingleFieldIndex, which drops the legacy map. That migration is guarded per SingleFieldIndex instance, but ComparableIndexer.findNitriteIndex created instances with an unsynchronized check-then-act, so several threads arriving at once each got their own instance and each ran the migration. NitriteMVMap.drop() likewise tested its dropped flag and then ignored the result of its compare-and-set, and NitriteMVStore.openMap handed out one wrapper per caller. Once the first thread had removed the map, MVMap.getName() answered null for the rest, and NitriteMVStore.removeMap(null) died in ConcurrentHashMap.remove: NullPointerException: Cannot invoke "Object.hashCode()" because "key" is null at NitriteMVStore.removeMap at NitriteMVMap.drop at SingleFieldIndex.migrateLegacyIndex A second shape of the same race is a NullPointerException from Attributes.set via NitriteMap.updateLastModifiedTime, which read getName() several times. Seen on a production system on the first multi-threaded lookup after every restart, since every close left an empty map under the legacy name (fixed by #1295) and every start had to drop it again. - ComparableIndexer registers the index with computeIfAbsent, so the per-instance guard in SingleFieldIndex is the guard. - NitriteMVStore.openMap and openRTree register the wrapper with computeIfAbsent, so every holder shares one dropped flag; removeMap ignores a null name and does not open (and thereby create) a map that is no longer in the store. - NitriteMVMap captures its name at open and acts only when its compare-and-set succeeds, so drop() and close() run once and never ask MVMap for a name it no longer has. - NitriteMap.updateLastModifiedTime reads the name once. Co-Authored-By: Claude Fable 5.1 --- .../dizitart/no2/mvstore/NitriteMVMap.java | 22 ++++++----- .../dizitart/no2/mvstore/NitriteMVStore.java | 37 ++++++++++--------- .../dizitart/no2/index/ComparableIndexer.java | 18 +++------ .../org/dizitart/no2/store/NitriteMap.java | 11 +++--- 4 files changed, 44 insertions(+), 44 deletions(-) diff --git a/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/NitriteMVMap.java b/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/NitriteMVMap.java index 0a2a195d5..be11bf442 100644 --- a/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/NitriteMVMap.java +++ b/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/NitriteMVMap.java @@ -46,6 +46,9 @@ class NitriteMVMap implements NitriteMap { private final MVMap mvMap; + // captured at open: MVMap.getName() answers null once the map is removed from the store, + // which is exactly when the registry and the catalog still have to be told the name + private final String name; private final NitriteStore nitriteStore; private final MVStore mvStore; private final AtomicBoolean droppedFlag; @@ -54,6 +57,7 @@ class NitriteMVMap implements NitriteMap { NitriteMVMap(final MVMap mvMap, final NitriteStore nitriteStore) { this.mvMap = mvMap; + this.name = mvMap.getName(); this.nitriteStore = nitriteStore; this.mvStore = mvMap.getStore(); this.closedFlag = new AtomicBoolean(false); @@ -89,7 +93,7 @@ public void clear() { @Override public String getName() { - return mvMap.getName(); + return name; } @Override @@ -254,15 +258,16 @@ public boolean isEmpty() { @Override public void drop() { - if (!droppedFlag.get()) { - droppedFlag.compareAndSet(false, true); - closedFlag.compareAndSet(false, true); + // the compare-and-set is the guard: two threads that both saw the flag clear must not + // both remove the map + if (droppedFlag.compareAndSet(false, true)) { + closedFlag.set(true); releaseVersionUsages(); final MVStore.TxCounter txCounter = mvStore.registerVersionUsage(); try { - nitriteStore.closeMap(mvMap.getName()); - nitriteStore.removeMap(mvMap.getName()); + nitriteStore.closeMap(name); + nitriteStore.removeMap(name); } finally { mvStore.deregisterVersionUsage(txCounter); } @@ -276,10 +281,9 @@ public boolean isDropped() { @Override public void close() { - if (!closedFlag.get() && !droppedFlag.get()) { - closedFlag.compareAndSet(false, true); + if (!droppedFlag.get() && closedFlag.compareAndSet(false, true)) { releaseVersionUsages(); - nitriteStore.closeMap(mvMap.getName()); + nitriteStore.closeMap(name); } } diff --git a/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/NitriteMVStore.java b/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/NitriteMVStore.java index 19aa21cf0..061b630af 100644 --- a/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/NitriteMVStore.java +++ b/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/NitriteMVStore.java @@ -147,14 +147,12 @@ public boolean hasMap(String mapName) { @Override @SuppressWarnings("unchecked") public NitriteMap openMap(String mapName, Class keyType, Class valueType) { - if (nitriteMapRegistry.containsKey(mapName)) { - return (NitriteMVMap) nitriteMapRegistry.get(mapName); - } - - MVMap mvMap = openMVMap(mapName, null); - NitriteMVMap nitriteMVMap = new NitriteMVMap<>(mvMap, this); - nitriteMapRegistry.put(mapName, nitriteMVMap); - return nitriteMVMap; + // one wrapper per map, however many threads open it at once, so a drop() or close() + // through any holder is the drop or close every holder sees + return (NitriteMVMap) nitriteMapRegistry.computeIfAbsent(mapName, name -> { + MVMap mvMap = openMVMap(name, null); + return new NitriteMVMap<>(mvMap, this); + }); } @Override @@ -173,8 +171,15 @@ public void closeRTree(String rTreeName) { @Override public void removeMap(String name) { - MVMap mvMap = openMVMap(name, null); - mvStore.removeMap(mvMap); + if (StringUtils.isNullOrEmpty(name)) { + return; + } + // a map another thread has already removed is simply gone; openMVMap would create an + // empty map of that name only to remove it again + if (mvStore.hasMap(name)) { + MVMap mvMap = openMVMap(name, null); + mvStore.removeMap(mvMap); + } getCatalog().remove(name); nitriteMapRegistry.remove(name); } @@ -191,14 +196,10 @@ public void removeRTree(String rTreeName) { @Override @SuppressWarnings({"unchecked", "rawtypes"}) public NitriteRTree openRTree(String mapName, Class keyType, Class valueType) { - if (nitriteRTreeMapRegistry.containsKey(mapName)) { - return (NitriteMVRTreeMap) nitriteRTreeMapRegistry.get(mapName); - } - - MVRTreeMap map = (MVRTreeMap) openMVMap(mapName, new MVRTreeMap.Builder<>()); - NitriteMVRTreeMap nitriteMVRTreeMap = new NitriteMVRTreeMap(map, this); - nitriteRTreeMapRegistry.put(mapName, nitriteMVRTreeMap); - return nitriteMVRTreeMap; + return (NitriteMVRTreeMap) nitriteRTreeMapRegistry.computeIfAbsent(mapName, name -> { + MVRTreeMap map = (MVRTreeMap) openMVMap(name, new MVRTreeMap.Builder<>()); + return new NitriteMVRTreeMap(map, this); + }); } @Override diff --git a/nitrite/src/main/java/org/dizitart/no2/index/ComparableIndexer.java b/nitrite/src/main/java/org/dizitart/no2/index/ComparableIndexer.java index 45a03d3c6..273e7ce3d 100644 --- a/nitrite/src/main/java/org/dizitart/no2/index/ComparableIndexer.java +++ b/nitrite/src/main/java/org/dizitart/no2/index/ComparableIndexer.java @@ -105,17 +105,11 @@ private NitriteIndex findNitriteIndex(IndexDescriptor indexDescriptor, NitriteCo throw new IndexingException("Index descriptor cannot be null"); } - if (indexRegistry.containsKey(indexDescriptor)) { - return indexRegistry.get(indexDescriptor); - } - - NitriteIndex nitriteIndex; - if (indexDescriptor.isCompoundIndex()) { - nitriteIndex = new CompoundIndex(indexDescriptor, nitriteConfig.getNitriteStore()); - } else { - nitriteIndex = new SingleFieldIndex(indexDescriptor, nitriteConfig.getNitriteStore()); - } - indexRegistry.put(indexDescriptor, nitriteIndex); - return nitriteIndex; + // One instance per descriptor, however many threads ask for it at once. The lazy layout + // migration in SingleFieldIndex is guarded per instance, so two instances for the same + // index would each migrate and drop the legacy map, and the second drop fails. + return indexRegistry.computeIfAbsent(indexDescriptor, descriptor -> descriptor.isCompoundIndex() + ? new CompoundIndex(descriptor, nitriteConfig.getNitriteStore()) + : new SingleFieldIndex(descriptor, nitriteConfig.getNitriteStore())); } } diff --git a/nitrite/src/main/java/org/dizitart/no2/store/NitriteMap.java b/nitrite/src/main/java/org/dizitart/no2/store/NitriteMap.java index 07b26ce27..1db4a9aca 100644 --- a/nitrite/src/main/java/org/dizitart/no2/store/NitriteMap.java +++ b/nitrite/src/main/java/org/dizitart/no2/store/NitriteMap.java @@ -243,15 +243,16 @@ default void setAttributes(Attributes attributes) { */ default void updateLastModifiedTime() { if (!isDropped()) { - if (isNullOrEmpty(getName()) - || META_MAP_NAME.equals(getName())) return; + // read once: an adapter may answer null as soon as the map is removed from the store + String name = getName(); + if (isNullOrEmpty(name) || META_MAP_NAME.equals(name)) return; NitriteMap metaMap = getStore().openMap(META_MAP_NAME, String.class, Attributes.class); if (metaMap != null) { - Attributes attributes = metaMap.get(getName()); + Attributes attributes = metaMap.get(name); if (attributes == null) { - attributes = new Attributes(getName()); - metaMap.put(getName(), attributes); + attributes = new Attributes(name); + metaMap.put(name, attributes); } attributes.set(Attributes.LAST_MODIFIED_TIME, Long.toString(System.currentTimeMillis())); }