Skip to content

Commit

Permalink
Fix data races in scheduler, plugin manager and player list (MockBukk…
Browse files Browse the repository at this point in the history 
…it#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
  • Loading branch information
@frengor
frengor authored Nov 10, 2024
1 parent c73ff04 commit 8e34a7b
Show file tree
Hide file tree
Showing 7 changed files with 141 additions and 141 deletions.
79 changes: 42 additions & 37 deletions src/main/java/org/mockbukkit/mockbukkit/PlayerListMock.java
View file
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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;

/**
Expand All @@ -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<PlayerMock> onlinePlayers = Collections.synchronizedSet(new LinkedHashSet<>());
private final Set<OfflinePlayer> offlinePlayers = Collections.synchronizedSet(new HashSet<>());
private final Map<UUID, Long> lastLogins = Collections.synchronizedMap(new HashMap<>());
private final Map<UUID, Long> lastSeen = Collections.synchronizedMap(new HashMap<>());
private final Map<UUID, Long> firstPlayed = Collections.synchronizedMap(new HashMap<>());
private final Map<UUID, Boolean> hasPlayedBefore = Collections.synchronizedMap(new HashMap<>());
// These fields must be accessed while synchronizing on PlayerListMock.this
private final Set<PlayerMock> onlinePlayers = new CopyOnWriteArraySet<>(); // Iterator safety in getOnlinePlayers() (from Spigot implementation)
private final Set<OfflinePlayer> offlinePlayers = new HashSet<>(); // CopyOnWriteArraySet is not needed here, since getOfflinePlayers() already returns a copy
private final Map<UUID, Long> lastLogins = new HashMap<>();
private final Map<UUID, Long> lastSeen = new HashMap<>();
private final Map<UUID, Long> firstPlayed = new HashMap<>();
private final Map<UUID, Boolean> hasPlayedBefore = new HashMap<>();
private final Set<UUID> operators = new HashSet<>();

private final @NotNull IpBanListMock ipBans = new IpBanListMock();
private final @NotNull ProfileBanListMock profileBans = new ProfileBanListMock();

private final Set<UUID> operators = Collections.synchronizedSet(new HashSet<>());

/**
* Sets the maximum number of online players.
* <b>This is not currently enforced.</b>
Expand Down Expand Up @@ -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()));
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
}
Expand All @@ -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);
}
Expand All @@ -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);
Expand All @@ -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())
Expand All @@ -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);
Expand All @@ -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);
}
Expand All @@ -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);
Expand All @@ -218,7 +221,7 @@ public void setLastLogin(UUID uuid, long lastLogin)
* @return All server operators.
*/
@NotNull
public Set<OfflinePlayer> getOperators()
public synchronized Set<OfflinePlayer> getOperators()
{
return this.operators.stream().map(this::getOfflinePlayer).collect(Collectors.toSet());
}
Expand All @@ -229,22 +232,24 @@ public Set<OfflinePlayer> getOperators()
@NotNull
public Collection<PlayerMock> 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);
}

/**
* @return All offline and online players.
*/
@NotNull
public OfflinePlayer @NotNull [] getOfflinePlayers()
public synchronized OfflinePlayer @NotNull [] getOfflinePlayers()
{
return this.offlinePlayers.toArray(new OfflinePlayer[0]);
}

/**
* @return Whether anyone is online.
*/
public boolean isSomeoneOnline()
public synchronized boolean isSomeoneOnline()
{
return !this.onlinePlayers.isEmpty();
}
Expand All @@ -256,7 +261,7 @@ public boolean isSomeoneOnline()
* @return All online players whose names start with the provided name.
*/
@NotNull
public List<Player> matchPlayer(@NotNull String name)
public synchronized List<Player> matchPlayer(@NotNull String name)
{
String nameLower = name.toLowerCase(Locale.ENGLISH);
return this.onlinePlayers.stream()
Expand All @@ -272,7 +277,7 @@ public List<Player> 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()
Expand All @@ -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);

Expand Down Expand Up @@ -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)
{
Expand All @@ -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);
}

/**
Expand All @@ -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)
Expand All @@ -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);

Expand All @@ -395,15 +400,15 @@ public OfflinePlayer getOfflinePlayer(@NotNull UUID id)
/**
* Clears all online players.
*/
public void clearOnlinePlayers()
public synchronized void clearOnlinePlayers()
{
this.onlinePlayers.clear();
}

/**
* Clears all offline players.
*/
public void clearOfflinePlayers()
public synchronized void clearOfflinePlayers()
{
this.offlinePlayers.clear();
}
Expand All @@ -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);
}
Expand All @@ -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);

Expand Down
26 changes: 13 additions & 13 deletions src/main/java/org/mockbukkit/mockbukkit/ban/IpBanListMock.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<String, BanEntry<InetAddress>> bans = new HashMap<>();
private static final String TARGET_CANNOT_BE_NULL = "Target cannot be null";

@Override
@Deprecated(since = "1.20")
public @Nullable BanEntry<InetAddress> getBanEntry(@NotNull String target)
public synchronized @Nullable BanEntry<InetAddress> getBanEntry(@NotNull String target)
{
Preconditions.checkNotNull(target, TARGET_CANNOT_BE_NULL);
return bans.getOrDefault(target, null);
}

@Override
public @Nullable BanEntry<InetAddress> getBanEntry(@NotNull InetAddress target)
public synchronized @Nullable BanEntry<InetAddress> getBanEntry(@NotNull InetAddress target)
{
Preconditions.checkNotNull(target, TARGET_CANNOT_BE_NULL);
return bans.getOrDefault(InetAddresses.toAddrString(target), null);
}

@Override

public @Nullable BanEntry<InetAddress> addBan(@NotNull String target, @Nullable String reason, @Nullable Date expires, @Nullable String source)
public synchronized @Nullable BanEntry<InetAddress> addBan(@NotNull String target, @Nullable String reason, @Nullable Date expires, @Nullable String source)
{
Preconditions.checkNotNull(target, TARGET_CANNOT_BE_NULL);
BanEntry<InetAddress> entry = new IpBanEntryMock(
Expand All @@ -53,20 +53,20 @@ public class IpBanListMock implements org.bukkit.ban.IpBanList
}

@Override
public @Nullable BanEntry<InetAddress> addBan(@NotNull InetAddress target, @Nullable String reason, @Nullable Date expires, @Nullable String source)
public synchronized @Nullable BanEntry<InetAddress> 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<InetAddress> addBan(@NotNull InetAddress target, @Nullable String reason, @Nullable Instant expires, @Nullable String source)
public synchronized @Nullable BanEntry<InetAddress> 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<InetAddress> addBan(@NotNull InetAddress target, @Nullable String reason, @Nullable Duration duration, @Nullable String source)
public synchronized @Nullable BanEntry<InetAddress> 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);
Expand All @@ -75,7 +75,7 @@ public class IpBanListMock implements org.bukkit.ban.IpBanList
@Override
@Deprecated(since = "1.20")
@SuppressWarnings("rawtypes")
public @NotNull Set<BanEntry> getBanEntries()
public synchronized @NotNull Set<BanEntry> getBanEntries()
{
ImmutableSet.Builder<BanEntry> builder = ImmutableSet.builder();
for (String target : bans.keySet())
Expand All @@ -90,7 +90,7 @@ public class IpBanListMock implements org.bukkit.ban.IpBanList
}

@Override
public @NotNull Set<BanEntry<InetAddress>> getEntries()
public synchronized @NotNull Set<BanEntry<InetAddress>> getEntries()
{
ImmutableSet.Builder<BanEntry<InetAddress>> builder = ImmutableSet.builder();
for (String target : bans.keySet())
Expand All @@ -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);
Expand Down
Loading

0 comments on commit 8e34a7b

Please sign in to comment.