From 6a1d1a6288346d4779c36a0c53ffae613c629650 Mon Sep 17 00:00:00 2001 From: Team AppThreat Date: Sun, 25 Oct 2026 01:41:27 +0100 Subject: [PATCH] NodesList: look nodes up under the same monitor as add and remove contains and nodeById read the trove id index without a lock, while add, remove and compact change it under the list's monitor. A lookup that races an add which rehashes the index can miss a node that is in the list. Pass writers add nodes while the pass's parts traverse the graph, so a NodeRef built from an id, such as the OpNodes Assignment wrapper, could come out empty. Its first property read then tried to load the node from disk, where it had never been written, and failed with "unable to read node from disk". atom crashed this way in 9 of 10 usages runs on node-express-realworld-example-app with TypeScript types on. The lookups now take the monitor, and so does access to the label map, which readers also changed through computeIfAbsent. The lists handed out by nodesByLabel are still live and not safe to iterate during concurrent adds; that is left for a separate change. With this, the same project passes 30 of 30 runs with byte-identical usages, and juice-shop usages and reachables take the same time as before. The new NodesListTest case fails without the change. Bump to 3.1.1. Signed-off-by: Team AppThreat --- build.sbt | 2 +- .../java/overflowdb/util/NodesListTest.java | 38 +++++++++++++++++++ .../main/java/overflowdb/util/NodesList.java | 25 +++++++----- 3 files changed, 54 insertions(+), 11 deletions(-) diff --git a/build.sbt b/build.sbt index f4bf8be..62f32ce 100644 --- a/build.sbt +++ b/build.sbt @@ -1,5 +1,5 @@ ThisBuild / organization := "io.appthreat" -ThisBuild / version := "3.1.0" +ThisBuild / version := "3.1.1" ThisBuild / scalaVersion := "3.10.0" // sbt 2 applies bare settings to every subproject, so root-only settings live on the root project diff --git a/core-tests/src/test/java/overflowdb/util/NodesListTest.java b/core-tests/src/test/java/overflowdb/util/NodesListTest.java index 627ecfa..b9e1efe 100644 --- a/core-tests/src/test/java/overflowdb/util/NodesListTest.java +++ b/core-tests/src/test/java/overflowdb/util/NodesListTest.java @@ -6,7 +6,11 @@ import overflowdb.Graph; import java.util.ArrayList; +import java.util.List; import java.util.Vector; +import java.util.concurrent.ThreadLocalRandom; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNull; @@ -183,6 +187,40 @@ public void compactsAutomatically() { + /** A node that has been added is found by id while another thread keeps adding nodes, as when a pass writer adds + * nodes while the pass's parts traverse the graph. */ + @Test + public void lookupsDuringConcurrentAdds() throws Exception { + final int count = 300_000; + final NodesList nl = new NodesList(16); + final NodeRef[] refs = new NodeRef[count]; + for (int i = 0; i < count; i++) refs[i] = createDummyRef(i, "A"); + final AtomicInteger added = new AtomicInteger(); + final AtomicReference failure = new AtomicReference<>(); + final List readers = new ArrayList<>(); + for (int r = 0; r < 3; r++) { + readers.add(new Thread(() -> { + final ThreadLocalRandom random = ThreadLocalRandom.current(); + while (added.get() < count && failure.get() == null) { + final int known = added.get(); + if (known == 0) continue; + final int id = random.nextInt(known); + if (nl.nodeById(id) != refs[id] || !nl.contains(id)) { + failure.compareAndSet(null, "node " + id + " not found after " + known + " adds"); + } + } + })); + } + readers.forEach(Thread::start); + for (int i = 0; i < count && failure.get() == null; i++) { + nl.add(refs[i]); + added.set(i + 1); + } + added.set(count); + for (Thread t : readers) t.join(); + assertNull(failure.get()); + } + private NodeRef createDummyRef(long id, String label) { return new NodeRef(dummyGraph, id) { public String label() { diff --git a/core/src/main/java/overflowdb/util/NodesList.java b/core/src/main/java/overflowdb/util/NodesList.java index a032daa..d10a762 100644 --- a/core/src/main/java/overflowdb/util/NodesList.java +++ b/core/src/main/java/overflowdb/util/NodesList.java @@ -67,16 +67,19 @@ private int tryClaimEmptySlot() { return nextEmptySlot; } - public boolean contains(long id) { + /* Lookups take the same monitor as add/remove/compact. The trove map rehashes in place while a node is added, and + * a lookup racing that rehash can miss a node that is in the list. A pass writer adds nodes while the pass's parts + * traverse the graph, so a NodeRef built from an id (e.g. the OpNodes wrappers) could come out empty and then fail + * with "unable to read node from disk" for a node that was never written to disk. */ + public synchronized boolean contains(long id) { return nodeIndexByNodeId.containsKey(id); } - public Node nodeById(long id) { - if (nodeIndexByNodeId.containsKey(id)) { - return nodes[nodeIndexByNodeId.get(id)]; - } else { - return null; - } + public synchronized Node nodeById(long id) { + final int index = nodeIndexByNodeId.get(id); + // the map's no-entry value (0) is also a valid index + if (index == nodeIndexByNodeId.getNoEntryValue() && !nodeIndexByNodeId.containsKey(id)) return null; + return nodes[index]; } public synchronized void remove(Node node) { @@ -122,12 +125,14 @@ private synchronized void initialiseNodesByLabel(){ } } - public ArrayList nodesByLabel(String label) { + /* The label map is mutated by add/remove and by computeIfAbsent below, so access to it is synchronized too. The + * returned list itself is live: iterating it while another thread adds nodes with that label is still unsafe. */ + public synchronized ArrayList nodesByLabel(String label) { TMap> nodesByLabel = getNodesByLabel(); return nodesByLabel.computeIfAbsent(label, k -> new ArrayList<>()); } - public Set nodeLabels() { + public synchronized Set nodeLabels() { TMap> nodesByLabel = getNodesByLabel(); Set ret = new HashSet<>(nodesByLabel.size()); nodesByLabel.forEach((key, value) -> { @@ -201,7 +206,7 @@ protected int _elementDataSize() { return nodes.length; } - public int cardinality(String label) { + public synchronized int cardinality(String label) { TMap> nodesByLabel = getNodesByLabel(); if (nodesByLabel.containsKey(label)) return nodesByLabel.get(label).size();