Skip to content

Thread safety for [LuaObject] lazy initialization of Metatable property #309

@RobinsonWM

Description

@RobinsonWM

I was running unit tests in parallel, each of which does the following:

  1. Create a new LuaState just for this test
  2. Add one of my [LuaObjects] to the LuaState.Environment
  3. 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()");
    }
}

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions