I was running unit tests in parallel, each of which does the following:
- Create a new
LuaState just for this test
- Add one of my
[LuaObjects] to the LuaState.Environment
- Run some Lua code that uses the
[LuaObject] I added to the environment.
The tests were not sharing a LuaState or the same instance of a [LuaObject].
Some of the tests would fail with this exception:
Lua.LuaRuntimeException : Lua-CSharp: [string "obj:MyMethod()"]:1: attempt to index a userdata value (global 'obj')
at Lua.LuaRuntimeException.AttemptInvalidOperationOnLuaStack(LuaState state, String op, Int32 lastPc, Int32 reg)
at Lua.Runtime.LuaVirtualMachine.<GetTableValueSlowPath>g__ThrowInvalidOperationWithName|24_0(<>c__DisplayClass24_0&)
at Lua.Runtime.LuaVirtualMachine.GetTableValueSlowPath(LuaValue table, LuaValue key, VirtualMachineExecutionContext context, LuaValue& value, Boolean& doRestart)
at Lua.Runtime.LuaVirtualMachine.MoveNext(VirtualMachineExecutionContext context)
at Lua.Runtime.LuaVirtualMachine.VirtualMachineExecutionContext.ExecuteClosureAsyncImpl()
at Lua.LuaState.RunAsync(LuaFunction function, Int32 argumentCount, Int32 returnBase, CancellationToken cancellationToken)
at Lua.LuaStateExtensions.ExecuteAsync(LuaState state, LuaClosure closure, CancellationToken cancellationToken)
Workaround: I was able to work around it by calling GC.KeepAlive(MyObj.Metatable) one time before the tests start. (GC.KeepAlive() didn't actually do anything; it was just a way to call the Metatable property's getter and do nothing with it.)
Suggestion: I think there is a concurrency issue here, if multiple threads are concurrently getting the Metatable property for the first time:
|
builder.AppendLine("__metatable = new();"); |
|
foreach (var metamethod in metamethods) |
|
{ |
|
var metaMethodName = metamethod.ToString(); |
|
var lowerName = metaMethodName.ToLower(); |
|
builder.AppendLine( |
|
$"__metatable[global::Lua.Runtime.Metamethods.{metaMethodName}] = __metamethod_{lowerName};" |
|
); |
|
} |
That lazily initializes __metatable and then mutates it after assigning it. So if multiple threads are there, they could potentially be mutating the same object at the same time and corrupt it. (That is, a thread could call __metatable = new();, but by the next line, __metatable could now be a different object from the one this thread just assigned to it.)
So maybe this:
__metatable = new();
__metatable[global::Lua.Runtime.Metamethods.Index] = __metamethod_index;
__metatable[global::Lua.Runtime.Metamethods.NewIndex] = __metamethod_newindex;
return __metatable;
Needs to be more like this to guarantee that the LuaTable is only mutated by a single thread during initialization:
var metatable = new global::Lua.LuaTable();
metatable[global::Lua.Runtime.Metamethods.Index] = __metamethod_index;
metatable[global::Lua.Runtime.Metamethods.NewIndex] = __metamethod_newindex;
__metatable = metatable;
return __metatable;
Minimal repro with NUnit:
[LuaObject]
public partial class MyObj
{
[LuaMember(nameof(MyMethod))]
public void MyMethod()
{
}
}
public class ReproTest
{
[Test]
public async ValueTask Test1()
{
var state = LuaState.Create();
state.Environment["obj"] = new MyObj();
await state.DoStringAsync("obj:MyMethod()");
}
[Test]
public async ValueTask Test2()
{
var state = LuaState.Create();
state.Environment["obj"] = new MyObj();
await state.DoStringAsync("obj:MyMethod()");
}
[Test]
public async ValueTask Test3()
{
var state = LuaState.Create();
state.Environment["obj"] = new MyObj();
await state.DoStringAsync("obj:MyMethod()");
}
[Test]
public async ValueTask Test4()
{
var state = LuaState.Create();
state.Environment["obj"] = new MyObj();
await state.DoStringAsync("obj:MyMethod()");
}
}
I was running unit tests in parallel, each of which does the following:
LuaStatejust for this test[LuaObjects]to theLuaState.Environment[LuaObject]I added to the environment.The tests were not sharing a
LuaStateor the same instance of a[LuaObject].Some of the tests would fail with this exception:
Workaround: I was able to work around it by calling
GC.KeepAlive(MyObj.Metatable)one time before the tests start. (GC.KeepAlive()didn't actually do anything; it was just a way to call theMetatableproperty's getter and do nothing with it.)Suggestion: I think there is a concurrency issue here, if multiple threads are concurrently getting the
Metatableproperty for the first time:Lua-CSharp/src/Lua.SourceGenerator/LuaObjectGenerator.Emit.cs
Lines 1190 to 1198 in c48134e
That lazily initializes
__metatableand then mutates it after assigning it. So if multiple threads are there, they could potentially be mutating the same object at the same time and corrupt it. (That is, a thread could call__metatable = new();, but by the next line,__metatablecould now be a different object from the one this thread just assigned to it.)So maybe this:
Needs to be more like this to guarantee that the
LuaTableis only mutated by a single thread during initialization:Minimal repro with NUnit: