1
0
mirror of https://github.com/SquidDev-CC/CC-Tweaked synced 2025-10-24 18:37:38 +00:00

Add a helper class for working with attached computers

One of the easiest things to mess up with writing a custom peripheral is
handling attached peripherals. IPeripheral.{attach,detach} are called
from multiple threads, so naive implementations that just store
computers in a set/list will at some point throw a CME.

Historically I've suggested using a concurrent collection (i.e.
ConcurrentHashMap). While this solves the problems of CMEs, it still has
some flaws. If a computer is detached while iterating over the
collection, the iterator will still yield the now-detached peripheral,
causing usages of that computer (e.g. queueEvent) to throw an exception.

The only fix here is to use a lock when updating and iterating over the
collection. This does come with some risks, but I think they are not too
serious:

 - Lock contention: Contention is relatively rare in general (as
   peripheral attach/detach is not especially frequent). If we do see
   contention, both iteration and update actions are cheap, so I would
   not expect the other thread to be blocked for a significant time.

 - Deadlocks: One could imagine an implementation if IComputerAccess
   that holds a lock both when detaching a peripheral and inside
   queueEvent.

   If we queue an event on one thread, and try to detach on the other,
   we could see a deadlock:

     Thread 1                         | Thread 2
    ----------------------------------------------------------
     AttachedComputerSet.queueEvent   | MyModem.detach
      (take lock #1)                  |  (take lock #2)

     -> MyModem.queueEvent            | AttachedComputerSet.remove
      (wait on lock #2)               |  (wait on lock #1)

   Such code would have been broken already (some peripherals already
   use locks), so I'm fairly sure we've fixed this in CC. But definitely
   something to watch out for.

Anyway, the long and short of it:
 - Add a new AttachedComputerSet that can be used to track the computers
   attached to a peripheral. We also mention this in the attach/detach
   docs, to hopefully make it a little more obvoius.

 - Update speakers and monitors to use this new class.
This commit is contained in:
Jonathan Coates
2024-09-22 13:51:11 +01:00
parent 3042950507
commit e9aceca1de
4 changed files with 147 additions and 23 deletions

View File

@@ -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<IComputerAccess> 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);
}
}
}

View File

@@ -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<IComputerAccess> 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) {