From 4c803593ed4364010e895a810740417e3b8273f5 Mon Sep 17 00:00:00 2001
From: aromaa <me@joniaromaa.fi>
Date: Mon, 15 Apr 2024 20:25:11 +0300
Subject: [PATCH] Adjust PhaseState pooling

This is a correctness fix as we should not store the states
before they are properly closed and the state has been cleared.

We also didn't clear or return states to the pool that
didn't provide modifiers and could leak resources for
long periods of time until they were used again.

As added bonus we now pool more states and removes some branching.
---
 .../common/event/tracking/PhaseContext.java   | 18 ++++---------
 .../event/tracking/PooledPhaseState.java      | 26 +++----------------
 2 files changed, 9 insertions(+), 35 deletions(-)

diff --git a/src/main/java/org/spongepowered/common/event/tracking/PhaseContext.java b/src/main/java/org/spongepowered/common/event/tracking/PhaseContext.java
index 828c030c3b7..1b378e1f70d 100644
--- a/src/main/java/org/spongepowered/common/event/tracking/PhaseContext.java
+++ b/src/main/java/org/spongepowered/common/event/tracking/PhaseContext.java
@@ -323,22 +323,14 @@ public void close() { // Should never throw an exception
         }
         final PhaseTracker instance = PhaseTracker.getInstance();
         instance.completePhase(this);
-        if (!this.shouldProvideModifiers()) {
-            if (this.usedFrame != null) {
-                this.usedFrame.iterator().forEachRemaining(instance::popCauseFrame);
-            }
-            return;
-        }
-        if (this.usedFrame == null) {
-            // So, this part is interesting... Since the used frame is null, that means
-            // the cause stack manager still has the refernce of this context/phase, we have
-            // to "pop off" the list.
-            instance.popFrameMutator(this);
-        }
         if (this.usedFrame != null) {
             this.usedFrame.iterator().forEachRemaining(instance::popCauseFrame);
-            this.usedFrame.clear();
             this.usedFrame = null;
+        } else if (this.shouldProvideModifiers()) {
+            // So, this part is interesting... Since the used frame is null, that means
+            // the cause stack manager still has the reference of this context/phase, we have
+            // to "pop off" the list.
+            instance.popFrameMutator(this);
         }
         this.reset();
         this.isCompleted = false;
diff --git a/src/main/java/org/spongepowered/common/event/tracking/PooledPhaseState.java b/src/main/java/org/spongepowered/common/event/tracking/PooledPhaseState.java
index e21837bb35d..24e8ddce470 100644
--- a/src/main/java/org/spongepowered/common/event/tracking/PooledPhaseState.java
+++ b/src/main/java/org/spongepowered/common/event/tracking/PooledPhaseState.java
@@ -43,13 +43,13 @@ public final C createPhaseContext(final PhaseTracker tracker) {
         }
 
         if (tracker == PhaseTracker.SERVER) {
-            if (this.serverCached != null && !this.serverCached.isCompleted) {
+            if (this.serverCached != null) {
                 final C cached = this.serverCached;
                 this.serverCached = null;
                 return cached;
             }
         } else if (tracker == PhaseTracker.CLIENT) {
-            if (this.clientCached != null && !this.clientCached.isCompleted) {
+            if (this.clientCached != null) {
                 final C cached = this.clientCached;
                 this.clientCached = null;
                 return cached;
@@ -57,20 +57,9 @@ public final C createPhaseContext(final PhaseTracker tracker) {
         }
         final @Nullable C peek = tracker.getContextPoolFor(this).pollFirst();
         if (peek != null) {
-            if (tracker == PhaseTracker.SERVER) {
-                this.serverCached = peek;
-            } else if (tracker == PhaseTracker.CLIENT) {
-                this.clientCached = peek;
-            }
             return peek;
         }
-        final C maybeCached = this.createNewContext(tracker);
-        if (tracker == PhaseTracker.SERVER) {
-            this.serverCached = maybeCached;
-        } else if (tracker == PhaseTracker.CLIENT) {
-            this.clientCached = maybeCached;
-        }
-        return maybeCached;
+        return this.createNewContext(tracker);
     }
 
     final void releaseContextFromPool(final C context) {
@@ -79,25 +68,18 @@ final void releaseContextFromPool(final C context) {
             throw new IllegalStateException("Asynchronous Thread Access to PhaseTracker: " + createdTracker);
         }
         if (createdTracker == PhaseTracker.SERVER) {
-            if (this.serverCached == context) {
-                return;
-            }
             if (this.serverCached == null) {
                 this.serverCached = context;
                 return;
             }
         }
-        if (createdTracker == PhaseTracker.CLIENT) {
-            if (this.clientCached == context) {
-                return;
-            }
+        else if (createdTracker == PhaseTracker.CLIENT) {
             if (this.clientCached == null) {
                 this.clientCached = context;
                 return;
             }
         }
         createdTracker.getContextPoolFor(this).push(context);
-
     }
 
     protected abstract C createNewContext(PhaseTracker tracker);