Skip to content

Add CivScheduler facade and migrate civmodcore sites #3

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
grepsedawk wants to merge 3 commits into from
Closed

Add CivScheduler facade and migrate civmodcore sites #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
359db5e
Add CivScheduler facade and migrate own sites
grepsedawk May 30, 2026
6033343
Make migrated sites thread-safe; fix two bugs
grepsedawk May 30, 2026
4ef2f99
Add runEntity convenience overload
grepsedawk May 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions plugins/civmodcore-paper/src/main/java/vg/civcraft/mc/civmodcore/chat/dialog/Dialog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

import com.google.common.base.Preconditions;
import java.util.List;
import org.bukkit.Bukkit;
import org.bukkit.conversations.Conversation;
import org.bukkit.conversations.ConversationContext;
import org.bukkit.conversations.ConversationFactory;
Expand All @@ -11,6 +10,7 @@
import org.bukkit.entity.Player;
import org.bukkit.plugin.Plugin;
import vg.civcraft.mc.civmodcore.CivModCorePlugin;
import vg.civcraft.mc.civmodcore.scheduling.CivScheduler;

public abstract class Dialog {

Expand All @@ -33,7 +33,7 @@ public Dialog(final Player player, final Plugin plugin, final String prompt) {
Preconditions.checkNotNull(player, "Player cannot be null!");
Preconditions.checkNotNull(plugin, "Plugin cannot be null!");
this.player = player;
Bukkit.getScheduler().runTask(plugin, (Runnable) player::closeInventory);
CivScheduler.runEntity(player, player::closeInventory);
this.conversation = new ConversationFactory(plugin)
.withModality(false)
.withLocalEcho(false)
Expand Down
12 changes: 3 additions & 9 deletions ...odcore-paper/src/main/java/vg/civcraft/mc/civmodcore/inventory/gui/AnimatedClickable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,8 @@
import java.util.List;
import org.bukkit.entity.Player;
import org.bukkit.inventory.ItemStack;
import org.bukkit.scheduler.BukkitRunnable;
import org.bukkit.scheduler.BukkitTask;
import vg.civcraft.mc.civmodcore.CivModCorePlugin;
import vg.civcraft.mc.civmodcore.scheduling.CivScheduler;
import vg.civcraft.mc.civmodcore.scheduling.CivTask;

public class AnimatedClickable extends IClickable {

Expand Down Expand Up @@ -41,12 +40,7 @@ public ItemStack getItemStack() {
@Override
public void addedToInventory(final ClickableInventory inv, final int slot) {
// Schedule swapping out of item
BukkitTask task = new BukkitRunnable() {
@Override
public void run() {
inv.setItem(getNext(), slot);
}
}.runTaskTimer(CivModCorePlugin.getInstance(), timing, timing);
CivTask task = CivScheduler.runGlobalTimer(() -> inv.setItem(getNext(), slot), timing, timing);
inv.registerTask(task);
}

Expand Down
12 changes: 6 additions & 6 deletions ...dcore-paper/src/main/java/vg/civcraft/mc/civmodcore/inventory/gui/ClickableInventory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@
import org.bukkit.event.inventory.InventoryType;
import org.bukkit.inventory.Inventory;
import org.bukkit.inventory.ItemStack;
import org.bukkit.scheduler.BukkitTask;
import vg.civcraft.mc.civmodcore.CivModCorePlugin;
import vg.civcraft.mc.civmodcore.scheduling.CivScheduler;
import vg.civcraft.mc.civmodcore.scheduling.CivTask;

/**
* Represents an inventory filled with Clickables. Whenever one of those is
Expand Down Expand Up @@ -43,7 +43,7 @@ public class ClickableInventory {

private IClickable[] clickables;

private List<BukkitTask> runnables;
private List<CivTask> runnables;

private String name;
private Runnable onClose;
Expand Down Expand Up @@ -193,7 +193,7 @@ public void showInventory(Player p) {
public void showInventory(Player p, boolean eventSafe) {
if (p != null) {
if (eventSafe) {
Bukkit.getScheduler().runTask(CivModCorePlugin.getInstance(), () -> {
CivScheduler.runEntity(p, () -> {
p.openInventory(inventory);
openInventories.put(p.getUniqueId(), this);
});
Expand Down Expand Up @@ -245,7 +245,7 @@ public void setItem(ItemStack is, int slot) {
inventory.setItem(slot, is);
}

public void registerTask(BukkitTask runnable) {
public void registerTask(CivTask runnable) {
this.runnables.add(runnable);
}

Expand Down Expand Up @@ -310,7 +310,7 @@ private static void inventoryClosed(Player p, boolean force) {
private static void stopRunnables(ClickableInventory ci) {
if (ci.inventory.getViewers().size() == 1) {
// last one is closing
for (BukkitTask runnable : ci.runnables) {
for (CivTask runnable : ci.runnables) {
runnable.cancel();
}
}
Expand Down
11 changes: 7 additions & 4 deletions plugins/civmodcore-paper/src/main/java/vg/civcraft/mc/civmodcore/players/PlayerNames.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@

import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.stream.Stream;
import org.apache.commons.lang3.StringUtils;
import org.bukkit.Bukkit;
Expand All @@ -16,20 +16,23 @@
import org.bukkit.plugin.Plugin;
import org.jetbrains.annotations.ApiStatus;
import org.jetbrains.annotations.NotNull;
import vg.civcraft.mc.civmodcore.scheduling.CivScheduler;

public final class PlayerNames implements Listener {

private static final Set<String> names = new HashSet<>();
// Concurrent: the async-seed task writes on the global region thread while the login handler and external
// getPlayerNames() callers touch it from connection/region threads under Folia.
private static final Set<String> names = ConcurrentHashMap.newKeySet();

public PlayerNames(Plugin plugin) {
names.clear();
Bukkit.getScheduler().runTaskAsynchronously(plugin, () -> {
CivScheduler.runAsync(plugin, () -> {
OfflinePlayer[] players = Bukkit.getOfflinePlayers();
List<String> namesList = Stream.of(players)
.map(OfflinePlayer::getName)
.filter(StringUtils::isNotBlank)
.toList();
Bukkit.getScheduler().runTask(plugin, () -> {
CivScheduler.runGlobal(plugin, () -> {
names.addAll(namesList);
});
Comment on lines +35 to 37

@gemini-code-assist gemini-code-assist Bot May 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

On Folia, player login events and reads via getPlayerNames() can occur concurrently across different region threads. Since names is a non-thread-safe HashSet, modifying it here on the global region thread while other threads read or write to it will cause race conditions.

Please change names to a thread-safe set implementation, such as ConcurrentHashMap.newKeySet():

private static final Set<String> names = ConcurrentHashMap.newKeySet();

@grepsedawk grepsedawk May 30, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed real under Folia: the seed task writes via runGlobal on the global region thread, onLogin (PlayerLoginEvent, MONITOR) writes from the connection/login thread, and the public static getPlayerNames() is read from arbitrary threads. Replaced the HashSet with ConcurrentHashMap.newKeySet(). Set semantics are unchanged (the original was unordered, so no Navigable/sorted ops to preserve) and the names are non-null player-name Strings, so newKeySet is the correct choice over ConcurrentSkipListMap. Collections.unmodifiableSet still gives a thread-safe live view.

});
Expand Down
49 changes: 23 additions & 26 deletions ...e-paper/src/main/java/vg/civcraft/mc/civmodcore/players/scoreboard/bottom/BottomLine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,25 +3,27 @@
import java.util.Iterator;
import java.util.Map;
import java.util.Map.Entry;
import java.util.TreeMap;
import java.util.UUID;
import java.util.concurrent.ConcurrentHashMap;
import java.util.function.BiFunction;
import org.bukkit.Bukkit;
import org.bukkit.entity.Player;
import org.bukkit.scheduler.BukkitRunnable;
import vg.civcraft.mc.civmodcore.CivModCorePlugin;
import vg.civcraft.mc.civmodcore.scheduling.CivScheduler;
import vg.civcraft.mc.civmodcore.scheduling.CivTask;

public class BottomLine implements Comparable<BottomLine> {

private Map<UUID, String> texts;
private String identifier;
private BukkitRunnable updater;
private CivTask updater;
private int priority;

BottomLine(String identifier, int priority) {
this.identifier = identifier;
this.priority = priority;
this.texts = new TreeMap<>();
// Mutated by event handlers on region/entity threads while the updatePeriodically task iterates it
// on the global thread under Folia; UUID keys are never sorted, so a hash map is enough.
this.texts = new ConcurrentHashMap<>();
}

public String getIdentifier() {
Expand All @@ -44,30 +46,25 @@ public void updatePeriodically(BiFunction<Player, String, String> updateFunction
if (updater != null) {
updater.cancel();
}
updater = new BukkitRunnable() {

@Override
public void run() {
Iterator<Entry<UUID, String>> iter = texts.entrySet().iterator();
while (iter.hasNext()) {
Entry<UUID, String> entry = iter.next();
Player player = Bukkit.getPlayer(entry.getKey());
if (player != null) {
String newText = updateFunction.apply(player, entry.getValue());
if (newText == null) {
iter.remove();
BottomLineAPI.refreshIndividually(player.getUniqueId());
continue;
}
if (!newText.equals(entry.getValue())) {
entry.setValue(newText);
BottomLineAPI.refreshIndividually(player.getUniqueId());
}
updater = CivScheduler.runGlobalTimer(() -> {

@gemini-code-assist gemini-code-assist Bot May 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

On Folia, player data updates and removals occur on region-specific threads, while this periodic updater runs on the global region thread. Since texts is a non-thread-safe TreeMap, iterating over it here while other threads concurrently call updatePlayer or removePlayer will cause race conditions and ConcurrentModificationExceptions.

Please change texts to a thread-safe map, such as ConcurrentHashMap:

this.texts = new ConcurrentHashMap<>();

(Note: Since UUID keys do not require sorting, ConcurrentHashMap is a perfect high-performance replacement here.)

@grepsedawk grepsedawk May 30, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied. texts is now a ConcurrentHashMap. The map is written by updatePlayer/removePlayer from event handlers (citadel/bastion ModeListener, finale cooldown listeners, finale AsyncPacketHandler) which run on region/entity/async threads under Folia, while updatePeriodically's task iterates and structurally modifies it on the global thread via getGlobalRegionScheduler — a real cross-thread race on a plain TreeMap. UUID keys are non-null and no Navigable/sorted-order ops are used (refreshAll copies into its own separate map), so ConcurrentHashMap suffices over ConcurrentSkipListMap.

Iterator<Entry<UUID, String>> iter = texts.entrySet().iterator();
while (iter.hasNext()) {
Entry<UUID, String> entry = iter.next();
Player player = Bukkit.getPlayer(entry.getKey());
if (player != null) {
String newText = updateFunction.apply(player, entry.getValue());
if (newText == null) {
iter.remove();
BottomLineAPI.refreshIndividually(player.getUniqueId());
continue;
}
if (!newText.equals(entry.getValue())) {
entry.setValue(newText);
BottomLineAPI.refreshIndividually(player.getUniqueId());
}
}
}
};
updater.runTaskTimer(CivModCorePlugin.getInstance(), delay, delay);
}, delay, delay);
}

public void removePlayer(Player player) {
Expand Down
12 changes: 2 additions & 10 deletions ...aper/src/main/java/vg/civcraft/mc/civmodcore/players/scoreboard/bottom/BottomLineAPI.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,23 +9,15 @@
import org.bukkit.Bukkit;
import org.bukkit.ChatColor;
import org.bukkit.entity.Player;
import org.bukkit.scheduler.BukkitRunnable;
import vg.civcraft.mc.civmodcore.CivModCorePlugin;
import vg.civcraft.mc.civmodcore.scheduling.CivScheduler;

public final class BottomLineAPI {

private static Set<BottomLine> lines = new TreeSet<>();
private static final String SEPARATOR = ChatColor.BOLD + " " + ChatColor.BLACK + "|| " + ChatColor.RESET;

public static void init() {
BukkitRunnable run = new BukkitRunnable() {

@Override
public void run() {
refreshAll();
}
};
run.runTaskTimer(CivModCorePlugin.getInstance(), 15, 15);
CivScheduler.runGlobalTimer(BottomLineAPI::refreshAll, 15, 15);
}

public static BottomLine createBottomLine(String identifier, int priority) {
Expand Down
48 changes: 22 additions & 26 deletions ...-paper/src/main/java/vg/civcraft/mc/civmodcore/players/scoreboard/side/CivScoreBoard.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,27 +3,28 @@
import java.util.Iterator;
import java.util.Map;
import java.util.Map.Entry;
import java.util.TreeMap;
import java.util.UUID;
import java.util.concurrent.ConcurrentHashMap;
import java.util.function.BiFunction;
import org.bukkit.Bukkit;
import org.bukkit.entity.Player;
import org.bukkit.scheduler.BukkitRunnable;
import org.bukkit.scoreboard.DisplaySlot;
import org.bukkit.scoreboard.Objective;
import org.bukkit.scoreboard.Score;
import org.bukkit.scoreboard.Scoreboard;
import vg.civcraft.mc.civmodcore.CivModCorePlugin;
import vg.civcraft.mc.civmodcore.scheduling.CivScheduler;
import vg.civcraft.mc.civmodcore.scheduling.CivTask;

public class CivScoreBoard {

private String scoreName;
private Map<UUID, String> currentScoreText;
private BukkitRunnable updater;
private CivTask updater;

CivScoreBoard(String scoreName) {
this.scoreName = scoreName;
this.currentScoreText = new TreeMap<>();
// Updater runs on the global region thread while set/hide/purge mutate from player region threads on Folia.
this.currentScoreText = new ConcurrentHashMap<>();
}

public String getName() {
Expand All @@ -34,30 +35,25 @@ public void updatePeriodically(BiFunction<Player, String, String> updateFunction
if (updater != null) {
updater.cancel();
}
updater = new BukkitRunnable() {

@Override
public void run() {
Iterator<Entry<UUID, String>> iter = currentScoreText.entrySet().iterator();
while (iter.hasNext()) {
Entry<UUID, String> entry = iter.next();
Player player = Bukkit.getPlayer(entry.getKey());
if (player != null) {
String newText = updateFunction.apply(player, entry.getValue());
if (newText == null) {
hideForPlayer(player);
iter.remove();
continue;
}
if (!newText.equals(entry.getValue())) {
internalUpdate(player, entry.getValue(), newText);
entry.setValue(newText);
}
updater = CivScheduler.runGlobalTimer(() -> {

@gemini-code-assist gemini-code-assist Bot May 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

On Folia, scoreboard updates (like set, hide, or purge) are called from region-specific threads, while this periodic updater runs on the global region thread. Since currentScoreText is a non-thread-safe TreeMap, iterating over it here while other threads concurrently modify it will cause race conditions and ConcurrentModificationExceptions.

Please change currentScoreText to a thread-safe map, such as ConcurrentHashMap:

this.currentScoreText = new ConcurrentHashMap<>();

(Note: Since UUID keys do not require sorting, ConcurrentHashMap is a perfect high-performance replacement here.)

@grepsedawk grepsedawk May 30, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and applied. currentScoreText is keyed by UUID (always p.getUniqueId(), non-null) and the code uses no Navigable/sorted-order operations, so ConcurrentHashMap is the right fit. The race is real on Folia: the migrated updatePeriodically now uses CivScheduler.runGlobalTimer (global region thread) and is live in production (finale CooldownHandler/PearlCoolDownListener), while set/hide are invoked from PlayerSetting.setValue listener callbacks and event-driven HUD updates that run on player region/entity threads — concurrent mutation of a plain TreeMap during iteration. Swapped TreeMap -> ConcurrentHashMap; its entrySet iterator still supports iter.remove() and entry.setValue() used by the updater.

Iterator<Entry<UUID, String>> iter = currentScoreText.entrySet().iterator();
while (iter.hasNext()) {
Entry<UUID, String> entry = iter.next();
Player player = Bukkit.getPlayer(entry.getKey());
if (player != null) {
String newText = updateFunction.apply(player, entry.getValue());
if (newText == null) {
hideForPlayer(player);
iter.remove();
continue;
}
if (!newText.equals(entry.getValue())) {
internalUpdate(player, entry.getValue(), newText);
entry.setValue(newText);
}
}
}
};
updater.runTaskTimer(CivModCorePlugin.getInstance(), delay, delay);
}, delay, delay);
}

public void set(Player p, String newText) {
Expand Down
Loading
Loading