From 8e34a7bc5497c26214a6c0c2bd1ce8f5fbb1db7e Mon Sep 17 00:00:00 2001 From: fren_gor Date: Sun, 10 Nov 2024 09:22:28 +0100 Subject: [PATCH] Fix data races in scheduler, plugin manager and player list (#1183) * Fix scheduler data races * Use HandlerList.unregisterAll to unregister plugin events * Fix plugin manager data races * Fix player list race conditions * Make internal methods protected and @ApiStatus.Internal --- .../mockbukkit/mockbukkit/PlayerListMock.java | 79 +++++++------- .../mockbukkit/ban/IpBanListMock.java | 26 ++--- .../mockbukkit/ban/ProfileBanListMock.java | 22 ++-- .../mockbukkit/plugin/PluginManagerMock.java | 102 +++++++----------- .../scheduler/BukkitSchedulerMock.java | 31 ++++-- .../mockbukkit/scheduler/RepeatingTask.java | 4 +- .../mockbukkit/scheduler/ScheduledTask.java | 18 ++-- 7 files changed, 141 insertions(+), 141 deletions(-) diff --git a/src/main/java/org/mockbukkit/mockbukkit/PlayerListMock.java b/src/main/java/org/mockbukkit/mockbukkit/PlayerListMock.java index b1368632a2..b6f64fefa9 100644 --- a/src/main/java/org/mockbukkit/mockbukkit/PlayerListMock.java +++ b/src/main/java/org/mockbukkit/mockbukkit/PlayerListMock.java @@ -1,5 +1,6 @@ package org.mockbukkit.mockbukkit; +import com.google.common.collect.Iterators; import org.mockbukkit.mockbukkit.ban.IpBanListMock; import org.mockbukkit.mockbukkit.ban.ProfileBanListMock; import org.mockbukkit.mockbukkit.entity.OfflinePlayerMock; @@ -15,12 +16,12 @@ import java.util.Collections; import java.util.HashMap; import java.util.HashSet; -import java.util.LinkedHashSet; import java.util.List; import java.util.Locale; import java.util.Map; import java.util.Set; import java.util.UUID; +import java.util.concurrent.CopyOnWriteArraySet; import java.util.stream.Collectors; /** @@ -29,20 +30,21 @@ public class PlayerListMock { + // Remember to properly synchronize accesses to this field when this setting will be enforced private int maxPlayers = Integer.MAX_VALUE; - private final Set onlinePlayers = Collections.synchronizedSet(new LinkedHashSet<>()); - private final Set offlinePlayers = Collections.synchronizedSet(new HashSet<>()); - private final Map lastLogins = Collections.synchronizedMap(new HashMap<>()); - private final Map lastSeen = Collections.synchronizedMap(new HashMap<>()); - private final Map firstPlayed = Collections.synchronizedMap(new HashMap<>()); - private final Map hasPlayedBefore = Collections.synchronizedMap(new HashMap<>()); + // These fields must be accessed while synchronizing on PlayerListMock.this + private final Set onlinePlayers = new CopyOnWriteArraySet<>(); // Iterator safety in getOnlinePlayers() (from Spigot implementation) + private final Set offlinePlayers = new HashSet<>(); // CopyOnWriteArraySet is not needed here, since getOfflinePlayers() already returns a copy + private final Map lastLogins = new HashMap<>(); + private final Map lastSeen = new HashMap<>(); + private final Map firstPlayed = new HashMap<>(); + private final Map hasPlayedBefore = new HashMap<>(); + private final Set operators = new HashSet<>(); private final @NotNull IpBanListMock ipBans = new IpBanListMock(); private final @NotNull ProfileBanListMock profileBans = new ProfileBanListMock(); - private final Set operators = Collections.synchronizedSet(new HashSet<>()); - /** * Sets the maximum number of online players. * This is not currently enforced. @@ -87,10 +89,11 @@ public ProfileBanListMock getProfileBans() * @param player The player to add. */ @ApiStatus.Internal - public void addPlayer(@NotNull PlayerMock player) + public synchronized void addPlayer(@NotNull PlayerMock player) { - this.firstPlayed.putIfAbsent(player.getUniqueId(), System.currentTimeMillis()); - this.lastLogins.put(player.getUniqueId(), System.currentTimeMillis()); + long currentTime = System.currentTimeMillis(); + this.firstPlayed.putIfAbsent(player.getUniqueId(), currentTime); + this.lastLogins.put(player.getUniqueId(), currentTime); this.onlinePlayers.add(player); this.offlinePlayers.add(player); this.hasPlayedBefore.put(player.getUniqueId(), this.hasPlayedBefore.containsKey(player.getUniqueId())); @@ -102,7 +105,7 @@ public void addPlayer(@NotNull PlayerMock player) * @param player The player to disconnect. */ @ApiStatus.Internal - public void disconnectPlayer(@NotNull PlayerMock player) + public synchronized void disconnectPlayer(@NotNull PlayerMock player) { this.lastSeen.put(player.getUniqueId(), System.currentTimeMillis()); this.onlinePlayers.remove(player); @@ -116,7 +119,7 @@ public void disconnectPlayer(@NotNull PlayerMock player) * @return Whether the player has played before. * @see Player#hasPlayedBefore() */ - public boolean hasPlayedBefore(@NotNull UUID uuid) + public synchronized boolean hasPlayedBefore(@NotNull UUID uuid) { Preconditions.checkNotNull(uuid, "UUID cannot be null"); return this.hasPlayedBefore.getOrDefault(uuid, false); @@ -128,7 +131,7 @@ public boolean hasPlayedBefore(@NotNull UUID uuid) * @param player The player. */ @ApiStatus.Internal - public void addOfflinePlayer(@NotNull OfflinePlayer player) + public synchronized void addOfflinePlayer(@NotNull OfflinePlayer player) { this.offlinePlayers.add(player); } @@ -140,7 +143,7 @@ public void addOfflinePlayer(@NotNull OfflinePlayer player) * @return The time of first log-in, or 0. * @see OfflinePlayer#getFirstPlayed() */ - public long getFirstPlayed(UUID uuid) + public synchronized long getFirstPlayed(UUID uuid) { return this.firstPlayed.getOrDefault(uuid, 0L); } @@ -151,7 +154,7 @@ public long getFirstPlayed(UUID uuid) * @param uuid UUID of the player to set first played time for. * @param firstPlayed The first played time. Must be non-negative. */ - public void setFirstPlayed(UUID uuid, long firstPlayed) + public synchronized void setFirstPlayed(UUID uuid, long firstPlayed) { Preconditions.checkArgument(firstPlayed > 0, "First played time must be non-negative"); this.firstPlayed.put(uuid, firstPlayed); @@ -165,7 +168,7 @@ public void setFirstPlayed(UUID uuid, long firstPlayed) * @return The last time the player was seen online. * @see OfflinePlayer#getLastSeen() */ - public long getLastSeen(UUID uuid) + public synchronized long getLastSeen(UUID uuid) { OfflinePlayer player = getOfflinePlayer(uuid); if (player != null && player.isOnline()) @@ -182,7 +185,7 @@ public long getLastSeen(UUID uuid) * @param uuid UUID of the player to set last seen time for. * @param lastSeen The last seen time. Must be non-negative. */ - public void setLastSeen(UUID uuid, long lastSeen) + public synchronized void setLastSeen(UUID uuid, long lastSeen) { Preconditions.checkArgument(lastSeen > 0, "Last seen time must be non-negative"); this.lastSeen.put(uuid, lastSeen); @@ -196,7 +199,7 @@ public void setLastSeen(UUID uuid, long lastSeen) * @return The last time the player was seen online. * @see OfflinePlayer#getLastLogin() */ - public long getLastLogin(UUID uuid) + public synchronized long getLastLogin(UUID uuid) { return this.lastLogins.getOrDefault(uuid, 0L); } @@ -207,7 +210,7 @@ public long getLastLogin(UUID uuid) * @param uuid UUID of the player to set last login time for. * @param lastLogin The last login time. Must be non-negative. */ - public void setLastLogin(UUID uuid, long lastLogin) + public synchronized void setLastLogin(UUID uuid, long lastLogin) { Preconditions.checkArgument(lastLogin > 0, "Last login time must be non-negative"); this.lastLogins.put(uuid, lastLogin); @@ -218,7 +221,7 @@ public void setLastLogin(UUID uuid, long lastLogin) * @return All server operators. */ @NotNull - public Set getOperators() + public synchronized Set getOperators() { return this.operators.stream().map(this::getOfflinePlayer).collect(Collectors.toSet()); } @@ -229,6 +232,8 @@ public Set getOperators() @NotNull public Collection getOnlinePlayers() { + // No need to synchronize here, since onlinePlayers is already thread-safe + // Also, we're not accessing the data structure here, but just returning it return Collections.unmodifiableSet(this.onlinePlayers); } @@ -236,7 +241,7 @@ public Collection getOnlinePlayers() * @return All offline and online players. */ @NotNull - public OfflinePlayer @NotNull [] getOfflinePlayers() + public synchronized OfflinePlayer @NotNull [] getOfflinePlayers() { return this.offlinePlayers.toArray(new OfflinePlayer[0]); } @@ -244,7 +249,7 @@ public Collection getOnlinePlayers() /** * @return Whether anyone is online. */ - public boolean isSomeoneOnline() + public synchronized boolean isSomeoneOnline() { return !this.onlinePlayers.isEmpty(); } @@ -256,7 +261,7 @@ public boolean isSomeoneOnline() * @return All online players whose names start with the provided name. */ @NotNull - public List matchPlayer(@NotNull String name) + public synchronized List matchPlayer(@NotNull String name) { String nameLower = name.toLowerCase(Locale.ENGLISH); return this.onlinePlayers.stream() @@ -272,7 +277,7 @@ public List matchPlayer(@NotNull String name) * @return The player with the exact name provided, or null. */ @Nullable - public Player getPlayerExact(@NotNull String name) + public synchronized Player getPlayerExact(@NotNull String name) { String nameLower = name.toLowerCase(Locale.ENGLISH); return this.onlinePlayers.stream() @@ -287,7 +292,7 @@ public Player getPlayerExact(@NotNull String name) * @return The closest matching player. */ @Nullable - public Player getPlayer(@NotNull String name) + public synchronized Player getPlayer(@NotNull String name) { Player player = getPlayerExact(name); @@ -321,7 +326,7 @@ public Player getPlayer(@NotNull String name) * @return The player with the provided UUID, or null. */ @Nullable - public Player getPlayer(@NotNull UUID id) + public synchronized Player getPlayer(@NotNull UUID id) { for (Player player : this.onlinePlayers) { @@ -341,9 +346,9 @@ public Player getPlayer(@NotNull UUID id) * @return The player at the provided index. */ @NotNull - public PlayerMock getPlayer(int index) + public synchronized PlayerMock getPlayer(int index) { - return List.copyOf(this.onlinePlayers).get(index); + return Iterators.get(this.onlinePlayers.iterator(), index); } /** @@ -354,7 +359,7 @@ public PlayerMock getPlayer(int index) * @see #getPlayer(String) */ @NotNull - public OfflinePlayer getOfflinePlayer(@NotNull String name) + public synchronized OfflinePlayer getOfflinePlayer(@NotNull String name) { OfflinePlayer offlinePlayer = getOfflinePlayerIfCached(name); if (offlinePlayer != null) @@ -372,7 +377,7 @@ public OfflinePlayer getOfflinePlayer(@NotNull String name) * @see #getPlayer(UUID) */ @Nullable - public OfflinePlayer getOfflinePlayer(@NotNull UUID id) + public synchronized OfflinePlayer getOfflinePlayer(@NotNull UUID id) { Player player = getPlayer(id); @@ -395,7 +400,7 @@ public OfflinePlayer getOfflinePlayer(@NotNull UUID id) /** * Clears all online players. */ - public void clearOnlinePlayers() + public synchronized void clearOnlinePlayers() { this.onlinePlayers.clear(); } @@ -403,7 +408,7 @@ public void clearOnlinePlayers() /** * Clears all offline players. */ - public void clearOfflinePlayers() + public synchronized void clearOfflinePlayers() { this.offlinePlayers.clear(); } @@ -413,7 +418,7 @@ public void clearOfflinePlayers() * * @param operator The {@link UUID} of the Operator to add. */ - public void addOperator(UUID operator) + public synchronized void addOperator(UUID operator) { this.operators.add(operator); } @@ -423,12 +428,12 @@ public void addOperator(UUID operator) * * @param operator The {@link UUID} of the Operator to remove. */ - public void removeOperator(UUID operator) + public synchronized void removeOperator(UUID operator) { this.operators.remove(operator); } - public @Nullable OfflinePlayer getOfflinePlayerIfCached(String name) + public synchronized @Nullable OfflinePlayer getOfflinePlayerIfCached(String name) { Player player = getPlayer(name); diff --git a/src/main/java/org/mockbukkit/mockbukkit/ban/IpBanListMock.java b/src/main/java/org/mockbukkit/mockbukkit/ban/IpBanListMock.java index 7a324cc2b6..8ff1dc4c9c 100644 --- a/src/main/java/org/mockbukkit/mockbukkit/ban/IpBanListMock.java +++ b/src/main/java/org/mockbukkit/mockbukkit/ban/IpBanListMock.java @@ -18,27 +18,27 @@ public class IpBanListMock implements org.bukkit.ban.IpBanList { + // Accesses to this field must be done while synchronizing on IpBanListMock.this private final Map> bans = new HashMap<>(); private static final String TARGET_CANNOT_BE_NULL = "Target cannot be null"; @Override @Deprecated(since = "1.20") - public @Nullable BanEntry getBanEntry(@NotNull String target) + public synchronized @Nullable BanEntry getBanEntry(@NotNull String target) { Preconditions.checkNotNull(target, TARGET_CANNOT_BE_NULL); return bans.getOrDefault(target, null); } @Override - public @Nullable BanEntry getBanEntry(@NotNull InetAddress target) + public synchronized @Nullable BanEntry getBanEntry(@NotNull InetAddress target) { Preconditions.checkNotNull(target, TARGET_CANNOT_BE_NULL); return bans.getOrDefault(InetAddresses.toAddrString(target), null); } @Override - - public @Nullable BanEntry addBan(@NotNull String target, @Nullable String reason, @Nullable Date expires, @Nullable String source) + public synchronized @Nullable BanEntry addBan(@NotNull String target, @Nullable String reason, @Nullable Date expires, @Nullable String source) { Preconditions.checkNotNull(target, TARGET_CANNOT_BE_NULL); BanEntry entry = new IpBanEntryMock( @@ -53,20 +53,20 @@ public class IpBanListMock implements org.bukkit.ban.IpBanList } @Override - public @Nullable BanEntry addBan(@NotNull InetAddress target, @Nullable String reason, @Nullable Date expires, @Nullable String source) + public synchronized @Nullable BanEntry addBan(@NotNull InetAddress target, @Nullable String reason, @Nullable Date expires, @Nullable String source) { return addBan(InetAddresses.toAddrString(target), reason, expires, source); } @Override - public @Nullable BanEntry addBan(@NotNull InetAddress target, @Nullable String reason, @Nullable Instant expires, @Nullable String source) + public synchronized @Nullable BanEntry addBan(@NotNull InetAddress target, @Nullable String reason, @Nullable Instant expires, @Nullable String source) { Date date = expires != null ? Date.from(expires) : null; return this.addBan(target, reason, date, source); } @Override - public @Nullable BanEntry addBan(@NotNull InetAddress target, @Nullable String reason, @Nullable Duration duration, @Nullable String source) + public synchronized @Nullable BanEntry addBan(@NotNull InetAddress target, @Nullable String reason, @Nullable Duration duration, @Nullable String source) { Instant instant = duration != null ? Instant.now().plus(duration) : null; return this.addBan(target, reason, instant, source); @@ -75,7 +75,7 @@ public class IpBanListMock implements org.bukkit.ban.IpBanList @Override @Deprecated(since = "1.20") @SuppressWarnings("rawtypes") - public @NotNull Set getBanEntries() + public synchronized @NotNull Set getBanEntries() { ImmutableSet.Builder builder = ImmutableSet.builder(); for (String target : bans.keySet()) @@ -90,7 +90,7 @@ public class IpBanListMock implements org.bukkit.ban.IpBanList } @Override - public @NotNull Set> getEntries() + public synchronized @NotNull Set> getEntries() { ImmutableSet.Builder> builder = ImmutableSet.builder(); for (String target : bans.keySet()) @@ -105,26 +105,26 @@ public class IpBanListMock implements org.bukkit.ban.IpBanList } @Override - public boolean isBanned(@NotNull InetAddress target) + public synchronized boolean isBanned(@NotNull InetAddress target) { return this.bans.values().stream().anyMatch(banEntry -> banEntry.getBanTarget().equals(target)); } @Override - public boolean isBanned(@NotNull String target) + public synchronized boolean isBanned(@NotNull String target) { return this.bans.values().stream().anyMatch(banEntry -> InetAddresses.toAddrString(banEntry.getBanTarget()).equals(target)); } @Override - public void pardon(@NotNull InetAddress target) + public synchronized void pardon(@NotNull InetAddress target) { Preconditions.checkNotNull(target, TARGET_CANNOT_BE_NULL); this.pardon(InetAddresses.toAddrString(target)); } @Override - public void pardon(@NotNull String target) + public synchronized void pardon(@NotNull String target) { Preconditions.checkNotNull(target, TARGET_CANNOT_BE_NULL); this.bans.remove(target); diff --git a/src/main/java/org/mockbukkit/mockbukkit/ban/ProfileBanListMock.java b/src/main/java/org/mockbukkit/mockbukkit/ban/ProfileBanListMock.java index 1b9276697f..fe6567ccf5 100644 --- a/src/main/java/org/mockbukkit/mockbukkit/ban/ProfileBanListMock.java +++ b/src/main/java/org/mockbukkit/mockbukkit/ban/ProfileBanListMock.java @@ -22,19 +22,20 @@ public class ProfileBanListMock implements ProfileBanList { + // Accesses to this field must be done while synchronizing on ProfileBanListMock.this private final Map> bans = new HashMap<>(); private static final String TARGET_CANNOT_BE_NULL = "Target cannot be null"; @Override @Deprecated(since = "1.20") - public @Nullable BanEntry getBanEntry(@NotNull String target) + public synchronized @Nullable BanEntry getBanEntry(@NotNull String target) { Preconditions.checkNotNull(target, TARGET_CANNOT_BE_NULL); return bans.getOrDefault(target, null); } @Override - public @Nullable BanEntry getBanEntry(@NotNull PlayerProfile target) + public synchronized @Nullable BanEntry getBanEntry(@NotNull PlayerProfile target) { Preconditions.checkNotNull(target, TARGET_CANNOT_BE_NULL); return bans.getOrDefault(target.getName(), null); @@ -75,7 +76,10 @@ public class ProfileBanListMock implements ProfileBanList (reason == null || reason.isBlank()) ? null : reason ); - this.bans.put(target.getName(), entry); + synchronized (this) + { + this.bans.put(target.getName(), entry); + } return entry; } @@ -136,7 +140,7 @@ public void pardon(org.bukkit.profile.@NotNull PlayerProfile target) @Override @Deprecated(since = "1.20") - public @NotNull Set getBanEntries() + public synchronized @NotNull Set getBanEntries() { ImmutableSet.Builder builder = ImmutableSet.builder(); for (BanEntry ban : bans.values()) @@ -147,7 +151,7 @@ public void pardon(org.bukkit.profile.@NotNull PlayerProfile target) } @Override - public @NotNull Set> getEntries() + public synchronized @NotNull Set> getEntries() { ImmutableSet.Builder> builder = ImmutableSet.builder(); for (BanEntry ban : bans.values()) @@ -158,26 +162,26 @@ public void pardon(org.bukkit.profile.@NotNull PlayerProfile target) } @Override - public boolean isBanned(@NotNull PlayerProfile target) + public synchronized boolean isBanned(@NotNull PlayerProfile target) { return this.bans.values().stream().anyMatch(banEntry -> banEntry.getBanTarget().equals(target)); } @Override - public boolean isBanned(@NotNull String target) + public synchronized boolean isBanned(@NotNull String target) { return this.bans.values().stream().anyMatch(banEntry -> Objects.equals(banEntry.getBanTarget().getName(), target)); } @Override - public void pardon(@NotNull PlayerProfile target) + public synchronized void pardon(@NotNull PlayerProfile target) { Preconditions.checkNotNull(target, TARGET_CANNOT_BE_NULL); this.bans.remove(target.getName()); } @Override - public void pardon(@NotNull String target) + public synchronized void pardon(@NotNull String target) { Preconditions.checkNotNull(target, TARGET_CANNOT_BE_NULL); this.bans.remove(target); diff --git a/src/main/java/org/mockbukkit/mockbukkit/plugin/PluginManagerMock.java b/src/main/java/org/mockbukkit/mockbukkit/plugin/PluginManagerMock.java index 12c55d7f6f..ec4ef431b7 100644 --- a/src/main/java/org/mockbukkit/mockbukkit/plugin/PluginManagerMock.java +++ b/src/main/java/org/mockbukkit/mockbukkit/plugin/PluginManagerMock.java @@ -49,13 +49,14 @@ import java.util.Collections; import java.util.Comparator; import java.util.Enumeration; -import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Map.Entry; import java.util.Random; import java.util.Set; +import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.ThreadLocalRandom; +import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; import java.util.function.Predicate; import java.util.jar.JarFile; @@ -74,11 +75,13 @@ public class PluginManagerMock extends PermissionManagerMock implements PluginMa private static final Pattern VALID_PLUGIN_NAMES = Pattern.compile("^[A-Za-z0-9_.-]+$"); private final @NotNull ServerMock server; + + // These fields must be accessed only while synchronizing on PluginManagerMock.this private final List plugins = new ArrayList<>(); private final List commands = new ArrayList<>(); - private final List events = new ArrayList<>(); - private File parentTemporaryDirectory; - private final @NotNull Map> listeners = new HashMap<>(); + + private final List events = new CopyOnWriteArrayList<>(); // CopyOnWriteArrayList is necessary to return a Stream in getFiredEvents() + private final AtomicReference parentTemporaryDirectory = new AtomicReference<>(); // Initialized once in getParentTemporaryDirectory() and read-only afterwards /** * Constructs a new {@link PluginManagerMock} for the provided {@link ServerMock}. @@ -98,6 +101,7 @@ public PluginManagerMock(@NotNull ServerMock server) */ public void unload() { + File parentTemporaryDirectory = this.parentTemporaryDirectory.get(); if (parentTemporaryDirectory == null) return; @@ -239,7 +243,7 @@ public void assertEventNotFired(@NotNull Class eventClass, @Nul } @Override - public Plugin getPlugin(@NotNull String name) + public synchronized Plugin getPlugin(@NotNull String name) { Preconditions.checkNotNull(name, "Name cannot be null"); for (Plugin plugin : plugins) @@ -253,7 +257,7 @@ public Plugin getPlugin(@NotNull String name) } @Override - public Plugin @NotNull [] getPlugins() + public synchronized Plugin @NotNull [] getPlugins() { return plugins.toArray(new Plugin[0]); } @@ -263,9 +267,9 @@ public Plugin getPlugin(@NotNull String name) * * @return A collection of all available commands. */ - public @NotNull Collection getCommands() + public synchronized @NotNull Collection getCommands() { - return Collections.unmodifiableList(commands); + return List.copyOf(commands); } /** @@ -335,10 +339,19 @@ private boolean isConstructorCompatible(@NotNull Constructor constructor, @No */ public @NotNull File getParentTemporaryDirectory() throws IOException { + File parentTemporaryDirectory = this.parentTemporaryDirectory.get(); if (parentTemporaryDirectory == null) { Random random = ThreadLocalRandom.current(); - parentTemporaryDirectory = Files.createTempDirectory("MockBukkit-" + random.nextInt(0, Integer.MAX_VALUE)).toFile(); + File tmpDir = Files.createTempDirectory("MockBukkit-" + random.nextInt(0, Integer.MAX_VALUE)).toFile(); + File prev = this.parentTemporaryDirectory.compareAndExchange(null, tmpDir); + if (prev != null) + { + // Another thread set parentTemporaryDirectory. Prev is the correct tmp directory + Files.delete(tmpDir.toPath()); + return prev; + } + return tmpDir; } return parentTemporaryDirectory; } @@ -384,8 +397,11 @@ private boolean isConstructorCompatible(@NotNull Constructor constructor, @No public void registerLoadedPlugin(@NotNull Plugin plugin) { Preconditions.checkNotNull(plugin, "Plugin cannot be null"); - addCommandsFrom(plugin); - plugins.add(plugin); + synchronized (this) + { + addCommandsFrom(plugin); + plugins.add(plugin); + } plugin.onLoad(); } @@ -652,7 +668,7 @@ else if (value != null) * * @param plugin The plugin from which to read commands. */ - protected void addCommandsFrom(@NotNull Plugin plugin) + protected synchronized void addCommandsFrom(@NotNull Plugin plugin) { Preconditions.checkNotNull(plugin, "Plugin cannot be null"); Map> pluginCommands = plugin.getDescription().getCommands(); @@ -679,33 +695,18 @@ public void registerInterface(@NotNull Class loader) thr public boolean isPluginEnabled(@NotNull String name) { Preconditions.checkNotNull(name, "Name cannot be null"); - boolean result = false; - - for (Plugin mockedPlugin : plugins) - { - if (mockedPlugin.getName().equals(name)) - { - result = mockedPlugin.isEnabled(); - } - } - - return result; + Plugin plugin = getPlugin(name); + return plugin != null && plugin.isEnabled(); } @Override - public boolean isPluginEnabled(@Nullable Plugin plugin) + public synchronized boolean isPluginEnabled(@Nullable Plugin plugin) { - boolean result = false; - - for (Plugin mockedPlugin : plugins) + if (plugin != null && plugins.contains(plugin)) { - if (mockedPlugin.equals(plugin)) - { - result = plugin.isEnabled(); - } + return plugin.isEnabled(); } - - return result; + return false; } @Override @@ -746,7 +747,7 @@ public Plugin[] loadPlugins(@NotNull File directory) @Override public void disablePlugins() { - for (Plugin plugin : plugins) + for (Plugin plugin : getPlugins()) disablePlugin(plugin); } @@ -754,7 +755,10 @@ public void disablePlugins() public void clearPlugins() { disablePlugins(); - plugins.clear(); + synchronized (this) + { + plugins.clear(); + } } /** @@ -776,24 +780,10 @@ public void registerEvents(@NotNull Listener listener, @NotNull Plugin plugin) { throw new IllegalPluginAccessException("Plugin attempted to register " + listener + " while not enabled"); } - addListener(listener, plugin); for (Map.Entry, Set> entry : plugin.getPluginLoader().createRegisteredListeners(listener, plugin).entrySet()) { getEventListeners(getRegistrationClass(entry.getKey())).registerAll(entry.getValue()); } - - } - - private void addListener(@NotNull Listener listener, @NotNull Plugin plugin) - { - Preconditions.checkNotNull(listener, "Listener cannot be null"); - Preconditions.checkNotNull(plugin, "Listener cannot be null"); - List l = listeners.getOrDefault(plugin.getName(), new ArrayList<>()); - if (!l.contains(listener)) - { - l.add(listener); - listeners.put(plugin.getName(), l); - } } /** @@ -804,18 +794,7 @@ private void addListener(@NotNull Listener listener, @NotNull Plugin plugin) public void unregisterPluginEvents(@NotNull Plugin plugin) { Preconditions.checkNotNull(plugin, "Listener cannot be null"); - List listListener = listeners.get(plugin.getName()); - if (listListener != null) - { - for (Listener l : listListener) - { - for (Map.Entry, Set> entry : plugin.getPluginLoader().createRegisteredListeners(l, plugin).entrySet()) - { - getEventListeners(getRegistrationClass(entry.getKey())).unregister(plugin); - } - } - } - + HandlerList.unregisterAll(plugin); } @Override @@ -838,7 +817,6 @@ public void registerEvent(@NotNull Class event, @NotNull Listen { throw new IllegalPluginAccessException("Plugin attempted to register " + event + " while not enabled"); } - addListener(listener, plugin); getEventListeners(event).register(new RegisteredListener(listener, executor, priority, plugin, ignoreCancelled)); } diff --git a/src/main/java/org/mockbukkit/mockbukkit/scheduler/BukkitSchedulerMock.java b/src/main/java/org/mockbukkit/mockbukkit/scheduler/BukkitSchedulerMock.java index 60d3f27822..039da745bd 100644 --- a/src/main/java/org/mockbukkit/mockbukkit/scheduler/BukkitSchedulerMock.java +++ b/src/main/java/org/mockbukkit/mockbukkit/scheduler/BukkitSchedulerMock.java @@ -13,7 +13,7 @@ import org.bukkit.scheduler.BukkitWorker; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; -import org.jetbrains.annotations.UnmodifiableView; +import org.jetbrains.annotations.Unmodifiable; import org.opentest4j.AssertionFailedError; import java.util.ArrayList; @@ -47,9 +47,9 @@ public class BukkitSchedulerMock implements BukkitScheduler 60L, TimeUnit.SECONDS, new SynchronousQueue<>()); private final ExecutorService asyncEventExecutor = Executors.newCachedThreadPool(); - private final List> queuedAsyncEvents = new ArrayList<>(); + private final List> queuedAsyncEvents = Collections.synchronizedList(new ArrayList<>()); private final TaskList scheduledTasks = new TaskList(); - private final List activeWorkers = new ArrayList<>(); + private final List activeWorkers = Collections.synchronizedList(new ArrayList<>()); private final AtomicReference asyncException = new AtomicReference<>(); // This variable must be accessed while synchronizing on BukkitSchedulerMock.this to avoid data races and race conditions @@ -57,7 +57,7 @@ public class BukkitSchedulerMock implements BukkitScheduler private final AtomicInteger id = new AtomicInteger(); private long executorTimeout = 60000; - private final List overdueTasks = new ArrayList<>(); + private volatile List overdueTasks = List.of(); // Always read-only. The reference is updated by saveOverdueTasks() private @NotNull Runnable wrapTask(@NotNull ScheduledTask task) { @@ -292,7 +292,13 @@ public void waitAsyncTasksFinished() */ public void waitAsyncEventsFinished() { - for (Future futureEvent : List.copyOf(queuedAsyncEvents)) + List> queuedAsyncEventsCopy; + synchronized (queuedAsyncEvents) + { + queuedAsyncEventsCopy = List.copyOf(queuedAsyncEvents); + } + + for (Future futureEvent : queuedAsyncEventsCopy) { if (futureEvent.isDone()) { @@ -472,7 +478,12 @@ public boolean isQueued(int taskId) @Override public @NotNull List getActiveWorkers() { - return this.activeWorkers; + synchronized (this.activeWorkers) + { + // Copying here is more efficient than using a CopyOnWriteArrayList, + // since this method is not called as much as wrapTask(...) + return List.copyOf(this.activeWorkers); + } } @Override @@ -603,16 +614,15 @@ protected int getActiveRunningCount() */ public void saveOverdueTasks() { - this.overdueTasks.clear(); - this.overdueTasks.addAll(getActiveWorkers()); + this.overdueTasks = getActiveWorkers(); } /** * @return A list of overdue tasks saved by {@link #saveOverdueTasks()}. */ - public @NotNull @UnmodifiableView List getOverdueTasks() + public @NotNull @Unmodifiable List getOverdueTasks() { - return Collections.unmodifiableList(this.overdueTasks); + return this.overdueTasks; } /** @@ -621,6 +631,7 @@ public void saveOverdueTasks() @Deprecated(forRemoval = true) public void assertNoOverdueTasks() { + List overdueTasks = this.overdueTasks; // Single read from volatile variable if (!overdueTasks.isEmpty()) throw new AssertionFailedError("There are overdue tasks: " + overdueTasks); } diff --git a/src/main/java/org/mockbukkit/mockbukkit/scheduler/RepeatingTask.java b/src/main/java/org/mockbukkit/mockbukkit/scheduler/RepeatingTask.java index 96e951d622..e7c01487ce 100644 --- a/src/main/java/org/mockbukkit/mockbukkit/scheduler/RepeatingTask.java +++ b/src/main/java/org/mockbukkit/mockbukkit/scheduler/RepeatingTask.java @@ -2,6 +2,7 @@ import org.bukkit.plugin.Plugin; import org.bukkit.scheduler.BukkitTask; +import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.NotNull; import java.util.function.Consumer; @@ -59,7 +60,8 @@ public long getPeriod() /** * Updates the scheduled tick for the next run. */ - public void updateScheduledTick() + @ApiStatus.Internal + protected void updateScheduledTick() { setScheduledTick(getScheduledTick() + period); } diff --git a/src/main/java/org/mockbukkit/mockbukkit/scheduler/ScheduledTask.java b/src/main/java/org/mockbukkit/mockbukkit/scheduler/ScheduledTask.java index faaf64df1d..336f4a80a4 100644 --- a/src/main/java/org/mockbukkit/mockbukkit/scheduler/ScheduledTask.java +++ b/src/main/java/org/mockbukkit/mockbukkit/scheduler/ScheduledTask.java @@ -8,6 +8,7 @@ import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import java.util.Collections; import java.util.LinkedList; import java.util.List; import java.util.concurrent.CancellationException; @@ -22,14 +23,14 @@ public class ScheduledTask implements BukkitTask, BukkitWorker private final int id; private final Plugin plugin; private final boolean isSync; - private boolean isCancelled = false; - private long scheduledTick; - private boolean running; + private volatile boolean isCancelled = false; + private volatile long scheduledTick; + private volatile boolean running; private final @Nullable Runnable runnable; private final @Nullable Consumer consumer; - private final List cancelListeners = new LinkedList<>(); - private Thread thread; - private boolean submitted = false; + private final List cancelListeners = Collections.synchronizedList(new LinkedList<>()); + private volatile Thread thread; + private volatile boolean submitted = false; /** * Constructs a new {@link ScheduledTask} with the provided parameters. @@ -82,7 +83,6 @@ private ScheduledTask(int id, Plugin plugin, boolean isSync, long scheduledTick, this.running = false; } - /** * @return Whether the task is running. */ @@ -98,12 +98,11 @@ public boolean isRunning() * @param running Whether the task is running. */ @ApiStatus.Internal - public void setRunning(boolean running) + protected void setRunning(boolean running) { this.running = running; } - /** * Get the tick at which the task is scheduled to run at. * @@ -119,6 +118,7 @@ public long getScheduledTick() * * @param scheduledTick The tick at which the task is scheduled to run at. */ + @ApiStatus.Internal protected void setScheduledTick(long scheduledTick) { this.scheduledTick = scheduledTick;