From 17baaf6721fa8985d7fab33d3dc707dfae270705 Mon Sep 17 00:00:00 2001 From: Tim203 Date: Sat, 14 Jun 2025 15:36:39 +0200 Subject: [PATCH] Fix the remaining concurrency issues with scoreboard display slots (#5592) With the scoreboard updater thread, there can be multiple threads editing specific bits which can result in concurrency issues. This PR aims to remove those remaining concurrency issues. --- .../display/slot/BelownameDisplaySlot.java | 52 ++++++------ .../display/slot/PlayerlistDisplaySlot.java | 81 ++++++++++--------- .../geyser/session/cache/EntityCache.java | 46 +++++++---- .../JavaFinishConfigurationTranslator.java | 9 +-- 4 files changed, 107 insertions(+), 81 deletions(-) diff --git a/core/src/main/java/org/geysermc/geyser/scoreboard/display/slot/BelownameDisplaySlot.java b/core/src/main/java/org/geysermc/geyser/scoreboard/display/slot/BelownameDisplaySlot.java index 42a1e8c3f..825fd447e 100644 --- a/core/src/main/java/org/geysermc/geyser/scoreboard/display/slot/BelownameDisplaySlot.java +++ b/core/src/main/java/org/geysermc/geyser/scoreboard/display/slot/BelownameDisplaySlot.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2024 GeyserMC. http://geysermc.org + * Copyright (c) 2024-2025 GeyserMC. http://geysermc.org * * Permission is hereby granted, free of charge, to any person obtaining a copy * of this software and associated documentation files (the "Software"), to deal @@ -61,44 +61,42 @@ public class BelownameDisplaySlot extends DisplaySlot { // remove is handled in #remove() if (updateType == UpdateType.ADD) { - for (PlayerEntity player : session.getEntityCache().getAllPlayerEntities()) { - playerRegistered(player); - } + session.getEntityCache().forEachPlayerEntity(this::playerRegistered); return; } if (updateType == UpdateType.UPDATE) { - for (PlayerEntity player : session.getEntityCache().getAllPlayerEntities()) { + session.getEntityCache().forEachPlayerEntity(player -> { setBelowNameText(player, scoreFor(player.getUsername())); - } + }); updateType = UpdateType.NOTHING; return; } - for (var score : displayScores.values()) { - // we don't have to worry about a score not existing, because that's handled by both - // this method when an objective is added and addScore/playerRegistered. - // we only have to update them, if they have changed - // (or delete them, if the score no longer exists) - if (!score.shouldUpdate()) { - continue; - } + synchronized (displayScores) { + for (var score : displayScores.values()) { + // we don't have to worry about a score not existing, because that's handled by both + // this method when an objective is added and addScore/playerRegistered. + // we only have to update them, if they have changed + // (or delete them, if the score no longer exists) + if (!score.shouldUpdate()) { + continue; + } - if (score.referenceRemoved()) { - clearBelowNameText(score.player()); - continue; - } + if (score.referenceRemoved()) { + clearBelowNameText(score.player()); + continue; + } - score.markUpdated(); - setBelowNameText(score.player(), score.reference()); + score.markUpdated(); + setBelowNameText(score.player(), score.reference()); + } } } @Override public void remove() { updateType = UpdateType.REMOVE; - for (PlayerEntity player : session.getEntityCache().getAllPlayerEntities()) { - clearBelowNameText(player); - } + session.getEntityCache().forEachPlayerEntity(this::clearBelowNameText); } @Override @@ -119,7 +117,9 @@ public class BelownameDisplaySlot extends DisplaySlot { @Override public void playerRemoved(PlayerEntity player) { - displayScores.remove(player.getGeyserId()); + synchronized (displayScores) { + displayScores.remove(player.getGeyserId()); + } } private void addDisplayScore(ScoreReference reference) { @@ -131,7 +131,9 @@ public class BelownameDisplaySlot extends DisplaySlot { private BelownameDisplayScore addDisplayScore(PlayerEntity player, ScoreReference reference) { var score = new BelownameDisplayScore(this, objective.getScoreboard().nextId(), reference, player); - displayScores.put(player.getGeyserId(), score); + synchronized (displayScores) { + displayScores.put(player.getGeyserId(), score); + } return score; } diff --git a/core/src/main/java/org/geysermc/geyser/scoreboard/display/slot/PlayerlistDisplaySlot.java b/core/src/main/java/org/geysermc/geyser/scoreboard/display/slot/PlayerlistDisplaySlot.java index 6fd83ab8d..4660ddad2 100644 --- a/core/src/main/java/org/geysermc/geyser/scoreboard/display/slot/PlayerlistDisplaySlot.java +++ b/core/src/main/java/org/geysermc/geyser/scoreboard/display/slot/PlayerlistDisplaySlot.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2024 GeyserMC. http://geysermc.org + * Copyright (c) 2024-2025 GeyserMC. http://geysermc.org * * Permission is hereby granted, free of charge, to any person obtaining a copy * of this software and associated documentation files (the "Software"), to deal @@ -26,7 +26,6 @@ package org.geysermc.geyser.scoreboard.display.slot; import it.unimi.dsi.fastutil.longs.Long2ObjectMap; -import it.unimi.dsi.fastutil.longs.Long2ObjectMaps; import it.unimi.dsi.fastutil.longs.Long2ObjectOpenHashMap; import java.util.ArrayList; import java.util.Collections; @@ -41,8 +40,7 @@ import org.geysermc.geyser.session.GeyserSession; import org.geysermc.mcprotocollib.protocol.data.game.scoreboard.ScoreboardPosition; public class PlayerlistDisplaySlot extends DisplaySlot { - private final Long2ObjectMap displayScores = - Long2ObjectMaps.synchronize(new Long2ObjectOpenHashMap<>()); + private final Long2ObjectMap displayScores = new Long2ObjectOpenHashMap<>(); private final List removedScores = Collections.synchronizedList(new ArrayList<>()); public PlayerlistDisplaySlot(GeyserSession session, Objective objective) { @@ -71,35 +69,37 @@ public class PlayerlistDisplaySlot extends DisplaySlot { removedScores.clear(); } - for (var score : displayScores.values()) { - if (score.referenceRemoved()) { - ScoreInfo cachedInfo = score.cachedInfo(); - // cachedInfo can be null here when ScoreboardUpdater is being used and a score is added and - // removed before a single update cycle is performed - if (cachedInfo != null) { - removeScores.add(cachedInfo); + synchronized (displayScores) { + for (var score : displayScores.values()) { + if (score.referenceRemoved()) { + ScoreInfo cachedInfo = score.cachedInfo(); + // cachedInfo can be null here when ScoreboardUpdater is being used and a score is added and + // removed before a single update cycle is performed + if (cachedInfo != null) { + removeScores.add(cachedInfo); + } + continue; } - continue; - } - //todo does an animated title exist on tab? - boolean add = objectiveAdd || objectiveUpdate; - boolean exists = score.exists(); + //todo does an animated title exist on tab? + boolean add = objectiveAdd || objectiveUpdate; + boolean exists = score.exists(); - if (score.shouldUpdate()) { - score.update(objective); - add = true; - } + if (score.shouldUpdate()) { + score.update(objective); + add = true; + } - if (add) { - addScores.add(score.cachedInfo()); - } + if (add) { + addScores.add(score.cachedInfo()); + } - // we need this as long as MCPE-143063 hasn't been fixed. - // the checks after 'add' are there to prevent removing scores that - // are going to be removed anyway / don't need to be removed - if (add && exists && objectiveNothing) { - removeScores.add(score.cachedInfo()); + // we need this as long as MCPE-143063 hasn't been fixed. + // the checks after 'add' are there to prevent removing scores that + // are going to be removed anyway / don't need to be removed + if (add && exists && objectiveNothing) { + removeScores.add(score.cachedInfo()); + } } } @@ -124,16 +124,17 @@ public class PlayerlistDisplaySlot extends DisplaySlot { players.add(selfPlayer); } - for (PlayerEntity player : players) { - var score = - new PlayerlistDisplayScore(this, objective.getScoreboard().nextId(), reference, player.getGeyserId()); - displayScores.put(player.getGeyserId(), score); + synchronized (displayScores) { + for (PlayerEntity player : players) { + var score = new PlayerlistDisplayScore(this, objective.getScoreboard().nextId(), reference, player.getGeyserId()); + displayScores.put(player.getGeyserId(), score); + } } } private void registerExisting() { playerRegistered(session.getPlayerEntity()); - session.getEntityCache().getAllPlayerEntities().forEach(this::playerRegistered); + session.getEntityCache().forEachPlayerEntity(this::playerRegistered); } @Override @@ -142,14 +143,20 @@ public class PlayerlistDisplaySlot extends DisplaySlot { if (reference == null) { return; } - var score = - new PlayerlistDisplayScore(this, objective.getScoreboard().nextId(), reference, player.getGeyserId()); - displayScores.put(player.getGeyserId(), score); + + var score = new PlayerlistDisplayScore(this, objective.getScoreboard().nextId(), reference, player.getGeyserId()); + synchronized (displayScores) { + displayScores.put(player.getGeyserId(), score); + } } @Override public void playerRemoved(PlayerEntity player) { - var score = displayScores.remove(player.getGeyserId()); + PlayerlistDisplayScore score; + synchronized (displayScores) { + score = displayScores.remove(player.getGeyserId()); + } + if (score == null) { return; } diff --git a/core/src/main/java/org/geysermc/geyser/session/cache/EntityCache.java b/core/src/main/java/org/geysermc/geyser/session/cache/EntityCache.java index 78d21e63b..a0e5db79c 100644 --- a/core/src/main/java/org/geysermc/geyser/session/cache/EntityCache.java +++ b/core/src/main/java/org/geysermc/geyser/session/cache/EntityCache.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2019-2022 GeyserMC. http://geysermc.org + * Copyright (c) 2019-2025 GeyserMC. http://geysermc.org * * Permission is hereby granted, free of charge, to any person obtaining a copy * of this software and associated documentation files (the "Software"), to deal @@ -32,11 +32,11 @@ import it.unimi.dsi.fastutil.longs.Long2ObjectOpenHashMap; import it.unimi.dsi.fastutil.objects.Object2ObjectOpenHashMap; import it.unimi.dsi.fastutil.objects.ObjectArrayList; import java.util.ArrayList; -import java.util.Collection; import java.util.List; import java.util.Map; import java.util.UUID; import java.util.concurrent.atomic.AtomicLong; +import java.util.function.Consumer; import lombok.Getter; import org.geysermc.geyser.entity.type.Entity; import org.geysermc.geyser.entity.type.Tickable; @@ -136,10 +136,12 @@ public class EntityCache { } public void addPlayerEntity(PlayerEntity entity) { - // putIfAbsent matches the behavior of playerInfoMap in Java as of 1.19.3 - boolean exists = playerEntities.putIfAbsent(entity.getUuid(), entity) != null; - if (exists) { - return; + synchronized (playerEntities) { + // putIfAbsent matches the behavior of playerInfoMap in Java as of 1.19.3 + boolean exists = playerEntities.putIfAbsent(entity.getUuid(), entity) != null; + if (exists) { + return; + } } // notify scoreboard for new entity @@ -148,21 +150,29 @@ public class EntityCache { } public PlayerEntity getPlayerEntity(UUID uuid) { - return playerEntities.get(uuid); + synchronized (playerEntities) { + return playerEntities.get(uuid); + } } public List getPlayersByName(String name) { var list = new ArrayList(); - for (PlayerEntity player : playerEntities.values()) { - if (name.equals(player.getUsername())) { - list.add(player); + synchronized (playerEntities) { + for (PlayerEntity player : playerEntities.values()) { + if (name.equals(player.getUsername())) { + list.add(player); + } } } return list; } public PlayerEntity removePlayerEntity(UUID uuid) { - var player = playerEntities.remove(uuid); + PlayerEntity player; + synchronized (playerEntities) { + player = playerEntities.remove(uuid); + } + if (player != null) { // notify scoreboard session.getWorldCache().getScoreboard().playerRemoved(player); @@ -170,12 +180,20 @@ public class EntityCache { return player; } - public Collection getAllPlayerEntities() { - return playerEntities.values(); + /** + * Run a specific bit of code for each cached player entity. + * As usual with synchronized, try to minimize the amount of work you because you block the PlayerList collection. + */ + public void forEachPlayerEntity(Consumer player) { + synchronized (playerEntities) { + playerEntities.values().forEach(player); + } } public void removeAllPlayerEntities() { - playerEntities.clear(); + synchronized (playerEntities) { + playerEntities.clear(); + } } public void addBossBar(UUID uuid, BossBar bossBar) { diff --git a/core/src/main/java/org/geysermc/geyser/translator/protocol/java/JavaFinishConfigurationTranslator.java b/core/src/main/java/org/geysermc/geyser/translator/protocol/java/JavaFinishConfigurationTranslator.java index 22b8c6663..73f938284 100644 --- a/core/src/main/java/org/geysermc/geyser/translator/protocol/java/JavaFinishConfigurationTranslator.java +++ b/core/src/main/java/org/geysermc/geyser/translator/protocol/java/JavaFinishConfigurationTranslator.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2024 GeyserMC. http://geysermc.org + * Copyright (c) 2024-2025 GeyserMC. http://geysermc.org * * Permission is hereby granted, free of charge, to any person obtaining a copy * of this software and associated documentation files (the "Software"), to deal @@ -29,7 +29,6 @@ import org.cloudburstmc.protocol.bedrock.data.inventory.crafting.recipe.MultiRec import org.cloudburstmc.protocol.bedrock.data.inventory.crafting.recipe.RecipeData; import org.cloudburstmc.protocol.bedrock.packet.CraftingDataPacket; import org.cloudburstmc.protocol.bedrock.packet.PlayerListPacket; -import org.geysermc.geyser.entity.type.player.PlayerEntity; import org.geysermc.geyser.registry.Registries; import org.geysermc.geyser.session.GeyserSession; import org.geysermc.geyser.translator.protocol.PacketTranslator; @@ -59,9 +58,9 @@ public class JavaFinishConfigurationTranslator extends PacketTranslator entries = new ArrayList<>(); - for (PlayerEntity otherEntity : session.getEntityCache().getAllPlayerEntities()) { - entries.add(new PlayerListPacket.Entry(otherEntity.getTabListUuid())); - } + session.getEntityCache().forEachPlayerEntity(otherPlayer -> { + entries.add(new PlayerListPacket.Entry(otherPlayer.getTabListUuid())); + }); PlayerListUtils.batchSendPlayerList(session, entries, PlayerListPacket.Action.REMOVE); session.getEntityCache().removeAllPlayerEntities();