Skip to content

Commit 7bdfdef

Browse files
anidotnetclaude
andcommitted
test and changelog for the concurrent legacy-index drop fix
Two regression tests in NitriteMVStoreTest, both failing on main: - a drop() through a wrapper whose map another holder already dropped reproduces the production NullPointerException in NitriteMVStore.removeMap exactly - sixteen threads making the first read of a legacy-layout index at once, fifty rounds, must all see the row and leave the legacy map gone Changelog entry under Unreleased, which becomes 5.4.0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 51f128e commit 7bdfdef

2 files changed

Lines changed: 95 additions & 1 deletion

File tree

‎CHANGELOG.md‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
## Unreleased
22

3-
Ten changes from [@brettwooldridge](https://github.com/brettwooldridge), most of them found on a production system. Four are data-integrity fixes, three of which can end with a store that will not reopen or a query that quietly returns the wrong rows.
3+
Eleven changes from [@brettwooldridge](https://github.com/brettwooldridge), most of them found on a production system. Four are data-integrity fixes, three of which can end with a store that will not reopen or a query that quietly returns the wrong rows.
44

55
### Issue Fixes
66

@@ -27,6 +27,11 @@ Ten changes from [@brettwooldridge](https://github.com/brettwooldridge), most of
2727
- **`clear()` and `dropIndex()` reach every layout map an index occupies** ([#1295](https://github.com/nitrite/nitrite-java/pull/1295))
2828
- `IndexManager.close()`, `clearAll()` and `dropIndexDescriptor()` acted only on the map name recorded in `IndexMeta`, which is the classic one. That already missed the composite map of a non-unique index: after `collection.clear()` its rows survived, and a query on that index returned the ids of the cleared documents alongside the new ones - two live documents, four results. It also broke `ChangeIdField`, whose `createIndex` found the previous index map still populated and rebuilt over it.
2929

30+
- **The first concurrent read of an index after a restart no longer fails dropping its legacy map** ([#1309](https://github.com/nitrite/nitrite-java/pull/1309))
31+
- The first read of an index still in a legacy layout migrates it and drops the legacy map, once per index instance. `ComparableIndexer` created those instances with an unsynchronized check-then-act, so threads arriving together could each get an instance of their own and each run the migration. On MVStore the second drop asked `MVMap.getName()` for a map that was already gone, got `null`, and failed with `NullPointerException` in `NitriteMVStore.removeMap`; the same race also threw from `Attributes.set` through `NitriteMap.updateLastModifiedTime`.
32+
- Observed on a production system on the first multi-threaded lookup after every restart, because every close before [#1295](https://github.com/nitrite/nitrite-java/pull/1295) left an empty map under the legacy name for the next start to drop.
33+
- The indexer and the MVStore map and R-tree registries now create their entries with `computeIfAbsent`, so there is one index instance and one map wrapper per name. `NitriteMVMap` keeps the name it was opened with and acts only when its compare-and-set wins, so `drop()` and `close()` run once; `removeMap` ignores a null name and no longer creates an empty map only to remove it.
34+
3035
- **A unique index no longer rejects a document over a key that document already holds** ([#1295](https://github.com/nitrite/nitrite-java/pull/1295))
3136
- `addNitriteIds` treated any existing id under the key as a violation, so it counted the writer's own id against it. Another document under the key is a violation; the same document again is not - which is what a unique index over an array field with a repeated element does, and what an index rebuild or a replayed write does.
3237

‎nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/mvstore/NitriteMVStoreTest.java‎

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,21 +23,38 @@
2323
import static org.junit.Assert.assertNull;
2424
import static org.junit.Assert.assertThrows;
2525
import static org.junit.Assert.assertTrue;
26+
import static org.dizitart.no2.common.util.IndexUtils.deriveIndexMapName;
27+
import static org.dizitart.no2.filters.FluentFilter.where;
2628
import static org.mockito.ArgumentMatchers.anyInt;
2729
import static org.mockito.Mockito.doAnswer;
30+
import static org.mockito.Mockito.doReturn;
2831
import static org.mockito.Mockito.mock;
2932

3033
import java.lang.reflect.Field;
3134
import java.nio.file.Files;
3235
import java.nio.file.Path;
36+
import java.util.ArrayList;
37+
import java.util.Collections;
3338
import java.util.Iterator;
39+
import java.util.List;
40+
import java.util.Set;
3441
import java.util.concurrent.CountDownLatch;
3542
import java.util.concurrent.ExecutorService;
3643
import java.util.concurrent.Executors;
3744
import java.util.concurrent.Future;
3845
import java.util.concurrent.TimeUnit;
3946

47+
import org.dizitart.no2.NitriteConfig;
48+
import org.dizitart.no2.collection.FindPlan;
49+
import org.dizitart.no2.collection.NitriteId;
50+
import org.dizitart.no2.common.DBValue;
51+
import org.dizitart.no2.common.Fields;
4052
import org.dizitart.no2.exceptions.NitriteIOException;
53+
import org.dizitart.no2.filters.ComparableFilter;
54+
import org.dizitart.no2.filters.IndexScanFilter;
55+
import org.dizitart.no2.index.IndexDescriptor;
56+
import org.dizitart.no2.index.IndexType;
57+
import org.dizitart.no2.index.NonUniqueIndexer;
4158
import org.dizitart.no2.store.NitriteMap;
4259
import org.h2.mvstore.MVStore;
4360
import org.junit.Test;
@@ -167,6 +184,78 @@ public void testCompactingClosesAreSerialized() throws Exception {
167184
}
168185
}
169186

187+
@Test
188+
public void testDropThroughAWrapperWhoseMapIsAlreadyGone() {
189+
// MVMap.getName() answers null once the map is removed, so a second holder's drop()
190+
// used to hand removeMap a null name and fail
191+
final NitriteMVStore store = openInMemoryStore();
192+
try {
193+
NitriteMap<String, String> stale = store.openMap("m", String.class, String.class);
194+
stale.close();
195+
NitriteMap<String, String> current = store.openMap("m", String.class, String.class);
196+
current.drop();
197+
198+
stale.drop();
199+
assertEquals("m", stale.getName());
200+
assertFalse(store.hasMap("m"));
201+
} finally {
202+
store.close();
203+
}
204+
}
205+
206+
@Test
207+
public void testConcurrentFirstReadOfALegacyIndexMigratesOnce() throws Exception {
208+
// the first multi-threaded read after a reopen used to build one index instance per
209+
// thread, each migrating and dropping the legacy map; the second drop failed
210+
final int threads = 16;
211+
final ExecutorService executorService = Executors.newFixedThreadPool(threads);
212+
try {
213+
for (int round = 0; round < 50; round++) {
214+
final NitriteMVStore store = openInMemoryStore();
215+
try {
216+
final IndexDescriptor desc = new IndexDescriptor(IndexType.NON_UNIQUE,
217+
Fields.withNames("a"), "c");
218+
final NitriteId id = NitriteId.createId(1L);
219+
store.openMap(deriveIndexMapName(desc), DBValue.class, ArrayList.class)
220+
.put(new DBValue("k"), new ArrayList<>(Collections.singletonList(id)));
221+
222+
final NitriteConfig config = mock(NitriteConfig.class);
223+
doReturn(store).when(config).getNitriteStore();
224+
final FindPlan plan = new FindPlan();
225+
plan.setIndexDescriptor(desc);
226+
plan.setIndexScanFilter(new IndexScanFilter(Collections.singletonList(
227+
(ComparableFilter) where("a").eq("k"))));
228+
229+
final NonUniqueIndexer indexer = new NonUniqueIndexer();
230+
final CountDownLatch start = new CountDownLatch(1);
231+
final List<Future<Set<NitriteId>>> reads = new ArrayList<>();
232+
for (int t = 0; t < threads; t++) {
233+
reads.add(executorService.submit(() -> {
234+
start.await();
235+
return indexer.findByFilter(plan, config);
236+
}));
237+
}
238+
start.countDown();
239+
for (Future<Set<NitriteId>> read : reads) {
240+
assertEquals(Collections.singleton(id), read.get(5, TimeUnit.SECONDS));
241+
}
242+
assertFalse(store.hasMap(deriveIndexMapName(desc)));
243+
} finally {
244+
store.close();
245+
}
246+
}
247+
} finally {
248+
executorService.shutdownNow();
249+
}
250+
}
251+
252+
private NitriteMVStore openInMemoryStore() {
253+
final NitriteMVStore store = new NitriteMVStore();
254+
store.setStoreConfig(new MVStoreConfig());
255+
store.openOrCreate();
256+
return store;
257+
}
258+
170259
private NitriteMVStore createCompactingStore(final MVStore mvStore) throws Exception {
171260
final NitriteMVStore nitriteMVStore = new NitriteMVStore();
172261
final MVStoreConfig config = new MVStoreConfig();

0 commit comments

Comments
 (0)