From 04900dc82f143e6342b7196a0126a939ead09609 Mon Sep 17 00:00:00 2001 From: Jonathan Coates Date: Thu, 21 Mar 2024 19:38:01 +0000 Subject: [PATCH] Skip main-thread tasks if peripheral is detached Due to the asynchronous nature of main-thread tasks, it's possible for them to be executed on peripherals which have been detached. This has been known for a long time (#893 was opened back in 2021), but finding a good solution here is tricky. Most of the time the method will silently succeed, but if we try to interact with an IComputerAccess (such as in inventory methods, as seen in #1750), we throw a NotAttachedException exception and spam the logs! This is an initial step towards fixing this - when calling a peripheral method via peripheral.call/modem.callRemote, we now wrap any enqueued main-thread tasks and silently skip them if the peripheral has been detached since. This means that peripheral methods may start to return nil when they didn't before. I think this is *fine* (though not ideal for sure!) - we return nil if the peripheral has been detached, so it's largely equivalent to that. --- .../peripheral/generic/GenericPeripheral.java | 12 +++- .../modem/wired/WiredModemPeripheral.java | 19 ++++++- .../core/apis/PeripheralAPI.java | 21 ++++++- .../core/computer/GuardedLuaContext.java | 55 +++++++++++++++++++ 4 files changed, 101 insertions(+), 6 deletions(-) create mode 100644 projects/core/src/main/java/dan200/computercraft/core/computer/GuardedLuaContext.java diff --git a/projects/common/src/main/java/dan200/computercraft/shared/peripheral/generic/GenericPeripheral.java b/projects/common/src/main/java/dan200/computercraft/shared/peripheral/generic/GenericPeripheral.java index d57208b8f..ee4f1f299 100644 --- a/projects/common/src/main/java/dan200/computercraft/shared/peripheral/generic/GenericPeripheral.java +++ b/projects/common/src/main/java/dan200/computercraft/shared/peripheral/generic/GenericPeripheral.java @@ -11,6 +11,7 @@ import dan200.computercraft.api.lua.MethodResult; import dan200.computercraft.api.peripheral.IComputerAccess; import dan200.computercraft.api.peripheral.IDynamicPeripheral; import dan200.computercraft.api.peripheral.IPeripheral; +import dan200.computercraft.core.computer.GuardedLuaContext; import net.minecraft.core.Direction; import net.minecraft.world.level.block.entity.BlockEntity; @@ -26,12 +27,16 @@ public final class GenericPeripheral implements IDynamicPeripheral { private final Set additionalTypes; private final List methods; + private @Nullable GuardedLuaContext contextWrapper; + private final GuardedLuaContext.Guard guard; + GenericPeripheral(BlockEntity tile, Direction side, String type, Set additionalTypes, List methods) { this.side = side; this.tile = tile; this.type = type; this.additionalTypes = additionalTypes; this.methods = methods; + this.guard = () -> !tile.isRemoved() && tile.getLevel() != null && tile.getLevel().isLoaded(tile.getBlockPos()); } public Direction side() { @@ -47,7 +52,12 @@ public final class GenericPeripheral implements IDynamicPeripheral { @Override public MethodResult callMethod(IComputerAccess computer, ILuaContext context, int method, IArguments arguments) throws LuaException { - return methods.get(method).apply(context, computer, arguments); + var contextWrapper = this.contextWrapper; + if (contextWrapper == null || !contextWrapper.wraps(context)) { + contextWrapper = this.contextWrapper = new GuardedLuaContext(context, guard); + } + + return methods.get(method).apply(contextWrapper, computer, arguments); } @Override diff --git a/projects/common/src/main/java/dan200/computercraft/shared/peripheral/modem/wired/WiredModemPeripheral.java b/projects/common/src/main/java/dan200/computercraft/shared/peripheral/modem/wired/WiredModemPeripheral.java index ac14bd962..ce1374b27 100644 --- a/projects/common/src/main/java/dan200/computercraft/shared/peripheral/modem/wired/WiredModemPeripheral.java +++ b/projects/common/src/main/java/dan200/computercraft/shared/peripheral/modem/wired/WiredModemPeripheral.java @@ -15,6 +15,7 @@ import dan200.computercraft.api.peripheral.IPeripheral; import dan200.computercraft.api.peripheral.NotAttachedException; import dan200.computercraft.api.peripheral.WorkMonitor; import dan200.computercraft.core.apis.PeripheralAPI; +import dan200.computercraft.core.computer.GuardedLuaContext; import dan200.computercraft.core.methods.PeripheralMethod; import dan200.computercraft.core.util.LuaUtil; import dan200.computercraft.shared.computer.core.ServerContext; @@ -304,7 +305,7 @@ public abstract class WiredModemPeripheral extends ModemPeripheral implements Wi return wrappers == null ? null : wrappers.get(remoteName); } - private static class RemotePeripheralWrapper implements IComputerAccess { + private static class RemotePeripheralWrapper implements IComputerAccess, GuardedLuaContext.Guard { private final WiredModemElement element; private final IPeripheral peripheral; private final IComputerAccess computer; @@ -317,6 +318,8 @@ public abstract class WiredModemPeripheral extends ModemPeripheral implements Wi private volatile boolean attached; private final Set mounts = new HashSet<>(); + private @Nullable GuardedLuaContext contextWrapper; + RemotePeripheralWrapper(WiredModemElement element, IPeripheral peripheral, IComputerAccess computer, String name, Map methods) { this.element = element; this.peripheral = peripheral; @@ -364,7 +367,19 @@ public abstract class WiredModemPeripheral extends ModemPeripheral implements Wi public MethodResult callMethod(ILuaContext context, String methodName, IArguments arguments) throws LuaException { var method = methodMap.get(methodName); if (method == null) throw new LuaException("No such method " + methodName); - return method.apply(peripheral, context, this, arguments); + + // Wrap the ILuaContext. We try to reuse the previous context where possible to avoid allocations. + var contextWrapper = this.contextWrapper; + if (contextWrapper == null || !contextWrapper.wraps(context)) { + contextWrapper = this.contextWrapper = new GuardedLuaContext(context, this); + } + + return method.apply(peripheral, contextWrapper, this, arguments); + } + + @Override + public boolean checkValid() { + return attached; } // IComputerAccess implementation diff --git a/projects/core/src/main/java/dan200/computercraft/core/apis/PeripheralAPI.java b/projects/core/src/main/java/dan200/computercraft/core/apis/PeripheralAPI.java index 70fdb0b1c..0b5307fcd 100644 --- a/projects/core/src/main/java/dan200/computercraft/core/apis/PeripheralAPI.java +++ b/projects/core/src/main/java/dan200/computercraft/core/apis/PeripheralAPI.java @@ -11,6 +11,7 @@ import dan200.computercraft.api.peripheral.IPeripheral; import dan200.computercraft.api.peripheral.NotAttachedException; import dan200.computercraft.api.peripheral.WorkMonitor; import dan200.computercraft.core.computer.ComputerSide; +import dan200.computercraft.core.computer.GuardedLuaContext; import dan200.computercraft.core.methods.MethodSupplier; import dan200.computercraft.core.methods.PeripheralMethod; import dan200.computercraft.core.metrics.Metrics; @@ -26,7 +27,7 @@ import java.util.*; * @hidden */ public class PeripheralAPI implements ILuaAPI, IAPIEnvironment.IPeripheralChangeListener { - private class PeripheralWrapper extends ComputerAccess { + private class PeripheralWrapper extends ComputerAccess implements GuardedLuaContext.Guard { private final String side; private final IPeripheral peripheral; @@ -35,6 +36,8 @@ public class PeripheralAPI implements ILuaAPI, IAPIEnvironment.IPeripheralChange private final Map methodMap; private boolean attached = false; + private @Nullable GuardedLuaContext contextWrapper; + PeripheralWrapper(IPeripheral peripheral, String side) { super(environment); this.side = side; @@ -91,9 +94,21 @@ public class PeripheralAPI implements ILuaAPI, IAPIEnvironment.IPeripheralChange if (method == null) throw new LuaException("No such method " + methodName); - try (var ignored = environment.time(Metrics.PERIPHERAL_OPS)) { - return method.apply(peripheral, context, this, arguments); + // Wrap the ILuaContext. We try to reuse the previous context where possible to avoid allocations - this + // should be pretty common as ILuaMachine uses a constant context. + var contextWrapper = this.contextWrapper; + if (contextWrapper == null || !contextWrapper.wraps(context)) { + contextWrapper = this.contextWrapper = new GuardedLuaContext(context, this); } + + try (var ignored = environment.time(Metrics.PERIPHERAL_OPS)) { + return method.apply(peripheral, contextWrapper, this, arguments); + } + } + + @Override + public boolean checkValid() { + return isAttached(); } // IComputerAccess implementation diff --git a/projects/core/src/main/java/dan200/computercraft/core/computer/GuardedLuaContext.java b/projects/core/src/main/java/dan200/computercraft/core/computer/GuardedLuaContext.java new file mode 100644 index 000000000..06dc65235 --- /dev/null +++ b/projects/core/src/main/java/dan200/computercraft/core/computer/GuardedLuaContext.java @@ -0,0 +1,55 @@ +// SPDX-FileCopyrightText: 2023 The CC: Tweaked Developers +// +// SPDX-License-Identifier: MPL-2.0 + +package dan200.computercraft.core.computer; + +import dan200.computercraft.api.lua.ILuaContext; +import dan200.computercraft.api.lua.LuaException; +import dan200.computercraft.api.lua.LuaTask; + +/** + * A {@link ILuaContext} which checks if context is valid when before executing + * {@linkplain #issueMainThreadTask(LuaTask) main-thread tasks}. + */ +public final class GuardedLuaContext implements ILuaContext { + private final ILuaContext original; + private final Guard guard; + + public GuardedLuaContext(ILuaContext original, Guard guard) { + this.original = original; + this.guard = guard; + } + + /** + * Determine if this {@link GuardedLuaContext} wraps another context. + *

+ * This may be used to avoid constructing new guarded contexts, in a pattern something like: + * + * 

{@code
+     * var contextWrapper = this.contextWrapper;
+     * if(contextWrapper == null || !contextWrapper.wraps(context)) {
+     *     contextWrapper = this.contextWrapper = new GuardedLuaContext(context, this);
+     * }
+     * }
+ * + * @param context The original context. + * @return Whether {@code this} wraps {@code context}. + */ + public boolean wraps(ILuaContext context) { + return original == context; + } + + @Override + public long issueMainThreadTask(LuaTask task) throws LuaException { + return original.issueMainThreadTask(() -> guard.checkValid() ? task.execute() : null); + } + + /** + * The function which checks if the context is still valid. + */ + @FunctionalInterface + public interface Guard { + boolean checkValid(); + } +}