[bobhenl]

Overview

No linked issue.

Description

Request.requests is a static map keyed by thread. Tool actions in PlatformManager run on the actor's queue, but the Request.reset() in the event handlers' finally runs on the calling thread, so the pool thread keeps its request. The edit session holds its actor, so the last player who used that pool thread stays in memory after logging out. A heap dump after ~14 h showed 4 disconnected players held only through this map.

• The five tool actions in PlatformManager now reset the request in their own finally, through a resetRequestAfter wrapper.
• Commands and BrushTool already reset on the thread they run on, so they are unchanged.

worldedit-core tests pass, but they don't cover this path. Not tested on a server yet.

Submitter Checklist

• Make sure you are opening from a topic branch (/feature/fix/docs/ branch (right side)) and not your main branch.
• Ensure that the pull request title represents the desired changelog entry.
• New public fields and methods are annotated with @since TODO.
• I read and followed the contribution guidelines.

[Copilot]

🔵 Needs a closer look

The change alters thread-local Request lifecycle on reused pool threads and is untested on a live server, so its nested-task runtime behavior needs human validation beyond automated review.

No open issues 🎉

What changed in this PR

This PR fixes a memory leak in FastAsyncWorldEdit where disconnected players were being retained in memory. Request.requests is a static ConcurrentHashMap keyed by Thread. Tasks queued through AsyncNotifyKeyedQueue run on reused ForkJoinPool worker threads and may populate a thread-local Request (holding an EditSession, which references the Actor) without ever clearing it. The stale entry then pins the last player who used that pool thread until the thread runs another task — a heap dump after ~14h showed 4 disconnected players retained solely through this map. The fix resets the request in a finally block after each top-level task, matching the populate-then-reset()-in-finally convention already used in command/tool code paths.

Changes:

• AsyncNotifyKeyedQueue#call now clears the thread's Request in a finally block (including when the task throws), guarded so only the task that created the request clears it (nested tasks don't remove the outer task's request).
• Added Request.hasRequest() to check for an existing thread request without creating an empty one via computeIfAbsent.

File	Description
worldedit-core/​.../​session/​request/​Request.java	Adds hasRequest() to non-destructively check whether the current thread already has a request.
worldedit-core/​.../​core/​util/​task/​AsyncNotifyKeyedQueue.java	Resets the thread-local Request in a finally after a top-level queued task completes, preventing EditSession/Actor retention on reused pool threads.

No blocking issues were found. The nested-task guard (ownsRequest = !Request.hasRequest()) correctly preserves an outer task's request when a nested task executes inline on the same worker thread, and the finally guarantees cleanup on both normal completion and exceptions. Since the operation is keyed by Thread.currentThread(), the non-atomic hasRequest()/reset() sequence is safe (only the owning thread mutates its own entry). This change also improves Actor#cancel() accuracy by no longer leaving completed edit sessions in Request.getAll().

_{Review effort: Balanced}

[MattBDev]

I see no problems with this change from the diff. Additionally, the PR description and the details from Copilot show it to be as straightforward as it looks while solving a legit issue that I've also noticed but haven't been bothered to fix yet. I suggest @dordsor21 and @SirYwell both approve we merge though. 😉

[SirYwell]

I'm not sure if this is the right place to do that. I'm also not sure what the semantics of a "nested task" is. Do we remove the request properly in other cases? Could you explain what's going on there?

[bobhenl]

Pool threads in AsyncNotifyKeyedQueue are reused, and Request lives in a per-thread map. If a queued task creates a Request and never resets it, that Request keeps its EditSession and the actor reachable until the same thread happens to run another task. We saw that as retained memory on our server. By “nested task” I mean a task that runs inside another task on the same thread, for example when an outer operation calls into the queue and the work runs inline. In that case the outer task already owns the Request, so the inner one must not reset it. That’s why it checks Request.hasRequest() first and only resets when the task created the Request itself. Other paths (command handling via Request.reset() in the platform command manager) already clean up after themselves; this queue was the one place we found where nothing did. It can be moved elsewhere if you prefer a different spot.

[dordsor21]

I would much rather correctly reset the Request in each task - handling it in the queue isn't great - it's not actually fixing the underlying issue

[bobhenl]

Thanks, agreed. I moved the fix out of the queue and removed Request.hasRequest().

The leak came from the tool actions in PlatformManager: they run on the actor's queue, while the Request.reset() in the event handlers' finally runs on the calling thread, so the pool thread kept its request. Commands and BrushTool already reset on the thread they run on. The five tool actions now do the same through a small resetRequestAfter wrapper.

It compiles and the worldedit-core tests pass, but those don't cover this path and I haven't run it on a live server yet.

[MattBDev]

Thanks, agreed. I moved the fix out of the queue and removed Request.hasRequest().

The leak came from the tool actions in PlatformManager: they run on the actor's queue, while the Request.reset() in the event handlers' finally runs on the calling thread, so the pool thread kept its request. Commands and BrushTool already reset on the thread they run on. The five tool actions now do the same through a small resetRequestAfter wrapper.

It compiles and the worldedit-core tests pass, but those don't cover this path and I haven't run it on a live server yet.

This literally reads as AI. Are you familiar with what this code is doing or are you just relying on the AI?

Not comfortable keeping my approval on this right now.

[bobhenl]

Yeah I know what it does, I found it while chasing a memory leak on our server. A heap dump after 14h had 4 players who already logged out, and the only thing still holding them was Request.requests. That map is keyed by thread, and the pool thread that ran their last tool action still had its Request in there, with the EditSession and through it the actor.

The Request.reset() in PlatformManager is in the finally of the event handler, so it runs on the thread that fired the event. But the tool action itself goes through player.runAction() onto the actor’s queue and runs on a pool thread, so that thread’s entry never gets cleared until it happens to run something else that resets it.

Commands and BrushTool already reset on the thread they run on, so I only wrapped the 5 runAction calls in PlatformManager.

[bobhenl]

Tested this on our servers now. The build with this change (on top of the snapshot we were running) has been live on our creative server since Nov 10 and on survival since yesterday's restart.

Heap summaries after the uptime that used to show the leak:

• creative, 18.6 h uptime, 42 online: 1 Request instance, 43 WorldEdit BukkitPlayer (so no logged-out players kept by WorldEdit)
• survival, 7.5 h uptime, 62 online: 1 Request, 62 BukkitPlayer

Before the change, a 14 h heap dump had 4 logged-out players held only through Request.requests. Creative is where people use the wand and other tools all day, so that path gets plenty of use there.