diff --git a/projects/common/src/main/java/dan200/computercraft/shared/peripheral/monitor/MonitorBlockEntity.java b/projects/common/src/main/java/dan200/computercraft/shared/peripheral/monitor/MonitorBlockEntity.java index ef12d415d..928590b49 100644 --- a/projects/common/src/main/java/dan200/computercraft/shared/peripheral/monitor/MonitorBlockEntity.java +++ b/projects/common/src/main/java/dan200/computercraft/shared/peripheral/monitor/MonitorBlockEntity.java @@ -6,6 +6,7 @@ package dan200.computercraft.shared.peripheral.monitor; import com.google.common.annotations.VisibleForTesting; import dan200.computercraft.annotations.ForgeOverride; +import dan200.computercraft.api.peripheral.AttachedComputerSet; import dan200.computercraft.api.peripheral.IComputerAccess; import dan200.computercraft.api.peripheral.IPeripheral; import dan200.computercraft.core.terminal.Terminal; @@ -25,9 +26,6 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import javax.annotation.Nullable; -import java.util.Collections; -import java.util.Set; -import java.util.concurrent.ConcurrentHashMap; import java.util.function.Consumer; public class MonitorBlockEntity extends BlockEntity { @@ -53,7 +51,7 @@ public class MonitorBlockEntity extends BlockEntity { private @Nullable ClientMonitor clientMonitor; private @Nullable MonitorPeripheral peripheral; - private final Set computers = Collections.newSetFromMap(new ConcurrentHashMap<>()); + private final AttachedComputerSet computers = new AttachedComputerSet(); private boolean needsUpdate = false; private boolean needsValidating = false; @@ -487,7 +485,7 @@ public class MonitorBlockEntity extends BlockEntity { var monitor = getLoadedMonitor(x, y).getMonitor(); if (monitor == null) continue; - for (var computer : monitor.computers) fun.accept(computer); + computers.forEach(fun); } } } diff --git a/projects/common/src/main/java/dan200/computercraft/shared/peripheral/speaker/SpeakerPeripheral.java b/projects/common/src/main/java/dan200/computercraft/shared/peripheral/speaker/SpeakerPeripheral.java index 96405f1d9..54b8d6e07 100644 --- a/projects/common/src/main/java/dan200/computercraft/shared/peripheral/speaker/SpeakerPeripheral.java +++ b/projects/common/src/main/java/dan200/computercraft/shared/peripheral/speaker/SpeakerPeripheral.java @@ -4,11 +4,11 @@ package dan200.computercraft.shared.peripheral.speaker; -import com.google.errorprone.annotations.concurrent.GuardedBy; import dan200.computercraft.api.lua.ILuaContext; import dan200.computercraft.api.lua.LuaException; import dan200.computercraft.api.lua.LuaFunction; import dan200.computercraft.api.lua.LuaTable; +import dan200.computercraft.api.peripheral.AttachedComputerSet; import dan200.computercraft.api.peripheral.IComputerAccess; import dan200.computercraft.api.peripheral.IPeripheral; import dan200.computercraft.core.util.Nullability; @@ -33,7 +33,10 @@ import net.minecraft.world.item.RecordItem; import net.minecraft.world.level.block.state.properties.NoteBlockInstrument; import javax.annotation.Nullable; -import java.util.*; +import java.util.ArrayList; +import java.util.List; +import java.util.Optional; +import java.util.UUID; import static dan200.computercraft.api.lua.LuaValues.checkFinite; @@ -60,7 +63,7 @@ public abstract class SpeakerPeripheral implements IPeripheral { public static final int SAMPLE_RATE = 48000; private final UUID source = UUID.randomUUID(); - private final @GuardedBy("computers") Set computers = new HashSet<>(); + private final AttachedComputerSet computers = new AttachedComputerSet(); private long clock = 0; private long lastPositionTime; @@ -140,11 +143,7 @@ public abstract class SpeakerPeripheral implements IPeripheral { syncedPosition(position); // And notify computers that we have space for more audio. - synchronized (computers) { - for (var computer : computers) { - computer.queueEvent("speaker_audio_empty", computer.getAttachmentName()); - } - } + computers.forEach(c -> c.queueEvent("speaker_audio_empty", c.getAttachmentName())); } // Push position updates to any speakers which have ever played a note, @@ -353,16 +352,12 @@ public abstract class SpeakerPeripheral implements IPeripheral { @Override public void attach(IComputerAccess computer) { - synchronized (computers) { - computers.add(computer); - } + computers.add(computer); } @Override public void detach(IComputerAccess computer) { - synchronized (computers) { - computers.remove(computer); - } + computers.remove(computer); } static double clampVolume(double volume) { diff --git a/projects/core-api/src/main/java/dan200/computercraft/api/peripheral/AttachedComputerSet.java b/projects/core-api/src/main/java/dan200/computercraft/api/peripheral/AttachedComputerSet.java new file mode 100644 index 000000000..aef0ec035 --- /dev/null +++ b/projects/core-api/src/main/java/dan200/computercraft/api/peripheral/AttachedComputerSet.java @@ -0,0 +1,129 @@ +// SPDX-FileCopyrightText: 2024 The CC: Tweaked Developers +// +// SPDX-License-Identifier: MPL-2.0 + +package dan200.computercraft.api.peripheral; + +import javax.annotation.Nullable; +import java.util.HashSet; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CopyOnWriteArraySet; +import java.util.concurrent.locks.ReentrantReadWriteLock; +import java.util.function.Consumer; + +/** + * A thread-safe collection of computers. + *

+ * This collection is intended to be used by peripherals that need to maintain a set of all attached computers. + *

+ * It is recommended to use over Java's built-in concurrent collections (e.g. {@link CopyOnWriteArraySet} or + * {@link ConcurrentHashMap}), as {@link AttachedComputerSet} ensures that computers cannot be accessed after they are + * detached, guaranteeing that {@link NotAttachedException}s will not be thrown. + *

+ * To ensure this, {@link AttachedComputerSet} is not directly iterable, as we cannot ensure that computers are not + * detached while the iterator is running (and so trying to use the computer would error). Instead, computers should be + * looped over using {@link #forEach(Consumer)}. + * + *

Example

+ * + * 
{@code
+ * public class MyPeripheral implements IPeripheral {
+ *     private final AttachedComputerSet computers = new ComputerCollection();
+ *
+ *     @Override
+ *     public void attach(IComputerAccess computer) {
+ *         computers.add(computer);
+ *     }
+ *
+ *     @Override
+ *     public void detach(IComputerAccess computer) {
+ *         computers.remove(computer);
+ *     }
+ * }
+ * }
+ * + * @see IComputerAccess + * @see IPeripheral#attach(IComputerAccess) + * @see IPeripheral#detach(IComputerAccess) + */ +public final class AttachedComputerSet { + private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock(); + private final Set computers = new HashSet<>(0); + + /** + * Add a computer to this collection of computers. This should be called from + * {@link IPeripheral#attach(IComputerAccess)}. + * + * @param computer The computer to add. + */ + public void add(IComputerAccess computer) { + lock.writeLock().lock(); + try { + computers.add(computer); + } finally { + lock.writeLock().unlock(); + } + } + + /** + * Remove a computer from this collection of computers. This should be called from + * {@link IPeripheral#detach(IComputerAccess)}. + * + * @param computer The computer to remove. + */ + public void remove(IComputerAccess computer) { + lock.writeLock().lock(); + try { + computers.remove(computer); + } finally { + lock.writeLock().unlock(); + } + } + + /** + * Apply an action to each computer in this collection. + * + * @param action The action to apply. + */ + public void forEach(Consumer action) { + lock.readLock().lock(); + try { + computers.forEach(action); + } finally { + lock.readLock().unlock(); + } + } + + /** + * {@linkplain IComputerAccess#queueEvent(String, Object...) Queue an event} on all computers. + * + * @param event The name of the event to queue. + * @param arguments The arguments for this event. + * @see IComputerAccess#queueEvent(String, Object...) + */ + public void queueEvent(String event, @Nullable Object... arguments) { + forEach(c -> c.queueEvent(event, arguments)); + } + + /** + * Determine if this collection contains any computers. + *

+ * This method is primarily intended for presentation purposes (such as rendering an icon in the UI if a computer + * is attached to your peripheral). Due to the multi-threaded nature of peripherals, it is not recommended to guard + * any logic behind this check. + *

+ * For instance, {@code if(computers.hasComputers()) computers.queueEvent("foo");} contains a race condition, as + * there's no guarantee that any computers are still attached within the body of the if statement. + * + * @return Whether this collection is non-empty. + */ + public boolean hasComputers() { + lock.readLock().lock(); + try { + return !computers.isEmpty(); + } finally { + lock.readLock().unlock(); + } + } +} diff --git a/projects/core-api/src/main/java/dan200/computercraft/api/peripheral/IPeripheral.java b/projects/core-api/src/main/java/dan200/computercraft/api/peripheral/IPeripheral.java index ca3123a61..3d96075a8 100644 --- a/projects/core-api/src/main/java/dan200/computercraft/api/peripheral/IPeripheral.java +++ b/projects/core-api/src/main/java/dan200/computercraft/api/peripheral/IPeripheral.java @@ -48,8 +48,9 @@ public interface IPeripheral { * {@code peripheral.call()}. This method can be used to keep track of which computers are attached to the * peripheral, or to take action when attachment occurs. *

- * Be aware that will be called from both the server thread and ComputerCraft Lua thread, and so must be thread-safe - * and reentrant. + * Be aware that may be called from both the server thread and ComputerCraft Lua thread, and so must be thread-safe + * and reentrant. If you need to store a list of attached computers, it is recommended you use a + * {@link AttachedComputerSet}. * * @param computer The interface to the computer that is being attached. Remember that multiple computers can be * attached to a peripheral at once. @@ -68,8 +69,9 @@ public interface IPeripheral { * This method can be used to keep track of which computers are attached to the peripheral, or to take action when * detachment occurs. *

- * Be aware that this will be called from both the server and ComputerCraft Lua thread, and must be thread-safe - * and reentrant. + * Be aware that this may be called from both the server and ComputerCraft Lua thread, and must be thread-safe + * and reentrant. If you need to store a list of attached computers, it is recommended you use a + * {@link AttachedComputerSet}. * * @param computer The interface to the computer that is being detached. Remember that multiple computers can be * attached to a peripheral at once.