Fix untag race condition

Author: ChanceSD

pvpmanager/src/main/java/me/chancesd/pvpmanager/player/CombatPlayer.java

@@ -30,6 +30,7 @@
 import me.chancesd.pvpmanager.setting.Permissions;
 import me.chancesd.pvpmanager.tasks.NewbieTask;
 import me.chancesd.pvpmanager.utils.CombatUtils;
+import me.chancesd.sdutils.scheduler.SDTask;
 import me.chancesd.sdutils.scheduler.ScheduleUtils;
 import me.chancesd.sdutils.utils.Log;
 import me.chancesd.sdutils.utils.MCVersion;
@@ -49,6 +50,7 @@ public class CombatPlayer extends EcoPlayer {
 	private volatile long totalTagTime;
 	private long lastKillCommandTime;
 	private NewbieTask newbieTask;
+	private SDTask pendingUntagTask;
 	private CombatPlayer enemy;
 	private final Set<CombatPlayer> lastHitters = new HashSet<>();
 	private final Map<String, Integer> victim = new HashMap<>();
@@ -151,7 +153,7 @@ public final void setNewbie(final boolean newbie, final long time) {
 	 * @param other           The other player involved in the attack
 	 * @param timeMiliseconds How long the player should be tagged for
 	 */
-	public final void tag(final boolean isAttacker, final CombatPlayer other, final long timeMiliseconds) {
+	public final synchronized void tag(final boolean isAttacker, final CombatPlayer other, final long timeMiliseconds) {
 		if (hasPerm(Permissions.EXEMPT_COMBAT_TAG)) {
 			Log.debug("Not tagging " + getName() + " because player has permission: " + Permissions.EXEMPT_COMBAT_TAG);
 			return;
@@ -164,6 +166,13 @@ public final void tag(final boolean isAttacker, final CombatPlayer other, final
 			return;
 		}
 
+		// Cancel any pending untag task
+		if (pendingUntagTask != null && !pendingUntagTask.isCancelled()) {
+			pendingUntagTask.cancel();
+			Log.debug("Cancelled stale untag task for " + getName());
+			pendingUntagTask = null;
+		}
+
 		this.totalTagTime = timeMiliseconds;
 
 		final PlayerTagEvent event = new PlayerTagEvent(getPlayer(), this, isAttacker, other.getPlayer());
@@ -207,17 +216,17 @@ public final void tag(final boolean isAttacker, final CombatPlayer other) {
 	/**
 	 * Takes the player out of combat
 	 */
-	public final void untag(final UntagReason reason) {
+	public final synchronized void untag(final UntagReason reason) {
 		if (!isInCombat()) {
-			Log.debug("Not untagging " + getName() + " because player is not tagged.");
 			return;
 		}
 		final PlayerUntagEvent event = new PlayerUntagEvent(getPlayer(), this, reason);
-		ScheduleUtils.ensureMainThread(() -> {
+		pendingUntagTask = ScheduleUtils.ensureMainThread(() -> {
 			Bukkit.getPluginManager().callEvent(event);
 			if (Conf.DISABLE_FLY.asBool() && Conf.RESTORE_FLY.asBool() && getWasAllowedFlight()) {
 				getPlayer().setAllowFlight(getWasAllowedFlight()); // Sync because there's an async catcher on MC 1.8
 			}
+			pendingUntagTask = null;
 		}, getPlayer());
 
 		if (Conf.GLOWING_IN_COMBAT.asBool() && MCVersion.isAtLeast(MCVersion.V1_9)) {
@@ -423,7 +432,7 @@ private void initializeNameTag() {
 	/**
 	 * Apply loaded player data to this CombatPlayer
 	 */
-	public void applyPlayerData(final PlayerData data) {
+	public synchronized void applyPlayerData(final PlayerData data) {
 		// Apply loaded data (overriding defaults)
 		this.pvpState = data.isPvpEnabled();
 		this.toggleTime = data.getToggleTime();
@@ -443,9 +452,7 @@ public void applyPlayerData(final PlayerData data) {
 
 		this.loaded = true;
 		// Wake up any threads waiting for data to load
-		synchronized (this) {
-			notifyAll();
-		}
+		notifyAll();
 		Log.debug("Finished loading data for " + this + (nametag != null ? " with " + nametag.getClass().getSimpleName() : ""));
 	}
 

pvpmanager/src/main/java/me/chancesd/pvpmanager/tasks/TagTask.java

@@ -53,25 +53,13 @@ public final void onPlayerTag(final PlayerTagEvent event) {
 
 	@EventHandler
 	public final void onPlayerUntag(final PlayerUntagEvent event) {
-		stopTracking(event.getCombatPlayer());
+		stopTracking(event.getCombatPlayer(), event.getReason());
 	}
 
 	private final void startTracking(final CombatPlayer combatPlayer) {
 		final ProgressBar progressBar = new ProgressBar(Conf.ACTION_BAR_MESSAGE.asString(), Conf.ACTION_BAR_BARS.asInt(), combatPlayer.getTotalTagTime(),
 				Conf.ACTION_BAR_SYMBOL.asString());
 
-		final TimeProgressSource timeProgressSource = new TimeProgressSource() {
-			@Override
-			public long getGoal() {
-				return combatPlayer.getTotalTagTime();
-			}
-
-			@Override
-			public long getProgress() {
-				return System.currentTimeMillis() - combatPlayer.getTaggedTime();
-			}
-		};
-
 		final CountdownData.Builder builder = new CountdownData.Builder();
 		if (Conf.ACTION_BAR_ENABLED.asBool()) {
 			builder.withActionBar(progressBar, timeSource -> {
@@ -83,24 +71,37 @@ public long getProgress() {
 		if (Conf.BOSS_BAR_ENABLED.asBool()) {
 			builder.withBossBar(bossBar.build(), timeSource -> {
 				final double secondsRemaining = (timeSource.getGoal() - timeSource.getProgress()) / 1000.0;
-				final String message = Conf.BOSS_BAR_MESSAGE.asString().replace("<time>",
-						Double.toString(Utils.roundTo1Decimal(secondsRemaining)));
+				final String message = Conf.BOSS_BAR_MESSAGE.asString().replace("<time>", Double.toString(Utils.roundTo1Decimal(secondsRemaining)));
 				return CombatUtils.processPlaceholders(combatPlayer.getPlayer(), message);
 			});
 		}
+
+		final TimeProgressSource timeProgressSource = new TimeProgressSource() {
+			@Override
+			public long getGoal() {
+				return combatPlayer.getTotalTagTime();
+			}
+
+			@Override
+			public long getProgress() {
+				return System.currentTimeMillis() - combatPlayer.getTaggedTime();
+			}
+		};
 		final CountdownData countdownData = builder.withTimeSource(timeProgressSource)
-				.onFinish(() -> {
-					combatPlayer.untag(UntagReason.TIME_EXPIRED);
-				})
+				.onFinish(() -> combatPlayer.untag(UntagReason.TIME_EXPIRED))
 				.build(combatPlayer.getPlayer());
 
 		display.createCountdown(combatPlayer.getPlayer(), countdownData);
 		taggedCountdowns.put(combatPlayer, countdownData);
 	}
 
-	private final void stopTracking(final CombatPlayer combatPlayer) {
+	private final void stopTracking(final CombatPlayer combatPlayer, final UntagReason reason) {
 		final CountdownData countdownData = taggedCountdowns.remove(combatPlayer);
-		display.cancelCountdown(combatPlayer.getPlayer(), countdownData);
+
+		// Don't call cancelCountdown for TIME_EXPIRED, already removed by display
+		if (reason != UntagReason.TIME_EXPIRED) {
+			display.cancelCountdown(combatPlayer.getPlayer(), countdownData);
+		}
 	}
 
 	public Set<CombatPlayer> getTaggedPlayers() {