Make FaweStreamChangeSet.blockSize atomic and document read reentrancy#3598
Open
MattBDev wants to merge 3 commits into
Open
Make FaweStreamChangeSet.blockSize atomic and document read reentrancy#3598MattBDev wants to merge 3 commits into
MattBDev wants to merge 3 commits into
Conversation
blockSize was a plain long incremented from six add* methods that run on pipeline worker threads and read unsynchronized from isEmpty()/longSize()/ size(). Neither the increments nor the 64-bit reads were atomic, so counts could be lost and readers could observe a torn value. Use a LongAdder, which stays cheap under the concurrent writers this path actually sees. RollbackOptimizedHistory's constructor is updated for the new field type. Its (int) truncation of the incoming long size is deliberately preserved here: it is a separate known bug and fixing it is out of scope for this change. Also documents that a single FaweStreamChangeSet is not safe for concurrent or overlapping read traversals: posDel, idDel, originX, originZ and version hold decoder state on the instance rather than per traversal, so a second concurrent read, or a read racing readHeader(), corrupts the running deltas. This is documentation only; a runtime guard needs the per-traversal codec refactor and is not attempted here. Adds FaweStreamChangeSetBlockSizeTest covering concurrent add() counting. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR is part of Phase 1 concurrency hardening for com.fastasyncworldedit.core.history, fixing incorrect history size reporting by making FaweStreamChangeSet.blockSize safe under concurrent writes, and documenting non-reentrant read traversal behavior.
Changes:
- Replace
blockSizefrom a plainlongtoLongAdderand update call sites/size readers to useincrement()/sum(). - Update
RollbackOptimizedHistoryinitialization for the newblockSizetype while preserving the historical(int)truncation behavior (documented as intentional). - Add a regression test that stresses concurrent writers to ensure
longSize()remains accurate.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| worldedit-core/src/main/java/com/fastasyncworldedit/core/history/changeset/FaweStreamChangeSet.java | Makes blockSize atomic via LongAdder and documents non-reentrant read traversal constraints. |
| worldedit-core/src/main/java/com/fastasyncworldedit/core/history/RollbackOptimizedHistory.java | Adjusts constructor logic to initialize the LongAdder-based blockSize. |
| worldedit-core/src/test/java/com/fastasyncworldedit/core/history/changeset/FaweStreamChangeSetBlockSizeTest.java | Adds a concurrent-writer regression test for the blockSize counter. |
| worldedit-core/build.gradle.kts | Adds lz4Java to test dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…avadoc If a worker future threw or timed out, the test rethrew immediately from the catch block, skipping the graceful shutdown()/awaitTermination below it and leaking non-daemon threads into the rest of the suite. Move executor construction outside the try and unconditionally shutdownNow() in a finally, so a stuck or failed worker no longer leaks threads. Also corrects the class javadoc, which claimed COMPRESSION_LEVEL was forced to 0 because lz4-java is missing from the test classpath - inaccurate, since this PR adds lz4-java as a test dependency. The real reason: level 0 bypasses the compression backend entirely so the test only measures counter accuracy, and @isolated is needed because Settings is process-global mutable state. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This test's PR only migrates blockSize to a LongAdder; it doesn't include the (separately-fixed, sibling-PR) double-checked-locking fix for getBlockOS(). Without a single-threaded warm-up call first, the first wave of concurrent add() calls below would race on that unrelated, still-present lazy-init bug too, rather than exercising only the counter accuracy this test targets. getBlockOS() itself doesn't touch blockSize, so the expected total asserted at the end is unaffected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of Phase 1 (concurrency hardening) of the
com.fastasyncworldedit.core.historyimprovement plan. One of five independent PRs.The bug
blockSizewas a plainlong, incremented from sixadd*methods that run on pipeline worker threads and read unsynchronized fromisEmpty()/longSize()/size(). Neither the increments nor the 64-bit reads were atomic, so counts were lost and readers could observe a torn value. SincelongSize()is what history reports as its size, this silently under-reports how much an edit actually recorded.The fix
blockSizeis now aLongAdder— chosen overAtomicLongbecause this path has many concurrent writers and infrequent reads, which is exactly whatLongAdderoptimises for.RollbackOptimizedHistory's constructor is updated for the new field type. Its(int)truncation of the incominglongsize is deliberately preserved — that is a separate known bug (alongsilently truncated toint), and fixing it here would be out of scope. It is called out in a comment.FaweStreamChangeSetis not safe for concurrent or overlapping read traversals:posDel,idDel,originX,originZandversionhold decoder state on the instance rather than per traversal, so a second concurrent read — or a read racingreadHeader()— corrupts the running deltas. Documentation only; a runtime guard needs the per-traversal codec refactor.Verification
:worldedit-core:compileJava/compileTestJavapass.FaweStreamChangeSetBlockSizeTestis a genuine regression test, and the strongest evidence in this series: pre-fix it fails withexpected: <16000> but was: <13070>— 2,930 lost increments — and passes with the fix.Reviewer note — possible API break
blockSizeisprotected, so changingprotected long→protected final LongAdderis a source/binary break for any third-party plugin subclassingFaweStreamChangeSet. OnlyRollbackOptimizedHistoryuses it in-tree. If that matters for the public API, the alternative is a privateLongAdderbehind protected accessors — happy to switch if preferred.Notes
DiskStorageHistorychanges; that scope creep was reverted so this PR only covers the counter and docs.git worktreeadditionally need the Grgit fix from Add baseline benchmark for history write path (pre Phase 1 concurrency fixes) #3593; deliberately not included (CI uses a normal checkout).🤖 Generated with Claude Code