Document the secondary pool's task contract - #3610
Open
MattBDev wants to merge 2 commits into
Open
Conversation
The javadoc on QueueHandler#async described what the secondary pool is for, but not what a task submitted to it is required to avoid. Two rules were relied on implicitly and are now stated: - A task must not block waiting on a future that this pool completes. ForkJoinPool only compensates for blocking it observes through managedBlock, so monitorenter and Future#get leave a worker counted as running and no replacement is started. Callers with such a dependency should submit the dependent half separately and chain the futures rather than blocking inside one task. - Actor and command work does not belong here; Actor#runAction serialises per actor without holding a worker. Also notes that downstream plugins should use their own threads rather than this pool, which is sized for FAWE's own cleanup work. Documentation only; no behavioural change.
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds clearer documentation for the secondary ForkJoinPool to prevent deadlocks and misuse (especially blocking waits and actor/command work) when submitting “cleanup” tasks.
Changes:
- Expanded Javadoc on
forkJoinPoolSecondaryto document a strict submission contract and rationale. - Added the same contract summary to the three
async(...)overloads that submit to the secondary pool.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+219
to
+222
| * <p> | ||
| * The submitted task must not wait on a {@link Future} completed by this pool, and must not be actor or command work. | ||
| * See the {@code forkJoinPoolSecondary} field for the full contract and the reasoning behind it. | ||
| * </p> |
Member
There was a problem hiding this comment.
Mm this is probably a good point
Comment on lines
+219
to
+222
| * <p> | ||
| * The submitted task must not wait on a {@link Future} completed by this pool, and must not be actor or command work. | ||
| * See the {@code forkJoinPoolSecondary} field for the full contract and the reasoning behind it. | ||
| * </p> |
The full contract lived on the private forkJoinPoolSecondary field,
which most Javadoc builds exclude, so the async(...) overloads'
"See the field" pointer led nowhere for readers of the generated docs.
Move the contract to the public getForkJoinPoolSecondary() accessor
and have the field and the three async(...) overloads link to it with
{@link}/@see instead of an unresolvable textual reference.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (3)
worldedit-core/src/main/java/com/fastasyncworldedit/core/queue/implementation/QueueHandler.java:216
- Same as the other overloads: this method submits work to the secondary pool and returns a
Futureimmediately, so "Complete a task" is misleading in the Javadoc.
* Complete a task in the {@code forkJoinPoolSecondary} queue. Secondary queue should be used for "cleanup" tasks that are
* likely to be shorter in life than those submitted to the primary queue. They may be IO-bound tasks.
worldedit-core/src/main/java/com/fastasyncworldedit/core/queue/implementation/QueueHandler.java:232
- Same wording nit as the other
async(...)overloads: this API submits work and returns aFuture, it doesn’t "complete" the task synchronously.
* Complete a task in the {@code forkJoinPoolSecondary} queue. Secondary queue should be used for "cleanup" tasks that are
* likely to be shorter in life than those submitted to the primary queue. They may be IO-bound tasks.
worldedit-core/src/main/java/com/fastasyncworldedit/core/queue/implementation/QueueHandler.java:198
- The Javadoc says "Complete a task" but these
async(...)overloads are submitting work to the secondary pool (returning aFutureimmediately). Using "Submit"/"Run" here would be more accurate and avoids implying the task is already completed when the method returns.
This issue also appears in the following locations of the same file:
- line 215
- line 231
* Complete a task in the {@code forkJoinPoolSecondary} queue. Secondary queue should be used for "cleanup" tasks that are
* likely to be shorter in life than those submitted to the primary queue. They may be IO-bound tasks.
MattBDev
enabled auto-merge (squash)
August 11, 2026 21:09
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.
@dordsor21 — this is the documentation work you asked for, split out as promised in #3609. It stands alone and doesn't depend on that PR landing.
What this adds
QueueHandler#async's javadoc said what the secondary pool is for ("cleanup" tasks, possibly IO-bound) but never said what a task submitted to it must not do. Two rules were load-bearing and undocumented:1. A task must not wait on anything this pool completes.
ForkJoinPoolonly compensates for blocking it can observe throughmanagedBlock. It cannot seemonitorenterorFuture#get, so a blocked worker is still counted as running and no replacement is started. Every worker can end up parked waiting for work only this pool can perform.The javadoc now states the rule and the remedy you described — if a task does have a dependency, don't block inside it; submit the dependent half separately and chain the futures.
2. Actor and command work doesn't belong here.
Player-facing actions go through
Actor#runAction(or thequeueAction/runAsyncIfFreewrappers), which serialises per actor without holding a worker on this pool.Plus a note that downstream plugins should use their own threads rather than this pool, which is sized by
parallel-threadsfor FAWE's own cleanup work.Shape of the change
The full contract lives on the
forkJoinPoolSecondaryfield. The threeasync(...)overloads get a two-line summary pointing at it, rather than repeating it three times.Documentation only — no behavioural change, no API change.