BlockStateOctreeInterface can answer from the wrong chunk when two threads use it
BlockStateOctreeInterface.get0 caches the last chunk in three fields (chunk, prevChunkX, prevChunkZ), and its callers only hold the read lock, so several threads can be in it at once. One thread can then see another's new prevChunkX/prevChunkZ while chunk is still the old chunk, take it as a hit, and answer isSolid from the wrong chunk. The return also reads this.chunk again, so it can pick up a chunk another thread has just stored.
Two threads do overlap there:
- When the solver misses its tick,
onTick0cancels it (L540) andtick()solves on the game thread (L608), while the cancelled solver keeps going until its nextThread.interrupted()check (L676). - On a re-plan, the new
ElytraBehaviorstarts solving on the same context (L424) while the old one's solver is still shutting down.
The last race that came up in #5117 turned out not to be one, so here's a reproduction: get0 from 1.21.4 with the chunk table stubbed (one chunk all solid, one all air), and two threads asking about the blocks either side of the border. java ChunkCacheRace.java on JDK 21, about 3 seconds (the counts vary a lot between runs):
round 1: current 17,792 wrong, proposed 0 wrong (of 40,000,000 each)
round 2: current 2,766 wrong, proposed 0 wrong (of 40,000,000 each)
round 3: current 5,687 wrong, proposed 0 wrong (of 40,000,000 each)import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.atomic.AtomicLong;
// Run with: java ChunkCacheRace.java
// get0 is BlockStateOctreeInterface.get0 from 1.21.4 without its y range check, with the chunk table stubbed:
// chunk (0, 0) is all solid and chunk (1, 0) all air, so every answer has a known right value.
// Two threads share one cache and ask about the blocks either side of the border between them.
public class ChunkCacheRace {
record StubChunk(boolean solid) {
boolean isSolid(int x, int y, int z) { return solid; }
}
static final ConcurrentHashMap<Long, StubChunk> TABLE = new ConcurrentHashMap<>();
static {
TABLE.put(0L, new StubChunk(true));
TABLE.put(1L, new StubChunk(false));
}
static StubChunk getChunkOrDefault(int chunkX, int chunkZ) {
return TABLE.get((long) chunkX);
}
interface Cache { boolean get0(int x, int y, int z); }
// As in 1.21.4.
static final class Current implements Cache {
StubChunk chunk;
private int prevChunkX = Integer.MAX_VALUE;
private int prevChunkZ = Integer.MAX_VALUE;
public boolean get0(int x, int y, int z) {
final int chunkX = x >> 4;
final int chunkZ = z >> 4;
if (this.chunk == null | ((chunkX ^ this.prevChunkX) | (chunkZ ^ this.prevChunkZ)) != 0) {
this.prevChunkX = chunkX;
this.prevChunkZ = chunkZ;
this.chunk = getChunkOrDefault(chunkX, chunkZ);
}
return this.chunk.isSolid(x & 0xF, y, z & 0xF);
}
}
// Proposed: the position and the chunk as one immutable value, read once.
static final class Proposed implements Cache {
private record CachedChunk(int chunkX, int chunkZ, StubChunk chunk) {}
private volatile CachedChunk cached;
public boolean get0(int x, int y, int z) {
final int chunkX = x >> 4;
final int chunkZ = z >> 4;
CachedChunk c = this.cached;
if (c == null || c.chunkX != chunkX || c.chunkZ != chunkZ) {
c = new CachedChunk(chunkX, chunkZ, getChunkOrDefault(chunkX, chunkZ));
this.cached = c;
}
return c.chunk.isSolid(x & 0xF, y, z & 0xF);
}
}
static long wrongAnswers(Cache cache, long queriesPerThread) throws InterruptedException {
final AtomicLong wrong = new AtomicLong();
final CountDownLatch go = new CountDownLatch(1);
final Thread[] threads = new Thread[2];
for (int t = 0; t < 2; t++) {
final int phase = t;
threads[t] = new Thread(() -> {
try { go.await(); } catch (InterruptedException e) { return; }
long bad = 0;
for (long i = 0; i < queriesPerThread; i++) {
final boolean solidSide = ((i + phase) & 1) == 0;
if (cache.get0(solidSide ? 15 : 16, 64, 8) != solidSide) bad++;
}
wrong.addAndGet(bad);
});
threads[t].start();
}
go.countDown();
for (Thread t : threads) t.join();
return wrong.get();
}
public static void main(String[] args) throws Exception {
final long n = 20_000_000;
for (int round = 1; round <= 3; round++) {
System.out.printf("round %d: current %,d wrong, proposed %,d wrong (of %,d each)%n",
round, wrongAnswers(new Current(), n), wrongAnswers(new Proposed(), n), 2 * n);
}
}
}That's a stress test. In game the overlap is brief. A hit gives a wrong collision answer, and it can leave the cache holding the wrong chunk for a position until the next lookup elsewhere or the next packed chunk. It isn't from the Java port either: the same three fields were there when chunk was a pointer, back to 2023. I found it reading the code, not from a crash.
The fix is one immutable record behind a single reference, read once into a local:
private volatile CachedChunk cached;
private record CachedChunk(int chunkX, int chunkZ, Chunk chunk) {}
CachedChunk c = this.cached;
if (c == null || c.chunkX != chunkX || c.chunkZ != chunkZ) {
c = new CachedChunk(chunkX, chunkZ, this.pathfinder.getChunkOrDefault(chunkX, chunkZ, true));
this.cached = c;
}
return c.chunk.isSolid(x & 0xF, adjustedY, z & 0xF);Every thread sees a whole pair or nothing. Threads can still overwrite each other's entry, but that only costs a lookup. The two boi.chunk = null in NetherPathfinderContext become boi.invalidate(). It's been in my fork since Sep 11 (0Mattias/cheesecake@401c69ff). Happy to open a PR, or to do it another way if you'd prefer, e.g. a cache per solver thread.
Source: cabaletta/baritone