Run edit-completion tasks on a dedicated executor

[MattBDev]

Fixes #3420.

Summary

Edit-completion callbacks (sending updated chunks to players, running the caller-supplied finalizer) were submitted to forkJoinPoolSecondary. They are also the tail of the future chain that every SingleThreadQueueExtent flush blocks on. So a task waiting on that pool can be waiting for a task that needs a worker from that same pool — and if no worker is free, neither ever finishes.

This routes completion callbacks to a dedicated executor that can always supply a thread, so the tail of a submission chain never depends on the secondary pool having spare capacity.

The cycle

Two edges are needed, and both were present:

1. Occupancy — something fills the secondary pool with tasks that block.
2. Dependency — a flush waiting on that pool needs a completion callback that is also scheduled on it.

The reporter's dump shows both: every live secondary worker consumed by FastAsyncVoxelSniper 3.2.3, all but one blocked at the entrance of its synchronized(session) block, and the last holding that monitor while blocked in iterateSubmissions waiting for a completion callback. Nothing progressed, so no FAWE command ran at all — including /fawe debugpaste, which is why the original reports have so little to go on.

Why the pool didn't just add a worker

Worth being precise about, because "raise parallel-threads" is the advice people keep giving and it cannot work.

private final ForkJoinPool forkJoinPoolSecondary = new ForkJoinPool(
        Settings.settings().QUEUE.PARALLEL_THREADS,
        new FaweForkJoinWorkerThreadFactory("FAWE Fork Join Pool Secondary - %s"),
        null, false
);

The 4-arg constructor leaves maximumPoolSize at 32767. The pool was free to grow and didn't, because ForkJoinPool only compensates for blocking it can observe through managedBlock — and neither monitorenter nor FutureTask.get is visible to it. It counted its wedged workers as running. The pool is blind, not bounded: no parallel-threads value avoids this.

(Also why this went unexplained for so long: a thread blocked on monitorenter shows no park and no await in a dump. It just looks stopped at a source line.)

The change

QueueHandler — new executor and a completion(Runnable, T) entry point:

private final ThreadPoolExecutor completionExecutor = new ThreadPoolExecutor(
        0, Integer.MAX_VALUE, 60L, TimeUnit.SECONDS,
        new SynchronousQueue<>(),
        new FaweBasicThreadFactory("FAWE Completion Executor - %d")
);

SynchronousQueue + unbounded max means a task never waits in a queue for a worker; core size 0 + 60s keepalive means it holds nothing at rest. CallerRunsPolicy (as FaweCache#newBlockingExecutor uses) means submission cannot be rejected even on shutdown or thread exhaustion — otherwise the default AbortPolicy would contradict the guarantee the javadoc makes. Capping it would reintroduce exactly this bug on the new pool, which is why it isn't capped. In practice it holds about one thread per edit actively finishing.

AbstractBukkitGetBlocks#handleCallFinalizer — async(...) → completion(...), with the invariant recorded in a comment.

adapter-1_21/PaperweightGetBlocks — predates the shared helper and carries its own copy of the chain, so it needs the same change directly. Every other adapter shares the helper.

SingleThreadQueueExtent — unrelated bug found while tracing this path: flush() and submit() didn't release getChunkLock in a finally, so an exception escaping submitUnchecked leaked it permanently. These queues are pooled and reused, so one failed edit could brick a queue instance for the rest of the server's uptime. Also drops a stray LOGGER.info("trim"). Happy to split this into its own PR — it is genuinely unrelated and would be easier to review separately.

Scope note: only the chunk callback/finalizer moves. AbstractChangeSet's history drain (AbstractChangeSet:453) stays on the secondary pool deliberately — its future is discarded at the call site, so nothing blocks on it and it carries no dependency edge.

The invariant

Completion tasks must never block on other FAWE futures. That holds for everything routed here today (the chunk callback/finalizer pair), and it's the thing to check before adding anything new. It's stated in the javadoc on completion(...).

On "commands shouldn't have gone there in the first place"

Agreed, and the javadoc already says so — the secondary pool is documented for short "cleanup" tasks that may be IO-bound. FAVS submitting whole player-synchronised edits there was misuse, and FAVS 3.2.4 correctly stops doing it.

The narrow argument for this PR is that the dependency edge doesn't require any misuse to exist. A completion callback is a cleanup task — it belongs on that pool by the javadoc's own definition. So does the history drain. The pool is therefore in the position of hosting a task that another task is blocked waiting for, which is a starvation cycle among tasks that all legitimately belong there. Misuse makes it easy to hit; it isn't what creates it.

That's the whole claim. This doesn't stop a caller from wedging the secondary pool, and it isn't meant to — it stops a wedged secondary pool from taking the completion path down with it.

I'd rather not oversell the "FAWE could do this to itself with no plugin" version of the argument. I went looking for it in the source: only two places in FAWE submit to this pool, AbstractChangeSet:453 (drainQueue, which doesn't block on a future) and AsyncNotifyQueue:59. The one genuine same-pool .get() is RollbackDatabase:53 in the constructor, and I haven't established that it runs on a secondary-pool thread. So: a plausible shape, not a demonstrated FAWE-only deadlock, and I'm not resting the PR on it.

On #3543

#3543 makes lock acquisition order consistent, which closes lock-order (ABBA) inversions. This is a different failure mode: thread-starvation deadlock needs no inversion and, in the FAVS case, involved a single monitor. Consistent ordering wouldn't have prevented it. The two changes are complementary rather than overlapping — I don't think #3543 subsumes this one.

On virtual threads

Agreed that the secondary pool is a good candidate, being wait-heavy. One caveat for the roadmap: synchronized pins the carrier thread until JEP 491 (JDK 24), and FAWE's sourceCompatibility is 21 — so on the minimum supported runtime a virtual-thread secondary pool would still have pinned on FAVS's synchronized(session) and starved the same way. It's an argument for raising the floor, not against the direction. Out of scope here either way.

Relationship to FastAsyncVoxelSniper #489

FAVS 3.2.4 moves snipes onto AsyncNotifyKeyedQueue and replaces synchronized(session) with per-UUID queueing. That is the right fix on their side and it resolves the reported freeze on its own.

Edge	Cut by	Scope
Occupancy — a caller fills the pool with blocked work	FAVS #489	That plugin
Dependency — completion needs a worker from that same pool	This PR	Every caller

Neither depends on the other.

Behavioral impact

• No public API removed or changed. completion(...) is new and @ApiStatus.Internal.
• Completion work moves off forkJoinPoolSecondary, freeing that pool for what it's documented for.
• One more pool, empty at rest.
• Threads follow the existing naming convention (FAWE Completion Executor - %d).

Testing

Built against the adapters this touches, and run on the server from the original #3420 report with the same plugin set — completion callbacks stayed off the contended pool.

No deterministic reproduction. The original deadlock depended on a plugin's thread timing, and I haven't built a synthetic test that reliably wedges the secondary pool. The argument for the change is structural — the dependency edge is visible in the source — rather than test-driven, and that's worth weighing against it.

Documentation follow-up

@dordsor21 — happy to take the documentation work you described as a separate PR: QueueHandler#async javadoc stating that the secondary pool is not for actor or command work, actor tasks going through Actor#runAction, and plugin-submitted tasks using their own threads. Say the word and I'll open it whether or not this one lands.

[Copilot]

Pull request overview

This PR addresses a thread-starvation deadlock risk in FAWE’s edit submission/flush pipeline by ensuring edit-completion callbacks (the tail of futures that SingleThreadQueueExtent#flush() can block on) run on a dedicated executor rather than the contended secondary ForkJoinPool.

Changes:

• Introduces QueueHandler#completion(Runnable, T) backed by a dedicated, on-demand ThreadPoolExecutor for edit-completion work.
• Routes finalizer/callback execution in Bukkit get-blocks implementations from async(...) (secondary pool) to completion(...).
• Fixes lock-release safety in SingleThreadQueueExtent#submit() and #flush() by unlocking getChunkLock in finally blocks, and removes a stray trim log line.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
worldedit-core/src/main/java/com/fastasyncworldedit/core/queue/implementation/SingleThreadQueueExtent.java	Ensures getChunkLock is always released on exceptional paths; removes noisy trim logging.
worldedit-core/src/main/java/com/fastasyncworldedit/core/queue/implementation/QueueHandler.java	Adds a dedicated completion executor and completion(...) API for starvation-resistant completion callbacks.
worldedit-bukkit/src/main/java/com/fastasyncworldedit/bukkit/adapter/AbstractBukkitGetBlocks.java	Moves chunk-callback/finalizer tail futures onto the new completion executor.
worldedit-bukkit/adapters/adapter-1_21/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_21_R1/PaperweightGetBlocks.java	Applies the same completion-executor routing for the 1.21 adapter’s bespoke chain.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[MattBDev]

Good catch — fixed. The javadoc claimed work that is not routed here: AbstractChangeSet:453 still submits drainQueue(false) via async(...), and this PR does not change that.

Corrected the field and method javadoc to describe what actually runs on this executor (the chunk callback/finalizer pair, i.e. send() plus the caller-supplied finalizer).

Keeping the history drain on the secondary pool is deliberate rather than an oversight. Its future is discarded at the call site, so nothing ever blocks on it and it carries no dependency edge — moving it would widen the change without closing anything. Added a scope note to the PR description to make that explicit.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

worldedit-core/src/main/java/com/fastasyncworldedit/core/queue/implementation/QueueHandler.java:96

• completionExecutor relies on the default RejectedExecutionHandler (AbortPolicy). If thread creation is ever rejected (e.g., executor shutdown or resource exhaustion), completion(...) will throw RejectedExecutionException, contradicting the method/javadoc expectation that completion tasks will always begin execution. Consider using CallerRunsPolicy (as used by FaweCache#newBlockingExecutor) to preserve the "must run" property under rejection scenarios.

    private final ThreadPoolExecutor completionExecutor = new ThreadPoolExecutor(
            0,
            Integer.MAX_VALUE,
            60L,
            TimeUnit.SECONDS,

[MattBDev]

Took the suppressed suggestion about the rejection handler — completionExecutor now uses ThreadPoolExecutor.CallerRunsPolicy(), matching FaweCache#newBlockingExecutor.

The default AbortPolicy did contradict what the javadoc promises: if a worker could not be started (executor shutdown, or the JVM unable to create further threads) completion(...) would have thrown RejectedExecutionException rather than the task being guaranteed to begin. Narrow window given the unbounded max size and SynchronousQueue, but the guarantee is the whole point of the executor, so it should hold rather than nearly hold.

Running inline on the submitting thread is safe here — the future is already complete by the time completion(...) returns, so it cannot reintroduce a wait on a thread that never arrives. Javadoc updated to state this.

[MattBDev]

It is worth noting that this PR should be merged AFTER #3543 because it applies fixes to files that this PR touches. Once that PR is merged, this PR can drop the changes made to adapter-1_21 as they will no longer be needed.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

[dordsor21]

I'm not really sure an unbounded thead pool is a good solution. We can't control what others do, so even if we technically limit the submission via STQE (though this is not a global limit), there is no functional limit. Typical target-size (for submissions) is often 1000s in people's confifs, so that's 1000s.of submissions on potentially 16 threads, each Thread spawned can be 50MB, which would just OOM the application and/or destroy performance.

None of the tasks here have any blocking dependencies on child tasks (at least not on child tasks also submitted to the secondary pool). The issue that saw secondary pool starvation was because FAVS command tasks (which synchronised on the LocalSession to enforce one-task-at-a-time) were being submitted to the secondary pool, which obviously need to have previous edits complete before freeing up their thread usage (fork pools don't switch tasks when it's based on a synchronized block). This meant that no tasks could complete at all. The solution is simply to only submit the correct tasks to the secondary pool.

If there is a remaining exhaustion form FAWE's own code then that should be addressed in place. We also should not have to account for potential API misuse in downstream plugins at the expense of FAWE itself imo. We should definitely correct the javadocs (in this case anything submitted to secondary pool should have no dependents in waits on, if it does, they should be resubmitted as separate futures).