Skip to content

Stop threads and subscribers that outlive a plugin disable - #1020

Open
tricrotism wants to merge 4 commits into
PlayPro:masterfrom
tricrotism:for-upstream/disable-cleanup
Open

tricrotism wants to merge 4 commits into
PlayPro:masterfrom
tricrotism:for-upstream/disable-cleanup

Conversation

@tricrotism

Copy link
Copy Markdown
Contributor

Summary

CoreProtect starts a handful of threads and subscribers on enable and never stops any of them on disable. They keep running against a disabled plugin and hold on to its classloader after a plugin reload. This change stops each of them during ShutdownService.safeShutdown.

The problem

  • WorldEdit subscriber. The EditSessionEvent subscriber is only unregistered when /co reload turns WorldEdit logging off (config/ConfigHandler.java:952). Shutdown never unregisters it. Every later edit session still wraps its extent in CoreProtectLogger from the disabled plugin, and edits that run while CoreProtect drains its queue at stop time still call Queue after serverRunning has gone false.
  • Cache sweeper thread. PluginInitializationService.java:164 starts new Thread(new CacheHandler()): unnamed, untracked, not a daemon. It only checks serverRunning after a full 14-slot pass, so it keeps sweeping for up to 14 seconds after disable. Nothing can stop it early, because its catch (Exception) treats an interrupt as an error and keeps looping.
  • Inspector lookups. BaseInspector, ArmorStandManipulateListener and HangingBreakByEntityListener start a new raw thread per inspector click. There is no bound on how many run at once, and nothing stops them at shutdown.
  • bStats. The Metrics object is created and discarded, so its scheduler thread keeps running until its next submit notices the plugin is disabled.
  • Folia shutdown checkpoint. EntitySpawnTracking.java:818 schedules a checkpoint task per tracked entity owned by another region, then waits up to 30 seconds for all of them. The comment above that loop already notes that "a newly scheduled task may never run during shutdown". When they do not run, the stop waits the full 30 seconds and then reports the TimeoutException as an error.

The fix

  • services/ShutdownService.java: unregisters the WorldEdit subscriber and stops bStats at the start of shutdown. After the queue drain it stops the cache thread and the inspector pool, before performDisable.
  • worldedit/CoreProtectLogger.java, worldedit/FastAsyncWorldEditLogger.java: pass edits straight through once serverRunning is false.
  • thread/CacheHandler.java: startThread() and stopThread(long) own a named daemon thread (CoreProtect-Cache). An interrupt ends the loop.
  • listener/player/inspector/BaseInspector.java: inspector lookups run on a pool of two named daemon threads (CoreProtect-Inspector-N) with a queue of 64. runLookup keeps the existing LookupThrottle handling. If the queue is full, it releases the throttle and tells the player the database is busy. shutdown() waits up to 5 seconds, then interrupts. The armor stand and hanging listeners use runLookup instead of starting their own threads.
  • services/PluginInitializationService.java: keeps the Metrics instance and calls its shutdown() on disable.
  • utility/EntitySpawnTracking.java: the Folia checkpoint waits 5 seconds and does not report a timeout. A task that has not run after 5 seconds during a stop is the case the existing comment describes, and it does not checkpoint whether the stop waits 5 or 30 seconds for it, so the shorter wait loses nothing the longer one saved.

Behaviour change

  • Shutdown prints the "WorldEdit integration disabled" line.
  • At most two inspector lookups run at once and further clicks queue behind them. Past 64 queued lookups the player gets the existing "database busy" message.
  • On Folia, a stop waits at most 5 seconds for checkpoint tasks that are not going to run, instead of 30, and no longer reports that timeout as an error.

Risk

  • unloadWorldEdit() is only called from /co reload today, not from performDisable, so it runs once at shutdown.
  • Two inspector threads: inspector lookups are short single-location queries, and upstream's LookupThrottle already limits each player to one at a time. The pool bounds the total across players, which upstream did not.
  • The inspector pool is created with the class but starts no thread until the first click, and core threads time out after 30 seconds idle.

Testing

Build: mvn package passes.
Row parity: the 47-step scenario on Paper 26.2 and Folia 1.21.11 with SQLite, ending in a normal server stop, matches upstream apart from the random plant that bone meal grows. No new errors, and the stop completed normally on both.

Suggested test: enable, inspect a few blocks, then /plugman reload CoreProtect (or disable and enable through a plugin manager) and take a thread dump. Upstream leaves an unnamed Thread-N running CacheHandler and one thread per inspector click. This branch leaves no CoreProtect threads. With WorldEdit installed, a //set after disabling CoreProtect should no longer go through CoreProtectLogger.

The WorldEdit subscriber, the cache sweeper thread, per-click inspector threads and the bStats scheduler all kept running after CoreProtect was disabled, and the Folia shutdown checkpoint could wait 30 seconds on tasks that never run. Shutdown now unregisters and stops each of them, inspector lookups use a small named pool, and the checkpoint waits 5 seconds.
@netlify

netlify Bot commented Sep 23, 2026

Copy link
Copy Markdown

❌ Deploy Preview for coreprotect failed. Why did it fail? →

Name Link
🔨 Latest commit de65faf
🔍 Latest deploy log https://app.netlify.com/projects/coreprotect/deploys/6ab3efdba5c2ac00070fd6e6

@Intelli

Intelli commented Sep 25, 2026

Copy link
Copy Markdown
Contributor

Thanks -- automated review is requesting the following changes:

  • Recreate the executor when the plugin is enabled again; the static final executor currently remains permanently shut down after a same-instance disable/enable.
  • Release each discarded queued lookup’s throttle slot. Tasks returned by shutdownNow() never execute their finally cleanup.
  • Account for active lookups that outlive interruption, with bounded termination checking and appropriate cancellation/diagnostics. The current implementation cannot guarantee that no inspector threads survive disable.
  • Please also remove the Folia checkpoint timeout change from this PR. Those checkpoints flush pending entity-container transactions, not just locations, and a five-second timeout does not prove the task will never run. Silently shortening that window needs separate justification.

The inspector executor was a static final that stayed shut down after a disable, so a same-instance enable could never run another lookup. It is now created on each enable. At shutdown, lookups still queued after the drain window release their throttle slot, since shutdownNow() returns them without running their finally block, and running lookups that survive the interrupt are logged by thread and player after a bounded wait. The Folia checkpoint timeout change is reverted to keep this change to thread cleanup.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants