1
0
mirror of https://github.com/SquidDev-CC/CC-Tweaked synced 2024-12-14 20:20:30 +00:00

Mount drives on the main thread

When the peripheral is attached, we add the computer to the map and
queue the actual disk to be mounted next tick. This avoids the
thread-safety issues with mutating the item (and creating disk ids) that
might be caused by doing it on the computer thread.

The mount is now also managed separately to the MediaStack, as that was
meant to be an immutable snapshot of the item!

Fixes #1282
This commit is contained in:
Jonathan Coates 2023-01-14 17:51:49 +00:00
parent 02b68b259e
commit edb21f33be
No known key found for this signature in database
GPG Key ID: B9E431FF07C98D06
6 changed files with 267 additions and 39 deletions

View File

@ -6,6 +6,7 @@
package dan200.computercraft.shared.peripheral.diskdrive;
import com.google.errorprone.annotations.concurrent.GuardedBy;
import dan200.computercraft.api.filesystem.Mount;
import dan200.computercraft.api.filesystem.WritableMount;
import dan200.computercraft.api.peripheral.IComputerAccess;
import dan200.computercraft.api.peripheral.IPeripheral;
@ -47,11 +48,14 @@ public final class DiskDriveBlockEntity extends AbstractContainerBlockEntity {
private final NonNullList<ItemStack> inventory = NonNullList.withSize(1, ItemStack.EMPTY);
private MediaStack media = MediaStack.EMPTY;
private @Nullable Mount mount;
private boolean recordPlaying = false;
// In order to avoid main-thread calls in the peripheral, we set flags to mark which operation should be performed,
// then read them when ticking.
private final AtomicReference<RecordCommand> recordQueued = new AtomicReference<>(null);
private final AtomicBoolean ejectQueued = new AtomicBoolean(false);
private final AtomicBoolean mountQueued = new AtomicBoolean(false);
public DiskDriveBlockEntity(BlockEntityType<DiskDriveBlockEntity> type, BlockPos pos, BlockState state) {
super(type, pos, state);
@ -109,6 +113,12 @@ public final class DiskDriveBlockEntity extends AbstractContainerBlockEntity {
}
}
}
if (mountQueued.get()) {
synchronized (this) {
mountAll();
}
}
}
@Override
@ -124,9 +134,9 @@ public final class DiskDriveBlockEntity extends AbstractContainerBlockEntity {
private void updateItem() {
var newDisk = getDiskStack();
if (ItemStack.isSame(newDisk, media.stack)) return;
if (ItemStack.isSameItemSameTags(newDisk, media.stack)) return;
var media = new MediaStack(newDisk.copy());
var media = MediaStack.of(newDisk);
if (newDisk.isEmpty()) {
updateBlockState(DiskDriveState.EMPTY);
@ -146,12 +156,10 @@ public final class DiskDriveBlockEntity extends AbstractContainerBlockEntity {
recordPlaying = false;
}
mount = null;
this.media = media;
// Mount new disk
if (!this.media.stack.isEmpty()) {
for (var computer : computers.entrySet()) mountDisk(computer.getKey(), computer.getValue(), this.media);
}
mountAll();
}
}
@ -163,11 +171,30 @@ public final class DiskDriveBlockEntity extends AbstractContainerBlockEntity {
return media;
}
/**
* Set the current disk stack, mounting/unmounting if needed.
*
* @param stack The new disk stack.
*/
void setDiskStack(ItemStack stack) {
setItem(0, stack);
setChanged();
}
/**
* Update the current disk stack, assuming the underlying item does not change. Unlike
* {@link #setDiskStack(ItemStack)} this will not change any mounts.
*
* @param stack The new disk stack.
*/
void updateDiskStack(ItemStack stack) {
setItem(0, stack);
if (!ItemStack.isSameItemSameTags(stack, media.stack)) {
media = MediaStack.of(stack);
super.setChanged();
}
}
@Nullable
String getDiskMountPath(IComputerAccess computer) {
synchronized (this) {
@ -176,15 +203,21 @@ public final class DiskDriveBlockEntity extends AbstractContainerBlockEntity {
}
}
void mount(IComputerAccess computer) {
/**
* Attach a computer to this disk drive. This sets up the {@link MountInfo} map and flags us to mount next tick. We
* don't mount here, as that might require mutating the current stack.
*
* @param computer The computer to attach.
*/
void attach(IComputerAccess computer) {
synchronized (this) {
var info = new MountInfo();
computers.put(computer, info);
mountDisk(computer, info, media);
mountQueued.set(true);
}
}
void unmount(IComputerAccess computer) {
void detach(IComputerAccess computer) {
synchronized (this) {
unmountDisk(computer, computers.remove(computer));
}
@ -202,10 +235,35 @@ public final class DiskDriveBlockEntity extends AbstractContainerBlockEntity {
ejectQueued.set(true);
}
/**
* Add our mount to all computers.
*/
@GuardedBy("this")
private void mountDisk(IComputerAccess computer, MountInfo info, MediaStack disk) {
var mount = disk.getMount((ServerLevel) getLevel());
if (mount != null) {
private void mountAll() {
doMountAll();
mountQueued.set(false);
}
/**
* The worker for {@link #mountAll()}. This is responsible for creating the mount and placing it on all computers.
*/
@GuardedBy("this")
private void doMountAll() {
if (computers.isEmpty() || media.media == null) return;
if (mount == null) {
var stack = getDiskStack();
mount = media.media.createDataMount(stack, (ServerLevel) level);
setDiskStack(stack);
}
if (mount == null) return;
for (var entry : computers.entrySet()) {
var computer = entry.getKey();
var info = entry.getValue();
if (info.mountPath != null) continue;
if (mount instanceof WritableMount writable) {
// Try mounting at the lowest numbered "disk" name we can
var n = 1;
@ -221,12 +279,10 @@ public final class DiskDriveBlockEntity extends AbstractContainerBlockEntity {
n++;
}
}
} else {
info.mountPath = null;
}
computer.queueEvent("disk", computer.getAttachmentName());
}
}
private static void unmountDisk(IComputerAccess computer, MountInfo info) {
if (info.mountPath != null) {

View File

@ -84,11 +84,12 @@ public class DiskDrivePeripheral implements IPeripheral {
var media = diskDrive.getMedia();
if (media.media == null) return;
var stack = media.stack.copy();
// We're on the main thread so the stack and media should be in sync.
var stack = diskDrive.getDiskStack();
if (!media.media.setLabel(stack, label.map(StringUtil::normaliseLabel).orElse(null))) {
throw new LuaException("Disk label cannot be changed");
}
diskDrive.setDiskStack(stack);
diskDrive.updateDiskStack(stack);
}
/**
@ -178,12 +179,12 @@ public class DiskDrivePeripheral implements IPeripheral {
@Override
public void attach(IComputerAccess computer) {
diskDrive.mount(computer);
diskDrive.attach(computer);
}
@Override
public void detach(IComputerAccess computer) {
diskDrive.unmount(computer);
diskDrive.detach(computer);
}
@Override

View File

@ -5,10 +5,8 @@
*/
package dan200.computercraft.shared.peripheral.diskdrive;
import dan200.computercraft.api.filesystem.Mount;
import dan200.computercraft.api.media.IMedia;
import dan200.computercraft.impl.MediaProviders;
import net.minecraft.server.level.ServerLevel;
import net.minecraft.sounds.SoundEvent;
import net.minecraft.world.item.ItemStack;
@ -17,18 +15,22 @@ import javax.annotation.Nullable;
/**
* An immutable snapshot of the current disk. This allows us to read the stack in a thread-safe manner.
*/
class MediaStack {
static final MediaStack EMPTY = new MediaStack(ItemStack.EMPTY);
final class MediaStack {
static final MediaStack EMPTY = new MediaStack(ItemStack.EMPTY, null);
final ItemStack stack;
final @Nullable IMedia media;
@Nullable
private Mount mount;
MediaStack(ItemStack stack) {
private MediaStack(ItemStack stack, @Nullable IMedia media) {
this.stack = stack;
media = MediaProviders.get(stack);
this.media = media;
}
public static MediaStack of(ItemStack stack) {
if (stack.isEmpty()) return EMPTY;
var freshStack = stack.copy();
return new MediaStack(freshStack, MediaProviders.get(freshStack));
}
@Nullable
@ -40,12 +42,4 @@ class MediaStack {
String getAudioTitle() {
return media != null ? media.getAudioTitle(stack) : null;
}
@Nullable
public Mount getMount(ServerLevel level) {
if (media == null) return null;
if (mount == null) mount = media.createDataMount(stack, level);
return mount;
}
}

View File

@ -8,6 +8,7 @@ package dan200.computercraft.gametest.core;
import com.mojang.brigadier.CommandDispatcher;
import dan200.computercraft.api.ComputerCraftAPI;
import dan200.computercraft.mixin.gametest.TestCommandAccessor;
import dan200.computercraft.shared.ModRegistry;
import net.minecraft.ChatFormatting;
import net.minecraft.commands.CommandSourceStack;
import net.minecraft.gametest.framework.GameTestRegistry;
@ -81,6 +82,27 @@ class CCTestCommand {
player.getLevel().addFreshEntity(armorStand);
return 0;
}))
.then(literal("give-computer").executes(context -> {
var player = context.getSource().getPlayerOrException();
var pos = StructureUtils.findNearestStructureBlock(player.blockPosition(), 15, player.getLevel());
if (pos == null) return error(context.getSource(), "No nearby test");
var structureBlock = (StructureBlockEntity) player.getLevel().getBlockEntity(pos);
if (structureBlock == null) return error(context.getSource(), "No nearby structure block");
var info = GameTestRegistry.getTestFunction(structureBlock.getStructurePath());
var item = ModRegistry.Items.COMPUTER_ADVANCED.get().create(1, info.getTestName());
if (!player.getInventory().add(item)) {
var itemEntity = player.drop(item, false);
if (itemEntity != null) {
itemEntity.setNoPickUpDelay();
itemEntity.setOwner(player.getUUID());
}
}
return 1;
}))
);
}

View File

@ -8,6 +8,7 @@ package dan200.computercraft.gametest
import dan200.computercraft.core.apis.FSAPI
import dan200.computercraft.gametest.api.*
import dan200.computercraft.shared.ModRegistry
import dan200.computercraft.shared.media.items.DiskItem
import dan200.computercraft.shared.peripheral.diskdrive.DiskDriveBlock
import dan200.computercraft.shared.peripheral.diskdrive.DiskDriveState
import dan200.computercraft.test.core.assertArrayEquals
@ -45,10 +46,14 @@ class Disk_Drive_Test {
thenWaitUntil { helper.assertItemEntityPresent(Items.MUSIC_DISC_13, stackAt, 0.0) }
}
/**
* A mount is initially attached, and then removed when the disk is ejected.
*/
@GameTest
fun Adds_removes_mount(helper: GameTestHelper) = helper.sequence {
thenIdle(2)
thenOnComputer {
thenOnComputer { } // Wait for the computer to start up
thenIdle(2) // Let the disk drive tick once to create the mount
thenOnComputer { // Then actually assert things!
getApi<FSAPI>().getDrive("disk").assertArrayEquals("right")
callPeripheral("right", "ejectDisk")
}
@ -56,6 +61,18 @@ class Disk_Drive_Test {
thenOnComputer { assertEquals(null, getApi<FSAPI>().getDrive("disk")) }
}
/**
* When creating a new mount, the item is with a new disk ID.
*/
@GameTest
fun Creates_disk_id(helper: GameTestHelper) = helper.sequence {
val drivePos = BlockPos(2, 2, 2)
thenWaitUntil {
val drive = helper.getBlockEntity(drivePos, ModRegistry.BlockEntities.DISK_DRIVE.get())
if (DiskItem.getDiskID(drive.getItem(0)) == -1) helper.fail("Disk has no item", drivePos)
}
}
/**
* Check comparators can read the contents of the disk drive
*/

View File

@ -0,0 +1,138 @@
{
DataVersion: 3218,
size: [5, 5, 5],
data: [
{pos: [0, 0, 0], state: "minecraft:polished_andesite"},
{pos: [0, 0, 1], state: "minecraft:polished_andesite"},
{pos: [0, 0, 2], state: "minecraft:polished_andesite"},
{pos: [0, 0, 3], state: "minecraft:polished_andesite"},
{pos: [0, 0, 4], state: "minecraft:polished_andesite"},
{pos: [1, 0, 0], state: "minecraft:polished_andesite"},
{pos: [1, 0, 1], state: "minecraft:polished_andesite"},
{pos: [1, 0, 2], state: "minecraft:polished_andesite"},
{pos: [1, 0, 3], state: "minecraft:polished_andesite"},
{pos: [1, 0, 4], state: "minecraft:polished_andesite"},
{pos: [2, 0, 0], state: "minecraft:polished_andesite"},
{pos: [2, 0, 1], state: "minecraft:polished_andesite"},
{pos: [2, 0, 2], state: "minecraft:polished_andesite"},
{pos: [2, 0, 3], state: "minecraft:polished_andesite"},
{pos: [2, 0, 4], state: "minecraft:polished_andesite"},
{pos: [3, 0, 0], state: "minecraft:polished_andesite"},
{pos: [3, 0, 1], state: "minecraft:polished_andesite"},
{pos: [3, 0, 2], state: "minecraft:polished_andesite"},
{pos: [3, 0, 3], state: "minecraft:polished_andesite"},
{pos: [3, 0, 4], state: "minecraft:polished_andesite"},
{pos: [4, 0, 0], state: "minecraft:polished_andesite"},
{pos: [4, 0, 1], state: "minecraft:polished_andesite"},
{pos: [4, 0, 2], state: "minecraft:polished_andesite"},
{pos: [4, 0, 3], state: "minecraft:polished_andesite"},
{pos: [4, 0, 4], state: "minecraft:polished_andesite"},
{pos: [0, 1, 0], state: "minecraft:air"},
{pos: [0, 1, 1], state: "minecraft:air"},
{pos: [0, 1, 2], state: "minecraft:air"},
{pos: [0, 1, 3], state: "minecraft:air"},
{pos: [0, 1, 4], state: "minecraft:air"},
{pos: [1, 1, 0], state: "minecraft:air"},
{pos: [1, 1, 1], state: "minecraft:air"},
{pos: [1, 1, 2], state: "computercraft:computer_advanced{facing:north,state:on}", nbt: {ComputerId: 1, Label: "disk_drive_test.creates_disk_id", On: 1b, id: "computercraft:computer_advanced"}},
{pos: [1, 1, 3], state: "minecraft:air"},
{pos: [1, 1, 4], state: "minecraft:air"},
{pos: [2, 1, 0], state: "minecraft:air"},
{pos: [2, 1, 1], state: "minecraft:air"},
{pos: [2, 1, 2], state: "computercraft:disk_drive{facing:north,state:full}", nbt: {Item: {Count: 1b, id: "computercraft:disk", tag: {Color: 1118481}}, id: "computercraft:disk_drive"}},
{pos: [2, 1, 3], state: "minecraft:air"},
{pos: [2, 1, 4], state: "minecraft:air"},
{pos: [3, 1, 0], state: "minecraft:air"},
{pos: [3, 1, 1], state: "minecraft:air"},
{pos: [3, 1, 2], state: "minecraft:air"},
{pos: [3, 1, 3], state: "minecraft:air"},
{pos: [3, 1, 4], state: "minecraft:air"},
{pos: [4, 1, 0], state: "minecraft:air"},
{pos: [4, 1, 1], state: "minecraft:air"},
{pos: [4, 1, 2], state: "minecraft:air"},
{pos: [4, 1, 3], state: "minecraft:air"},
{pos: [4, 1, 4], state: "minecraft:air"},
{pos: [0, 2, 0], state: "minecraft:air"},
{pos: [0, 2, 1], state: "minecraft:air"},
{pos: [0, 2, 2], state: "minecraft:air"},
{pos: [0, 2, 3], state: "minecraft:air"},
{pos: [0, 2, 4], state: "minecraft:air"},
{pos: [1, 2, 0], state: "minecraft:air"},
{pos: [1, 2, 1], state: "minecraft:air"},
{pos: [1, 2, 2], state: "minecraft:air"},
{pos: [1, 2, 3], state: "minecraft:air"},
{pos: [1, 2, 4], state: "minecraft:air"},
{pos: [2, 2, 0], state: "minecraft:air"},
{pos: [2, 2, 1], state: "minecraft:air"},
{pos: [2, 2, 2], state: "minecraft:air"},
{pos: [2, 2, 3], state: "minecraft:air"},
{pos: [2, 2, 4], state: "minecraft:air"},
{pos: [3, 2, 0], state: "minecraft:air"},
{pos: [3, 2, 1], state: "minecraft:air"},
{pos: [3, 2, 2], state: "minecraft:air"},
{pos: [3, 2, 3], state: "minecraft:air"},
{pos: [3, 2, 4], state: "minecraft:air"},
{pos: [4, 2, 0], state: "minecraft:air"},
{pos: [4, 2, 1], state: "minecraft:air"},
{pos: [4, 2, 2], state: "minecraft:air"},
{pos: [4, 2, 3], state: "minecraft:air"},
{pos: [4, 2, 4], state: "minecraft:air"},
{pos: [0, 3, 0], state: "minecraft:air"},
{pos: [0, 3, 1], state: "minecraft:air"},
{pos: [0, 3, 2], state: "minecraft:air"},
{pos: [0, 3, 3], state: "minecraft:air"},
{pos: [0, 3, 4], state: "minecraft:air"},
{pos: [1, 3, 0], state: "minecraft:air"},
{pos: [1, 3, 1], state: "minecraft:air"},
{pos: [1, 3, 2], state: "minecraft:air"},
{pos: [1, 3, 3], state: "minecraft:air"},
{pos: [1, 3, 4], state: "minecraft:air"},
{pos: [2, 3, 0], state: "minecraft:air"},
{pos: [2, 3, 1], state: "minecraft:air"},
{pos: [2, 3, 2], state: "minecraft:air"},
{pos: [2, 3, 3], state: "minecraft:air"},
{pos: [2, 3, 4], state: "minecraft:air"},
{pos: [3, 3, 0], state: "minecraft:air"},
{pos: [3, 3, 1], state: "minecraft:air"},
{pos: [3, 3, 2], state: "minecraft:air"},
{pos: [3, 3, 3], state: "minecraft:air"},
{pos: [3, 3, 4], state: "minecraft:air"},
{pos: [4, 3, 0], state: "minecraft:air"},
{pos: [4, 3, 1], state: "minecraft:air"},
{pos: [4, 3, 2], state: "minecraft:air"},
{pos: [4, 3, 3], state: "minecraft:air"},
{pos: [4, 3, 4], state: "minecraft:air"},
{pos: [0, 4, 0], state: "minecraft:air"},
{pos: [0, 4, 1], state: "minecraft:air"},
{pos: [0, 4, 2], state: "minecraft:air"},
{pos: [0, 4, 3], state: "minecraft:air"},
{pos: [0, 4, 4], state: "minecraft:air"},
{pos: [1, 4, 0], state: "minecraft:air"},
{pos: [1, 4, 1], state: "minecraft:air"},
{pos: [1, 4, 2], state: "minecraft:air"},
{pos: [1, 4, 3], state: "minecraft:air"},
{pos: [1, 4, 4], state: "minecraft:air"},
{pos: [2, 4, 0], state: "minecraft:air"},
{pos: [2, 4, 1], state: "minecraft:air"},
{pos: [2, 4, 2], state: "minecraft:air"},
{pos: [2, 4, 3], state: "minecraft:air"},
{pos: [2, 4, 4], state: "minecraft:air"},
{pos: [3, 4, 0], state: "minecraft:air"},
{pos: [3, 4, 1], state: "minecraft:air"},
{pos: [3, 4, 2], state: "minecraft:air"},
{pos: [3, 4, 3], state: "minecraft:air"},
{pos: [3, 4, 4], state: "minecraft:air"},
{pos: [4, 4, 0], state: "minecraft:air"},
{pos: [4, 4, 1], state: "minecraft:air"},
{pos: [4, 4, 2], state: "minecraft:air"},
{pos: [4, 4, 3], state: "minecraft:air"},
{pos: [4, 4, 4], state: "minecraft:air"}
],
entities: [],
palette: [
"minecraft:polished_andesite",
"minecraft:air",
"computercraft:computer_advanced{facing:north,state:on}",
"computercraft:disk_drive{facing:north,state:full}"
]
}