diff --git a/CHANGELOG.md b/CHANGELOG.md index acba432d..22da9cf0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,10 +27,11 @@ Eleven changes from [@brettwooldridge](https://github.com/brettwooldridge), most - **`clear()` and `dropIndex()` reach every layout map an index occupies** ([#1295](https://github.com/nitrite/nitrite-java/pull/1295)) - `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. -- **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)) +- **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), [#1315](https://github.com/nitrite/nitrite-java/issues/1315)) - 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`. - 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. - 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. + - Reported against 5.3.0 in [#1315](https://github.com/nitrite/nitrite-java/issues/1315): four threads making the first find on a reopened file database failed 21 of 400 finds on 5.3.0 and none with these changes. The reporter's reproduction is now `Issue1315Test`. - **Concurrent first reads of a map on an in-memory MVStore no longer fail inside H2** ([#1311](https://github.com/nitrite/nitrite-java/pull/1311)) - H2's `ObjectDataType` picks the delegate that compares serialized keys on first use, through an unsynchronized field, and `SerializedObjectType.compare` checks delegates by identity. Threads making a map's first key comparison together could each install their own and fail with `UnsupportedOperationException: Can not compare`. The code is the same in h2 2.4.240 and 2.5.250. diff --git a/nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/integration/collection/Issue1315Test.java b/nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/integration/collection/Issue1315Test.java new file mode 100644 index 00000000..3dafe7c8 --- /dev/null +++ b/nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/integration/collection/Issue1315Test.java @@ -0,0 +1,88 @@ +/* + * Copyright (c) 2017-2021 Nitrite author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package org.dizitart.no2.integration.collection; + +import org.dizitart.no2.Nitrite; +import org.dizitart.no2.collection.Document; +import org.dizitart.no2.collection.NitriteCollection; +import org.dizitart.no2.filters.FluentFilter; +import org.dizitart.no2.index.IndexOptions; +import org.dizitart.no2.index.IndexType; +import org.dizitart.no2.mvstore.MVStoreModule; +import org.junit.Test; + +import java.io.File; +import java.util.List; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.CyclicBarrier; + +import static org.junit.Assert.assertEquals; + +/** + * Regression test for Issue 1315. + *

+ * On 5.3.0 every close left an empty legacy index map behind, and the first finds on a + * reopened file database raced to migrate and drop it, failing with a + * {@code NullPointerException} in {@code NitriteMVStore.removeMap}. Fixed by #1295 and #1309. + */ +public class Issue1315Test { + + private static Nitrite open(File file) { + return Nitrite.builder() + .loadModule(MVStoreModule.withConfig().filePath(file.getPath()).build()) + .openOrCreate(); + } + + @Test + public void firstUseOfANonUniqueIndexFromSeveralThreads() throws Exception { + File file = File.createTempFile("nitrite-legacy-race", ".db"); + file.delete(); + try { + Nitrite db = open(file); + NitriteCollection items = db.getCollection("items"); + items.createIndex(IndexOptions.indexOptions(IndexType.NON_UNIQUE), "state"); + items.insert(Document.createDocument("state", "new")); + db.close(); + + int threads = 4, openings = 100; + List failures = new CopyOnWriteArrayList<>(); + for (int opening = 0; opening < openings; opening++) { + Nitrite reopened = open(file); + NitriteCollection collection = reopened.getCollection("items"); + CyclicBarrier start = new CyclicBarrier(threads); + Thread[] workers = new Thread[threads]; + for (int i = 0; i < threads; i++) { + workers[i] = new Thread(() -> { + try { + start.await(); + assertEquals(1, collection.find(FluentFilter.where("state").eq("new")).toList().size()); + } catch (Throwable t) { + failures.add(t); + } + }); + workers[i].start(); + } + for (Thread worker : workers) worker.join(); + reopened.close(); + } + assertEquals(failures.toString(), 0, failures.size()); + } finally { + file.delete(); + } + } +}