Skip to content

Commit

Permalink
Skip main-thread tasks if peripheral is detached
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
SquidDev committed Mar 21, 2024
1 parent 9b63cc8 commit 04900dc
Show file tree
Hide file tree
Showing 4 changed files with 100 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
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;

Expand All @@ -26,12 +27,16 @@ public final class GenericPeripheral implements IDynamicPeripheral {
private final Set<String> additionalTypes;
private final List<SaturatedMethod> methods;

private @Nullable GuardedLuaContext contextWrapper;
private final GuardedLuaContext.Guard guard;

GenericPeripheral(BlockEntity tile, Direction side, String type, Set<String> additionalTypes, List<SaturatedMethod> 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() {
Expand All @@ -47,7 +52,12 @@ public String[] getMethodNames() {

@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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
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;
Expand Down Expand Up @@ -304,7 +305,7 @@ private void attachPeripheralImpl(IComputerAccess computer, ConcurrentMap<String
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;
Expand All @@ -317,6 +318,8 @@ private static class RemotePeripheralWrapper implements IComputerAccess {
private volatile boolean attached;
private final Set<String> mounts = new HashSet<>();

private @Nullable GuardedLuaContext contextWrapper;

RemotePeripheralWrapper(WiredModemElement element, IPeripheral peripheral, IComputerAccess computer, String name, Map<String, PeripheralMethod> methods) {
this.element = element;
this.peripheral = peripheral;
Expand Down Expand Up @@ -364,7 +367,19 @@ public Collection<String> getMethodNames() {
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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
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;
Expand All @@ -26,7 +27,7 @@
* @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;

Expand All @@ -35,6 +36,8 @@ private class PeripheralWrapper extends ComputerAccess {
private final Map<String, PeripheralMethod> methodMap;
private boolean attached = false;

private @Nullable GuardedLuaContext contextWrapper;

PeripheralWrapper(IPeripheral peripheral, String side) {
super(environment);
this.side = side;
Expand Down Expand Up @@ -91,11 +94,23 @@ public MethodResult call(ILuaContext context, String methodName, IArguments argu

if (method == null) throw new LuaException("No such method " + methodName);

// 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, context, this, arguments);
return method.apply(peripheral, contextWrapper, this, arguments);
}
}

@Override
public boolean checkValid() {
return isAttached();
}

// IComputerAccess implementation

@Nullable
Expand Down
Original file line number Diff line number Diff line change
@@ -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.
* <p>
* This may be used to avoid constructing new guarded contexts, in a pattern something like:
*
* <pre>{@code
* var contextWrapper = this.contextWrapper;
* if(contextWrapper == null || !contextWrapper.wraps(context)) {
* contextWrapper = this.contextWrapper = new GuardedLuaContext(context, this);
* }
* }</pre>
*
* @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();
}
}

0 comments on commit 04900dc

Please sign in to comment.