Fix FakePlayer leaking CriteriaTrigger listener entries

[zxzinn]

Fixes #1487

The ServerPlayer super constructor calls PlayerList.getPlayerAdvancements(), which creates a PlayerAdvancements and registers listeners on every CriteriaTrigger. FakePlayer never uses advancements (awards are already no-oped), but the listeners are never unregistered, so they accumulate for the lifetime of the server.

On modded servers where mods frequently create FakePlayer instances (Create, Applied Energistics, etc.), this causes a gradual memory leak. Over hours of uptime the orphaned listener entries increase GC pressure, eventually leading to long GC pauses that can trigger the Server Watchdog.

The fix calls stopListening() at the end of the FakePlayer constructor to immediately unregister the listeners that the super constructor registered. This covers both FakePlayerFactory usage and direct subclasses of FakePlayer.

The same issue exists on all 1.21.x branches (1.21.3 through 1.21.11). Happy to open backport PRs if needed.

All existing game tests pass.

[neoforged-pr-publishing]

• Publish PR to GitHub Packages

[CLAassistant]

All committers have signed the CLA.

[XFactHD]

This fix is not correct and will break advancements when a fake player is created with a real player's UUID (which is very often done to make fake players work well with chunk protection mods).
PlayerAdvancements are cached by UUID in PlayerList. This leads to two scenarios:

• When the fake player is created first, then the real player takes over ownership of the PlayerAdvancements but the listeners are never re-registered, breaking advancements until the player reconnects since the disconnect causes the PlayerAdvancements to be removed from the cache
• When the real player is created first, then the fake player construction causes the listeners to be unregistered, breaking advancements for the real player until they reconnect

This is also why #1746 was closed as unsuitable.

Additionally this approach fails to protect against the memory leak when /reload is run as this will re-register the listeners.

[zxzinn]

This fix is not correct and will break advancements when a fake player is created with a real player's UUID (which is very often done to make fake players work well with chunk protection mods). PlayerAdvancements are cached by UUID in PlayerList. This leads to two scenarios:

• When the fake player is created first, then the real player takes over ownership of the PlayerAdvancements but the listeners are never re-registered, breaking advancements until the player reconnects since the disconnect causes the PlayerAdvancements to be removed from the cache
• When the real player is created first, then the fake player construction causes the listeners to be unregistered, breaking advancements for the real player until they reconnect

This is also why #1746 was closed as unsuitable.

Additionally this approach fails to protect against the memory leak when /reload is run as this will re-register the listeners.

I see, thanks for pointing out.

would the better approach to be changing PlayerList.getPlayerAdvancements() so that FakePlayer instances get their own PlayerAdvancements that is not stored in this.advancements? The object would need stopListening() called immediately after construction to prevent the leak, and since it's not in the cache, it can't interfere with a real player's entry?

[XFactHD]

The best solution is most likely to extend PlayerAdvancements, dummy out all relevant methods (i.e. the same thing we already do for the Connection) and then return a singleton instance of that class when a fake player queries the PlayerAdvancements for its UUID. Dummying out relevant methods also alleviates the need to unregister the instance and removes the overhead of unnecessarily loading advancement data.

[zxzinn]

The best solution is most likely to extend PlayerAdvancements, dummy out all relevant methods (i.e. the same thing we already do for the Connection) and then return a singleton instance of that class when a fake player queries the PlayerAdvancements for its UUID. Dummying out relevant methods also alleviates the need to unregister the instance and removes the overhead of unnecessarily loading advancement data.

PlayerAdvancements's constructor calls the private load() which registers listeners. A subclass can't prevent that. Is there a preferred way to handle this?