From 82a6702b334ef7afaa7f4e871eee39fcb2cc008a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Florian=20N=C3=BCcke?= Date: Thu, 27 Jan 2022 20:10:41 +0100 Subject: [PATCH] Rework RPCDevice to be more intuitive for typical cases. unmount() is now called in addition to when suspend() was called before. dispose() is called after unmount() if cleanup is required (machine stopped, device removed). --- build.gradle | 6 +- .../device/object/LifecycleAwareDevice.java | 11 +- .../api/bus/device/object/ObjectDevice.java | 4 +- .../cil/oc2/api/bus/device/rpc/RPCDevice.java | 74 +++++---- .../oc2/common/bus/RPCDeviceBusAdapter.java | 67 ++++---- .../device/item/AbstractItemRPCDevice.java | 4 +- .../item/FileImportExportCardItemDevice.java | 2 +- .../common/bus/device/rpc/RPCDeviceList.java | 4 +- .../oc2/common/vm/AbstractVirtualMachine.java | 4 +- .../li/cil/oc2/common/bus/RPCDeviceTests.java | 147 +++++++----------- 10 files changed, 150 insertions(+), 173 deletions(-) diff --git a/build.gradle b/build.gradle index 1a6d0f66..c7b8588e 100644 --- a/build.gradle +++ b/build.gradle @@ -97,8 +97,6 @@ minecraft { source sourceSets.main } } - - arg "-mixin.config=oc2.mixins.json" } client { @@ -119,6 +117,10 @@ minecraft { mixin { add sourceSets.main, "oc2.refmap.json" + + config 'oc2.mixins.json' + + quiet } task copyGeneratedResources(type: Copy) { diff --git a/src/main/java/li/cil/oc2/api/bus/device/object/LifecycleAwareDevice.java b/src/main/java/li/cil/oc2/api/bus/device/object/LifecycleAwareDevice.java index 2badd061..5c6fdfc7 100644 --- a/src/main/java/li/cil/oc2/api/bus/device/object/LifecycleAwareDevice.java +++ b/src/main/java/li/cil/oc2/api/bus/device/object/LifecycleAwareDevice.java @@ -11,23 +11,20 @@ import net.minecraft.world.level.block.entity.BlockEntity; */ public interface LifecycleAwareDevice { /** - * This method corresponds to {@link RPCDevice#mount()}. It is called when the device is initialized, either - * because its virtual machine starts running, or because it is added to a running virtual machine. + * This method corresponds to {@link RPCDevice#mount()}. */ default void onDeviceMounted() { } /** - * This method corresponds to {@link RPCDevice#unmount()}. It is called when the device is disposed, either - * because its virtual machine stops running, or because it is removed from a running virtual machine. + * This method corresponds to {@link RPCDevice#unmount()}. */ default void onDeviceUnmounted() { } /** - * This method corresponds to {@link RPCDevice#suspend()}. It is called when its virtual machine is suspended, - * either due to the containing chunk being unloaded, or the containing world being unloaded. + * This method corresponds to {@link RPCDevice#dispose()}. */ - default void onDeviceSuspended() { + default void onDeviceDisposed() { } } diff --git a/src/main/java/li/cil/oc2/api/bus/device/object/ObjectDevice.java b/src/main/java/li/cil/oc2/api/bus/device/object/ObjectDevice.java index e5f1885e..afda5cd5 100644 --- a/src/main/java/li/cil/oc2/api/bus/device/object/ObjectDevice.java +++ b/src/main/java/li/cil/oc2/api/bus/device/object/ObjectDevice.java @@ -102,9 +102,9 @@ public final class ObjectDevice implements RPCDevice { } @Override - public void suspend() { + public void dispose() { if (object instanceof LifecycleAwareDevice device) { - device.onDeviceSuspended(); + device.onDeviceDisposed(); } } diff --git a/src/main/java/li/cil/oc2/api/bus/device/rpc/RPCDevice.java b/src/main/java/li/cil/oc2/api/bus/device/rpc/RPCDevice.java index f650d71c..f35f45d4 100644 --- a/src/main/java/li/cil/oc2/api/bus/device/rpc/RPCDevice.java +++ b/src/main/java/li/cil/oc2/api/bus/device/rpc/RPCDevice.java @@ -20,35 +20,27 @@ import java.util.List; *

* The lifecycle for {@link RPCDevice}s is as follows: * 

- *    ┌──────────────────────────────────┐
- *    │VirtualMachine.isRunning() = false◄──────────────────────┐
- *    └────────────────┬─────────────────┘                      │
- *                     │                                        │
- *          ┌──────────▼───────────┐                            │
- *          │VirtualMachine.start()│                            │
- *          └──────────┬───────────┘                            │
- *                     │                                        │
- *                     │   ┌──────────┐                         │
- *                     │   │Chunk Load│   ┌───────────────────┐ │
- *                     ├───┼──────────◄───┤RPCDevice.suspend()│ │
- *                     │   │World Load│   └──────▲────────────┘ │
- *                     │   └──────────┘          │              │
- *                     │                         │              │
- *            ┌────────▼────────┐          ┌─────┴──────┐       │
- * ┌──────────►RPCDevice.mount()│          │Chunk Unload│       │
- * │          └────────┬────────┘        ┌─►────────────┤       │
- * │                   │                 │ │World Unload│       │
- * │ ┌─────────────────▼───────────────┐ │ └────────────┘       │
- * │ │VirtualMachine.isRunning() = true├─┤                      │
- * │ └─────┬───────────────────┬───────┘ │ ┌──────────────────┐ │
- * │       │                   │         │ │Computer Shutdown │ │
- * │ ┌─────▼──────┐     ┌──────▼───────┐ └─►──────────────────┤ │
- * └─┤Device Added│     │Device Removed│   │Computer Destroyed│ │
- *   └────────────┘     └──────┬───────┘   └─────┬────────────┘ │
- *                             │                 │              │
- *                  ┌──────────▼────────┐ ┌──────▼────────────┐ │
- *                  │RPCDevice.unmount()│ │RPCDevice.unmount()├─┘
- *                  └───────────────────┘ └───────────────────┘
+ * ┌──────────────┐ ┌────────────────┐
+ * │serializeNBT()│ │deserializeNBT()◄─────────────────┐
+ * └──────────────┘ └───────┬────────┘                 │
+ *   May be called          │VM starts or              │
+ *   at any time,           │resumes after             │
+ *   except while           │load                      │
+ *   unloaded...        ┌───▼───┐                      │
+ *                      │mount()│                      │
+ *                      └───┬───┘                      │
+ *                          │VM stops or               │
+ *                          │is unloaded               │
+ *                          │                          │
+ *                     ┌────▼────┐Chunk unloaded       │
+ *                     │unmount()├─────────────────────┤
+ *                     └────┬────┘                     │
+ *                          │VM stopped or             │
+ *                          │device removed            │
+ *                          │                          │
+ *                     ┌────▼────┐                     │
+ *                     │dispose()├─────────────────────┘
+ *                     └─────────┘
  *
* * @see ObjectDevice @@ -78,7 +70,7 @@ public interface RPCDevice extends Device { List getMethodGroups(); /** - * Called to initialize this device. + * Called to start this device. *

* This is called when the connected virtual machine starts, or when the device is added to an already running * virtual machine. @@ -87,22 +79,28 @@ public interface RPCDevice extends Device { } /** - * Called to dispose this device. + * Called to stop this device. *

- * Called when the connected virtual machine stops, or when the device is removed from a currently running - * virtual machine. + * Called when the connected virtual machine stops, the device is removed from a currently running + * virtual machine, or the connected virtual machine is suspended (chunk unload/server stopped/...). + *

+ * If {@link #mount()} was called, this is guaranteed to be called. */ default void unmount() { } /** - * Called when the device is suspended. + * Called to dispose this device. *

- * This can happen when the level area containing the context the device was loaded in is unloaded, - * e.g. due to player moving too far away from the area. + * Called when the connected virtual machine stops or the device is removed from a currently running + * virtual machine. *

- * Intended for soft-releasing unmanaged resource, i.e. non-persisted unmanaged resources. + * Will only be called on unmounted devices (i.e. will always be called after {@link #unmount()} if + * {@link #mount()} was called before). May be called without intermediary {@link #mount()} calls, e.g. + * virtual machine stops, then device is disconnected from the virtual machine. + *

+ * Intended for releasing persistent unmanaged resources. */ - default void suspend() { + default void dispose() { } } diff --git a/src/main/java/li/cil/oc2/common/bus/RPCDeviceBusAdapter.java b/src/main/java/li/cil/oc2/common/bus/RPCDeviceBusAdapter.java index bd72d899..2b537f88 100644 --- a/src/main/java/li/cil/oc2/common/bus/RPCDeviceBusAdapter.java +++ b/src/main/java/li/cil/oc2/common/bus/RPCDeviceBusAdapter.java @@ -36,7 +36,7 @@ public final class RPCDeviceBusAdapter implements Steppable { private final SerialDevice serialDevice; private final Gson gson; - private final ArrayList devices = new ArrayList<>(); + private final ArrayList devicesWithId = new ArrayList<>(); private final HashMap devicesById = new HashMap<>(); private final Set unmountedDevices = new HashSet<>(); private final Set mountedDevices = new HashSet<>(); @@ -82,14 +82,18 @@ public final class RPCDeviceBusAdapter implements Steppable { for (final RPCDevice device : mountedDevices) { device.unmount(); } - unmountedDevices.addAll(mountedDevices); - mountedDevices.clear(); } - public void suspend() { - for (final RPCDeviceWithIdentifier info : devices) { - info.device.suspend(); + public void dispose() { + for (final RPCDevice device : mountedDevices) { + device.unmount(); + device.dispose(); } + for (final RPCDeviceList device : unmountedDevices) { + device.dispose(); + } + unmountedDevices.addAll(mountedDevices); + mountedDevices.clear(); } public void reset() { @@ -115,10 +119,6 @@ public final class RPCDeviceBusAdapter implements Steppable { return; } - devices.clear(); - devicesById.clear(); - unmountedDevices.clear(); - // How device grouping works: // Each device can have multiple UUIDs due to being attached to multiple bus elements. // There is no guarantee that for each device D1 present on bus elements E1 and E2, @@ -162,31 +162,38 @@ public final class RPCDeviceBusAdapter implements Steppable { .add(identifier); }); - final Set newDevices = new HashSet<>(); + // Rebuild devices lists. + devicesWithId.clear(); + devicesById.clear(); + + final Set devices = new HashSet<>(); identifiersByDevice.forEach((device, identifiers) -> { final UUID identifier = selectIdentifierDeterministically(identifiers); - devices.add(new RPCDeviceWithIdentifier(identifier, device)); + devicesWithId.add(new RPCDeviceWithIdentifier(identifier, device)); devicesById.put(identifier, device); - newDevices.add(device); + devices.add(device); + + // Add to set of unmounted devices if we don't already track it. It's a set, so + // there won't be duplicates in the unmounted set due to this. + if (!mountedDevices.contains(device)) { + unmountedDevices.add(device); + } }); - // Add new devices to list of unmounted devices. List was cleared, so removed devices previously in - // list of unmounted devices are already gone. - for (final RPCDeviceList newDevice : newDevices) { - if (!mountedDevices.contains(newDevice)) { - unmountedDevices.add(newDevice); - } - } + // Remove devices from mounted set, call appropriate callbacks. + final HashSet removedMountedDevices = new HashSet<>(mountedDevices); + removedMountedDevices.removeAll(devices); + mountedDevices.removeAll(removedMountedDevices); + removedMountedDevices.forEach(device -> { + device.unmount(); + device.dispose(); + }); - // Remove removed devices from list of mounted devices. - final Iterator mountedDeviceIterator = mountedDevices.iterator(); - while (mountedDeviceIterator.hasNext()) { - final RPCDeviceList device = mountedDeviceIterator.next(); - if (!newDevices.contains(device)) { - device.unmount(); - mountedDeviceIterator.remove(); - } - } + // Remove devices from unmounted set, call appropriate callbacks. + final HashSet removedUnmountedDevices = new HashSet<>(unmountedDevices); + removedUnmountedDevices.removeAll(devices); + unmountedDevices.removeAll(removedUnmountedDevices); + removedUnmountedDevices.forEach(RPCDeviceList::dispose); } public void tick() { @@ -355,7 +362,7 @@ public final class RPCDeviceBusAdapter implements Steppable { } private void writeDeviceList() { - writeMessage(Message.MESSAGE_TYPE_LIST, devices); + writeMessage(Message.MESSAGE_TYPE_LIST, devicesWithId); } private void writeDeviceMethods(final UUID deviceId) { diff --git a/src/main/java/li/cil/oc2/common/bus/device/item/AbstractItemRPCDevice.java b/src/main/java/li/cil/oc2/common/bus/device/item/AbstractItemRPCDevice.java index 9293f412..17ef1558 100644 --- a/src/main/java/li/cil/oc2/common/bus/device/item/AbstractItemRPCDevice.java +++ b/src/main/java/li/cil/oc2/common/bus/device/item/AbstractItemRPCDevice.java @@ -42,7 +42,7 @@ public abstract class AbstractItemRPCDevice extends IdentityProxy imp } @Override - public void suspend() { - device.suspend(); + public void dispose() { + device.dispose(); } } diff --git a/src/main/java/li/cil/oc2/common/bus/device/item/FileImportExportCardItemDevice.java b/src/main/java/li/cil/oc2/common/bus/device/item/FileImportExportCardItemDevice.java index 666f15ed..0a59c03f 100644 --- a/src/main/java/li/cil/oc2/common/bus/device/item/FileImportExportCardItemDevice.java +++ b/src/main/java/li/cil/oc2/common/bus/device/item/FileImportExportCardItemDevice.java @@ -134,7 +134,7 @@ public final class FileImportExportCardItemDevice extends AbstractItemRPCDevice /////////////////////////////////////////////////////////////////// @Override - public void suspend() { + public void unmount() { reset(); } diff --git a/src/main/java/li/cil/oc2/common/bus/device/rpc/RPCDeviceList.java b/src/main/java/li/cil/oc2/common/bus/device/rpc/RPCDeviceList.java index d0e9cd16..087680b9 100644 --- a/src/main/java/li/cil/oc2/common/bus/device/rpc/RPCDeviceList.java +++ b/src/main/java/li/cil/oc2/common/bus/device/rpc/RPCDeviceList.java @@ -42,9 +42,9 @@ public record RPCDeviceList(ArrayList devices) implements RPCDevice { } @Override - public void suspend() { + public void dispose() { for (final RPCDevice device : devices) { - device.suspend(); + device.dispose(); } } diff --git a/src/main/java/li/cil/oc2/common/vm/AbstractVirtualMachine.java b/src/main/java/li/cil/oc2/common/vm/AbstractVirtualMachine.java index a874f452..0e37586c 100644 --- a/src/main/java/li/cil/oc2/common/vm/AbstractVirtualMachine.java +++ b/src/main/java/li/cil/oc2/common/vm/AbstractVirtualMachine.java @@ -91,7 +91,7 @@ public abstract class AbstractVirtualMachine implements VirtualMachine { public void suspend() { joinWorkerThread(); state.vmAdapter.suspend(); - state.rpcAdapter.suspend(); + state.rpcAdapter.unmount(); } @Override @@ -203,7 +203,7 @@ public abstract class AbstractVirtualMachine implements VirtualMachine { state.board.setRunning(false); state.board.reset(); state.rpcAdapter.reset(); - state.rpcAdapter.unmount(); + state.rpcAdapter.dispose(); state.vmAdapter.unmount(); runner = null; diff --git a/src/test/java/li/cil/oc2/common/bus/RPCDeviceTests.java b/src/test/java/li/cil/oc2/common/bus/RPCDeviceTests.java index 9ad35b03..c3be390d 100644 --- a/src/test/java/li/cil/oc2/common/bus/RPCDeviceTests.java +++ b/src/test/java/li/cil/oc2/common/bus/RPCDeviceTests.java @@ -30,134 +30,94 @@ public final class RPCDeviceTests { } @Test - public void emptyDevicesAreNotMounted() { - final RPCDevice device1 = mock(RPCDevice.class); - addDevice(device1); + public void resumeDoesNotMountDirectly() { + final RPCDevice device1 = addDevice(); adapter.resume(controller, true); - adapter.mount(); verify(device1, never()).mount(); } + @Test + public void emptyDevicesAreNotMounted() { + final RPCDevice device = addEmptyDevice(); + adapter.resume(controller, true); + + adapter.mount(); + verify(device, never()).mount(); + } + @Test public void addedDevicesHaveMountCalled() { - final RPCDevice device1 = mock(RPCDevice.class); - when(device1.getMethodGroups()).thenReturn(Collections.singletonList(mock(RPCMethod.class))); - addDevice(device1); - + final RPCDevice device = addDevice(); adapter.resume(controller, true); - verify(device1, never()).mount(); adapter.mount(); - verify(device1).mount(); + verify(device).mount(); } @Test - public void mountedDevicesAreUnmountedWhenRemoved() { - final RPCDevice device1 = mock(RPCDevice.class); - when(device1.getMethodGroups()).thenReturn(Collections.singletonList(mock(RPCMethod.class))); - addDevice(device1); - + public void mountedDevicesAreUnmountedAndDisposedWhenRemoved() { + final RPCDevice device = addDevice(); adapter.resume(controller, true); - verify(device1, never()).mount(); - adapter.mount(); - verify(device1).mount(); - removeDevice(device1); + removeDevice(device); adapter.resume(controller, true); - verify(device1).unmount(); + verify(device).unmount(); + verify(device).dispose(); } @Test - public void mountedDevicesAreUnmountedOnGlobalUnmount() { - final RPCDevice device1 = mock(RPCDevice.class); - when(device1.getMethodGroups()).thenReturn(Collections.singletonList(mock(RPCMethod.class))); - addDevice(device1); - + public void unmountedDevicesAreDisposedWhenRemoved() { + final RPCDevice device = addDevice(); adapter.resume(controller, true); - verify(device1, never()).mount(); + removeDevice(device); + adapter.resume(controller, true); + verify(device, never()).unmount(); + verify(device).dispose(); + } + + @Test + public void mountedDevicesAreUnmountedButNotDisposedOnGlobalUnmount() { + final RPCDevice device = addDevice(); + adapter.resume(controller, true); adapter.mount(); - verify(device1).mount(); adapter.unmount(); - verify(device1).unmount(); + verify(device).unmount(); + verify(device, never()).dispose(); } @Test - public void unmountedDevicesAreNotUnmountedOnGlobalUnmount() { - final RPCDevice device1 = mock(RPCDevice.class); - when(device1.getMethodGroups()).thenReturn(Collections.singletonList(mock(RPCMethod.class))); - addDevice(device1); - + public void unmountedDevicesAreNotUnmountedAndNotDisposedOnGlobalUnmount() { + final RPCDevice device = addDevice(); adapter.resume(controller, true); - verify(device1, never()).mount(); adapter.unmount(); - verify(device1, never()).unmount(); + verify(device, never()).unmount(); + verify(device, never()).dispose(); } @Test - public void deviceListForwardsMount() { - final RPCDevice device1 = mock(RPCDevice.class); - final RPCDevice device2 = mock(RPCDevice.class); - final RPCDevice listDevice = new RPCDeviceList(new ArrayList<>(Arrays.asList(device1, device2))); - when(device1.getMethodGroups()).thenReturn(Collections.singletonList(mock(RPCMethod.class))); - when(device2.getMethodGroups()).thenReturn(Collections.singletonList(mock(RPCMethod.class))); - addDevice(listDevice); - + public void mountedDevicesAreUnmountedAndDisposedOnGlobalDispose() { + final RPCDevice device = addDevice(); adapter.resume(controller, true); - verify(device1, never()).mount(); - verify(device2, never()).mount(); - adapter.mount(); - verify(device1).mount(); - verify(device2).mount(); + + adapter.dispose(); + verify(device).unmount(); + verify(device).dispose(); } @Test - public void deviceListForwardsUnmount() { - final RPCDevice device1 = mock(RPCDevice.class); - final RPCDevice device2 = mock(RPCDevice.class); - final RPCDevice listDevice = new RPCDeviceList(new ArrayList<>(Arrays.asList(device1, device2))); - when(device1.getMethodGroups()).thenReturn(Collections.singletonList(mock(RPCMethod.class))); - when(device2.getMethodGroups()).thenReturn(Collections.singletonList(mock(RPCMethod.class))); - addDevice(listDevice); - + public void unmountedDevicesAreNotUnmountedButDisposedOnGlobalDispose() { + final RPCDevice device = addDevice(); adapter.resume(controller, true); - verify(device1, never()).mount(); - verify(device2, never()).mount(); - adapter.mount(); - verify(device1).mount(); - verify(device2).mount(); - - adapter.unmount(); - verify(device1).unmount(); - verify(device2).unmount(); - } - - @Test - public void deviceListForwardsSuspend() { - final RPCDevice device1 = mock(RPCDevice.class); - final RPCDevice device2 = mock(RPCDevice.class); - final RPCDevice listDevice = new RPCDeviceList(new ArrayList<>(Arrays.asList(device1, device2))); - when(device1.getMethodGroups()).thenReturn(Collections.singletonList(mock(RPCMethod.class))); - when(device2.getMethodGroups()).thenReturn(Collections.singletonList(mock(RPCMethod.class))); - addDevice(listDevice); - - adapter.resume(controller, true); - verify(device1, never()).mount(); - verify(device2, never()).mount(); - - adapter.mount(); - verify(device1).mount(); - verify(device2).mount(); - - adapter.suspend(); - verify(device1).suspend(); - verify(device2).suspend(); + adapter.dispose(); + verify(device, never()).unmount(); + verify(device).dispose(); } @Test @@ -187,6 +147,19 @@ public final class RPCDeviceTests { verify(device2, atMostOnce()).mount(); } + private RPCDevice addEmptyDevice() { + final RPCDevice device = mock(RPCDevice.class); + addDevice(device); + return device; + } + + private RPCDevice addDevice() { + final RPCDevice device = mock(RPCDevice.class); + when(device.getMethodGroups()).thenReturn(Collections.singletonList(mock(RPCMethod.class))); + addDevice(device); + return device; + } + private void addDevice(final Device device, UUID... identifiers) { if (identifiers.length == 0) { identifiers = new UUID[]{UUID.randomUUID()};