From 8fe509ecb1a941d58a417a8da29e3de142a57328 Mon Sep 17 00:00:00 2001 From: Jonathan Coates Date: Sat, 1 Apr 2023 11:02:51 +0100 Subject: [PATCH] Properly scope IArguments to the current function call This is a horrible commit: It's a breaking change in a pretty subtle way, which means it won't be visible while updating. Fortunately I think the only mod on 1.19.4 is Plethora, but other mods (Mek, Advanced Peripherals) may be impacted when they update. Sorry! For some motivation behind the original issue: The default IArguments implementation (VarargArguments) lazily converts Lua arguments to Java ones. This is mostly important when passing tables to Java functions, as we can avoid the conversion entirely if the function uses IArguments.getTableUnsafe. However, this lazy conversion breaks down if IArguments is accessed on a separate thread, as Lua values are not thread-safe. Thus we need to perform this conversion before the cross-thread sharing occurs. Now, ideally this would be an implementation detail and entirely invisible to the user. One approach here would be to only perform this lazy conversion for methods annotated with @LuaFunction(unsafe=true), and have it be eager otherwise. However, the peripheral API gets in the way here, as it means we can no longer inspect the "actual" method being invoked. And so, alas, this must leak into the public API. TLDR: If you're getting weird errors about scope, add an IArguments.escapes() call before sharing the arguments between threads. Closes #1384 --- .../shared/turtle/apis/TurtleAPI.java | 6 +- .../computercraft/api/lua/IArguments.java | 43 ++++- .../api/lua/ObjectArguments.java | 5 + .../dan200/computercraft/core/apis/OSAPI.java | 6 +- .../computercraft/core/asm/LuaMethod.java | 2 +- .../core/asm/PeripheralMethod.java | 2 +- .../computercraft/core/lua/TableImpl.java | 2 +- .../core/lua/VarargArguments.java | 150 +++++++++++++++--- .../core/computer/ComputerBootstrap.java | 2 +- .../core/lua/VarargArgumentsTest.java | 63 ++++++++ 10 files changed, 248 insertions(+), 33 deletions(-) create mode 100644 projects/core/src/test/java/dan200/computercraft/core/lua/VarargArgumentsTest.java diff --git a/projects/common/src/main/java/dan200/computercraft/shared/turtle/apis/TurtleAPI.java b/projects/common/src/main/java/dan200/computercraft/shared/turtle/apis/TurtleAPI.java index 3d1dd6ac0..ef735d584 100644 --- a/projects/common/src/main/java/dan200/computercraft/shared/turtle/apis/TurtleAPI.java +++ b/projects/common/src/main/java/dan200/computercraft/shared/turtle/apis/TurtleAPI.java @@ -219,7 +219,7 @@ public class TurtleAPI implements ILuaAPI { * @cc.since 1.4 */ @LuaFunction - public final MethodResult place(IArguments args) { + public final MethodResult place(IArguments args) throws LuaException { return trackCommand(new TurtlePlaceCommand(InteractDirection.FORWARD, args.getAll())); } @@ -235,7 +235,7 @@ public class TurtleAPI implements ILuaAPI { * @see #place For more information about placing items. */ @LuaFunction - public final MethodResult placeUp(IArguments args) { + public final MethodResult placeUp(IArguments args) throws LuaException { return trackCommand(new TurtlePlaceCommand(InteractDirection.UP, args.getAll())); } @@ -251,7 +251,7 @@ public class TurtleAPI implements ILuaAPI { * @see #place For more information about placing items. */ @LuaFunction - public final MethodResult placeDown(IArguments args) { + public final MethodResult placeDown(IArguments args) throws LuaException { return trackCommand(new TurtlePlaceCommand(InteractDirection.DOWN, args.getAll())); } diff --git a/projects/core-api/src/main/java/dan200/computercraft/api/lua/IArguments.java b/projects/core-api/src/main/java/dan200/computercraft/api/lua/IArguments.java index 9bfffd9ea..f984ad35e 100644 --- a/projects/core-api/src/main/java/dan200/computercraft/api/lua/IArguments.java +++ b/projects/core-api/src/main/java/dan200/computercraft/api/lua/IArguments.java @@ -35,9 +35,15 @@ public interface IArguments { * * @param index The argument number. * @return The argument's value, or {@code null} if not present. + * @throws LuaException If the argument cannot be converted to Java. This should be thrown in extraneous + * circumstances (if the conversion would allocate too much memory) and should + * not be thrown if the original argument is not present or is an unsupported + * data type (such as a function or userdata). + * @throws IllegalStateException If accessing these arguments outside the scope of the original function. See + * {@link #escapes()}. */ @Nullable - Object get(int index); + Object get(int index) throws LuaException; /** * Get the type name of the argument at the specific index. @@ -49,9 +55,7 @@ public interface IArguments { * @return The name of this type. * @see LuaValues#getType(Object) */ - default String getType(int index) { - return LuaValues.getType(get(index)); - } + String getType(int index); /** * Drop a number of arguments. The returned arguments instance will access arguments at position {@code i + count}, @@ -62,7 +66,14 @@ public interface IArguments { */ IArguments drop(int count); - default Object[] getAll() { + /** + * Get an array containing all as {@link Object}s. + * + * @return All arguments. + * @throws LuaException If an error occurred while fetching an argument. + * @see #get(int) To get a single argument. + */ + default Object[] getAll() throws LuaException { var result = new Object[count()]; for (var i = 0; i < result.length; i++) result[i] = get(i); return result; @@ -421,4 +432,26 @@ public interface IArguments { default Map optTable(int index, @Nullable Map def) throws LuaException { return optTable(index).orElse(def); } + + /** + * Create a version of these arguments which escapes the scope of the current function call. + *

+ * Some {@link IArguments} implementations provide a view over the underlying Lua data structures, allowing for + * zero-copy implementations of some methods (such as {@link #getTableUnsafe(int)} or {@link #getBytes(int)}). + * However, this means the arguments can only be accessed inside the current function call. + *

+ * If the arguments escape the scope of the current call (for instance, are later accessed on the main server + * thread), then these arguments must be marked as "escaping", which may choose to perform a copy of the underlying + * arguments. + *

+ * If you are using {@link LuaFunction#mainThread()}, this will be done automatically. However, if you call + * {@link ILuaContext#issueMainThreadTask(LuaTask)} (or similar), then you will need to mark arguments as escaping + * yourself. + * + * @return An {@link IArguments} instance which can escape the current scope. May be {@code this}. + * @throws LuaException For the same reasons as {@link #get(int)}. + */ + default IArguments escapes() throws LuaException { + return this; + } } diff --git a/projects/core-api/src/main/java/dan200/computercraft/api/lua/ObjectArguments.java b/projects/core-api/src/main/java/dan200/computercraft/api/lua/ObjectArguments.java index fd3ee1019..14e36fb44 100644 --- a/projects/core-api/src/main/java/dan200/computercraft/api/lua/ObjectArguments.java +++ b/projects/core-api/src/main/java/dan200/computercraft/api/lua/ObjectArguments.java @@ -51,6 +51,11 @@ public final class ObjectArguments implements IArguments { return index >= args.size() ? null : args.get(index); } + @Override + public String getType(int index) { + return LuaValues.getType(get(index)); + } + @Override public Object[] getAll() { return args.toArray(); diff --git a/projects/core/src/main/java/dan200/computercraft/core/apis/OSAPI.java b/projects/core/src/main/java/dan200/computercraft/core/apis/OSAPI.java index 38ad39759..0d2e2347f 100644 --- a/projects/core/src/main/java/dan200/computercraft/core/apis/OSAPI.java +++ b/projects/core/src/main/java/dan200/computercraft/core/apis/OSAPI.java @@ -136,7 +136,7 @@ public class OSAPI implements ILuaAPI { * @cc.see os.pullEvent To pull the event queued */ @LuaFunction - public final void queueEvent(String name, IArguments args) { + public final void queueEvent(String name, IArguments args) throws LuaException { apiEnvironment.queueEvent(name, args.drop(1).getAll()); } @@ -165,8 +165,8 @@ public class OSAPI implements ILuaAPI { * timer from firing. * * @param token The ID of the timer to cancel. - * @see #startTimer To start a timer. * @cc.since 1.6 + * @see #startTimer To start a timer. */ @LuaFunction public final void cancelTimer(int token) { @@ -351,7 +351,7 @@ public class OSAPI implements ILuaAPI { * January 1970 in the UTC timezone. * * If called with {@code local}, returns the number of milliseconds since 1 * January 1970 in the server's local timezone. - * + *

* :::info * The {@code ingame} time zone assumes that one Minecraft day consists of 86,400,000 * milliseconds. Since one in-game day is much faster than a real day (20 minutes), this diff --git a/projects/core/src/main/java/dan200/computercraft/core/asm/LuaMethod.java b/projects/core/src/main/java/dan200/computercraft/core/asm/LuaMethod.java index f9742de15..7de22ff1b 100644 --- a/projects/core/src/main/java/dan200/computercraft/core/asm/LuaMethod.java +++ b/projects/core/src/main/java/dan200/computercraft/core/asm/LuaMethod.java @@ -10,7 +10,7 @@ import java.util.Collections; public interface LuaMethod { Generator GENERATOR = new Generator<>(LuaMethod.class, Collections.singletonList(ILuaContext.class), - m -> (target, context, args) -> context.executeMainThreadTask(() -> ResultHelpers.checkNormalResult(m.apply(target, context, args))) + m -> (target, context, args) -> context.executeMainThreadTask(() -> ResultHelpers.checkNormalResult(m.apply(target, context, args.escapes()))) ); IntCache DYNAMIC = new IntCache<>( diff --git a/projects/core/src/main/java/dan200/computercraft/core/asm/PeripheralMethod.java b/projects/core/src/main/java/dan200/computercraft/core/asm/PeripheralMethod.java index 2096ebf8d..f20b85998 100644 --- a/projects/core/src/main/java/dan200/computercraft/core/asm/PeripheralMethod.java +++ b/projects/core/src/main/java/dan200/computercraft/core/asm/PeripheralMethod.java @@ -15,7 +15,7 @@ import java.util.Arrays; public interface PeripheralMethod { Generator GENERATOR = new Generator<>(PeripheralMethod.class, Arrays.asList(ILuaContext.class, IComputerAccess.class), - m -> (target, context, computer, args) -> context.executeMainThreadTask(() -> ResultHelpers.checkNormalResult(m.apply(target, context, computer, args))) + m -> (target, context, computer, args) -> context.executeMainThreadTask(() -> ResultHelpers.checkNormalResult(m.apply(target, context, computer, args.escapes()))) ); IntCache DYNAMIC = new IntCache<>( diff --git a/projects/core/src/main/java/dan200/computercraft/core/lua/TableImpl.java b/projects/core/src/main/java/dan200/computercraft/core/lua/TableImpl.java index 68fe755dc..0ffa65d76 100644 --- a/projects/core/src/main/java/dan200/computercraft/core/lua/TableImpl.java +++ b/projects/core/src/main/java/dan200/computercraft/core/lua/TableImpl.java @@ -103,7 +103,7 @@ class TableImpl implements dan200.computercraft.api.lua.LuaTable } private void checkValid() { - if (arguments.closed) { + if (arguments.isClosed()) { throw new IllegalStateException("Cannot use LuaTable after IArguments has been released"); } } diff --git a/projects/core/src/main/java/dan200/computercraft/core/lua/VarargArguments.java b/projects/core/src/main/java/dan200/computercraft/core/lua/VarargArguments.java index d0abf9676..5f60e1abd 100644 --- a/projects/core/src/main/java/dan200/computercraft/core/lua/VarargArguments.java +++ b/projects/core/src/main/java/dan200/computercraft/core/lua/VarargArguments.java @@ -7,6 +7,8 @@ package dan200.computercraft.core.lua; import dan200.computercraft.api.lua.IArguments; import dan200.computercraft.api.lua.LuaException; import dan200.computercraft.api.lua.LuaValues; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.squiddev.cobalt.*; import javax.annotation.Nullable; @@ -16,20 +18,64 @@ import java.util.Optional; import static org.squiddev.cobalt.Constants.NAME; final class VarargArguments implements IArguments { - private static final VarargArguments EMPTY = new VarargArguments(Constants.NONE); + private static final Logger LOG = LoggerFactory.getLogger(VarargArguments.class); + + private static final VarargArguments EMPTY = new VarargArguments(Constants.NONE); + private static boolean reportedIllegalGet; + + static { + EMPTY.escapes = EMPTY.closed = true; + } - boolean closed; private final Varargs varargs; - private @Nullable Object[] cache; + + private volatile boolean closed; + private final VarargArguments root; + + private @Nullable ArraySlice cache; + private @Nullable ArraySlice typeNames; + + private boolean escapes; private VarargArguments(Varargs varargs) { this.varargs = varargs; + root = this; + } + + private VarargArguments(Varargs varargs, VarargArguments root, int offset) { + this.varargs = varargs; + this.root = root; + escapes = root.escapes; + cache = root.cache == null ? null : root.cache.drop(offset); + typeNames = root.typeNames == null ? null : root.typeNames.drop(offset); } static VarargArguments of(Varargs values) { return values == Constants.NONE ? EMPTY : new VarargArguments(values); } + boolean isClosed() { + return root.closed; + } + + private void checkAccessible() { + if (isClosed() && !escapes) throwInaccessible(); + } + + private void throwInaccessible() { + var error = new IllegalStateException("Function arguments have escaped their original scope."); + if (!reportedIllegalGet) { + reportedIllegalGet = true; + LOG.error( + "A function attempted to access arguments outside the scope of the original function. This is probably " + + "caused by the function scheduling work on the main thread. You may need to call IArguments.escapes().", + error + ); + } + + throw error; + } + @Override public int count() { return varargs.count(); @@ -38,26 +84,37 @@ final class VarargArguments implements IArguments { @Nullable @Override public Object get(int index) { + checkAccessible(); if (index < 0 || index >= varargs.count()) return null; var cache = this.cache; if (cache == null) { - cache = this.cache = new Object[varargs.count()]; + cache = this.cache = new ArraySlice<>(new Object[varargs.count()], 0); } else { - var existing = cache[index]; + var existing = cache.get(index); if (existing != null) return existing; } - return cache[index] = CobaltLuaMachine.toObject(varargs.arg(index + 1), null); + var arg = varargs.arg(index + 1); + + // This holds as either a) the arguments are not closed or b) the arguments were escaped, in which case + // tables should have been converted already. + assert !isClosed() || !(arg instanceof LuaTable) : "Converting a LuaTable after arguments were closed."; + + var converted = CobaltLuaMachine.toObject(arg, null); + cache.set(index, converted); + return converted; } @Override public String getType(int index) { + checkAccessible(); + var value = varargs.arg(index + 1); - if (value instanceof LuaTable || value instanceof LuaUserdata) { - var metatable = value.getMetatable(null); - if (metatable != null && metatable.rawget(NAME) instanceof LuaString s) return s.toString(); - } + + // If we've escaped, read it from the precomputed list, otherwise get the custom name. + var name = escapes ? (typeNames == null ? null : typeNames.get(index)) : getCustomType(value); + if (name != null) return name; return value.typeName(); } @@ -66,11 +123,15 @@ final class VarargArguments implements IArguments { public IArguments drop(int count) { if (count < 0) throw new IllegalStateException("count cannot be negative"); if (count == 0) return this; - return new VarargArguments(varargs.subargs(count + 1)); + + var newArgs = varargs.subargs(count + 1); + if (newArgs == Constants.NONE) return EMPTY; + return new VarargArguments(newArgs, this, count); } @Override public double getDouble(int index) throws LuaException { + checkAccessible(); var value = varargs.arg(index + 1); if (!(value instanceof LuaNumber)) throw LuaValues.badArgument(index, "number", value.typeName()); return value.toDouble(); @@ -78,6 +139,7 @@ final class VarargArguments implements IArguments { @Override public long getLong(int index) throws LuaException { + checkAccessible(); var value = varargs.arg(index + 1); if (!(value instanceof LuaNumber)) throw LuaValues.badArgument(index, "number", value.typeName()); return value instanceof LuaInteger ? value.toInteger() : (long) LuaValues.checkFinite(index, value.toDouble()); @@ -85,6 +147,7 @@ final class VarargArguments implements IArguments { @Override public ByteBuffer getBytes(int index) throws LuaException { + checkAccessible(); var value = varargs.arg(index + 1); if (!(value instanceof LuaString str)) throw LuaValues.badArgument(index, "string", value.typeName()); return str.toBuffer(); @@ -92,6 +155,7 @@ final class VarargArguments implements IArguments { @Override public Optional optBytes(int index) throws LuaException { + checkAccessible(); var value = varargs.arg(index + 1); if (value.isNil()) return Optional.empty(); if (!(value instanceof LuaString str)) throw LuaValues.badArgument(index, "string", value.typeName()); @@ -100,9 +164,7 @@ final class VarargArguments implements IArguments { @Override public dan200.computercraft.api.lua.LuaTable getTableUnsafe(int index) throws LuaException { - if (closed) { - throw new IllegalStateException("Cannot use getTableUnsafe after IArguments has been closed."); - } + if (isClosed()) throw new IllegalStateException("Cannot use getTableUnsafe after IArguments has been closed."); var value = varargs.arg(index + 1); if (!(value instanceof LuaTable)) throw LuaValues.badArgument(index, "table", value.typeName()); @@ -111,9 +173,7 @@ final class VarargArguments implements IArguments { @Override public Optional> optTableUnsafe(int index) throws LuaException { - if (closed) { - throw new IllegalStateException("Cannot use optTableUnsafe after IArguments has been closed."); - } + if (isClosed()) throw new IllegalStateException("Cannot use optTableUnsafe after IArguments has been closed."); var value = varargs.arg(index + 1); if (value.isNil()) return Optional.empty(); @@ -121,7 +181,61 @@ final class VarargArguments implements IArguments { return Optional.of(new TableImpl(this, (LuaTable) value)); } - public void close() { + @Override + public IArguments escapes() { + if (escapes) return this; + + var cache = this.cache; + var typeNames = this.typeNames; + + for (int i = 0, count = varargs.count(); i < count; i++) { + var arg = varargs.arg(i + 1); + + // Convert tables. + if (arg instanceof LuaTable) { + if (cache == null) cache = new ArraySlice<>(new Object[count], 0); + cache.set(i, CobaltLuaMachine.toObject(arg, null)); + } + + // Fetch custom type names. + var typeName = getCustomType(arg); + if (typeName != null) { + if (typeNames == null) typeNames = new ArraySlice<>(new String[count], 0); + typeNames.set(i, typeName); + } + } + + escapes = true; + this.cache = cache; + this.typeNames = typeNames; + return this; + } + + void close() { closed = true; } + + private static @Nullable String getCustomType(LuaValue arg) { + if (!(arg instanceof LuaTable) && !(arg instanceof LuaUserdata)) return null; + + var metatable = arg.getMetatable(null); + return metatable != null && metatable.rawget(NAME) instanceof LuaString s ? s.toString() : null; + } + + private record ArraySlice(T[] array, int offset) { + // FIXME: We should be able to remove the @Nullables if we update NullAway. + + @Nullable + T get(int index) { + return array[offset + index]; + } + + void set(int index, @Nullable T value) { + array[offset + index] = value; + } + + ArraySlice drop(int count) { + return new ArraySlice<>(array, offset + count); + } + } } diff --git a/projects/core/src/test/java/dan200/computercraft/core/computer/ComputerBootstrap.java b/projects/core/src/test/java/dan200/computercraft/core/computer/ComputerBootstrap.java index 19c70be75..afdc54afa 100644 --- a/projects/core/src/test/java/dan200/computercraft/core/computer/ComputerBootstrap.java +++ b/projects/core/src/test/java/dan200/computercraft/core/computer/ComputerBootstrap.java @@ -118,7 +118,7 @@ public class ComputerBootstrap { } @LuaFunction - public final void log(IArguments arguments) { + public final void log(IArguments arguments) throws LuaException { LOG.info("[Computer] {}", Arrays.toString(arguments.getAll())); } diff --git a/projects/core/src/test/java/dan200/computercraft/core/lua/VarargArgumentsTest.java b/projects/core/src/test/java/dan200/computercraft/core/lua/VarargArgumentsTest.java new file mode 100644 index 000000000..7a76ea562 --- /dev/null +++ b/projects/core/src/test/java/dan200/computercraft/core/lua/VarargArgumentsTest.java @@ -0,0 +1,63 @@ +// SPDX-FileCopyrightText: 2023 The CC: Tweaked Developers +// +// SPDX-License-Identifier: MPL-2.0 + +package dan200.computercraft.core.lua; + +import dan200.computercraft.api.lua.LuaException; +import org.junit.jupiter.api.Test; +import org.squiddev.cobalt.Constants; +import org.squiddev.cobalt.LuaTable; +import org.squiddev.cobalt.ValueFactory; + +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +class VarargArgumentsTest { + private static LuaTable tableWithCustomType() { + var metatable = new LuaTable(); + metatable.rawset(Constants.NAME, ValueFactory.valueOf("some type")); + + var table = new LuaTable(); + table.setMetatable(null, metatable); + return table; + } + + @Test + public void testGet() { + var args = VarargArguments.of(tableWithCustomType()); + assertEquals(Map.of(), args.get(0)); + assertEquals("some type", args.getType(0)); + } + + @Test + public void testGetAfterEscape() { + var args = VarargArguments.of(tableWithCustomType()); + args.escapes(); + args.close(); + + assertEquals(Map.of(), args.get(0)); + assertEquals("some type", args.getType(0)); + } + + @Test + public void testGetAfterEscapeDrop() throws LuaException { + var args = VarargArguments.of(ValueFactory.varargsOf(Constants.NIL, tableWithCustomType())); + args.escapes(); + args.close(); + + assertEquals(Map.of(), args.drop(1).get(0)); + assertEquals("some type", args.drop(1).getType(0)); + } + + @Test + public void testGetAfterClose() { + var args = VarargArguments.of(tableWithCustomType()); + args.close(); + + assertThrows(IllegalStateException.class, () -> args.get(0)); + assertThrows(IllegalStateException.class, () -> args.getType(0)); + } +}