Suspend core by default when calling plugin command functions

[myk002]

Fixes: #5102
Should be tested with #5075

[ab9rf]

as per comments

i am especially not comfortable with adding a core suspender to viewscreen render or logic methods

[myk002]

as per comments

fixed

i am especially not comfortable with adding a core suspender to viewscreen render or logic methods

what about overlay.cpp, where we call into Lua from render, logic, and feed?

[ab9rf]

what about overlay.cpp, where we call into Lua from render, logic, and feed?

Might be needed there, since that's going to be using the Lua state in Core, which is shared across threads and needs to be arbitrated somehow. I don't know if the lock in the lua_State is sufficient here. @lethosor may be in a better position to offer an opinion here than I am; he is more familiar with that code than I am

update: I looked at the code used in overlay and it uses the lua_State from Core in ways that do not acquire the lock in the state so it does need to be arbitrated. So in this case we need to use some means to arbitrate access to Core::State to prevent two threads from using it simultaneously. We're basically using core suspenders for two not necessarily related purposes here, one to protect from unarbitrated access to DFHack's global state (specifically, the shared Lua state) from more than one thread, and also to protect from unarbitrated access to DF's global state from more than one state. Perhaps these should be distinct mutexes. I don't know.

[ab9rf]

Having looked over the CoreSuspender semantics I think this is probably OK.

We do need to ensure that access to the global Lua state is properly arbitrated, but this isn't the PR to do that.

[lethosor]

I agree suspending in a vmethod seems very unsafe to me. Did we remove CoreSuspendClaimer? I'm guessing from context that it was intended to be used as a stand-in for CoreSuspender on the render thread to prevent deadlocks. It's possible other things have changed since I Iast looked at this code that make this safer now.

[ab9rf]

Sorry I missed a mistake on review. There's a CoreSuspender in Core::Shutdown that absolutely should not be there

[ab9rf]

This shouldn't be here at all. This is in Core::Shutdown, the core suspension mechanism has already been dismantled at this point and all DFHack threads, as well as the DF simulation thread, have been terminated. The suspender should be removed entirely since there's nothing to protect against here.

This usage came up when I added instrumentation to detect attempts to acquire a CoreSuspender from the DF render thread, because doing this is unsafe. It happens that DF calls dfhooks_shutdown from the render (main) thread, so the attempt to acquire a CoreSuspender here set off the instrumentation.

[myk002]

Fixed in ebac377

[myk002]

Did we remove CoreSuspendClaimer?

CoreSuspendClaimer has been an alias for CoreSuspender since #1304 (merged in 2018). I just removed the alias to make the now-uniform semantics clearer.

[ab9rf]

I'm guessing from context that it was intended to be used as a stand-in for CoreSuspender on the render thread to prevent deadlocks.

it was aliased to CoreSuspender in a 2018 change. Also, there is simply no safe way to use a CoreSuspender from the render thread, or indeed do anything that might block the render thread, without risking a deadlock, because the render thread can be (and often is) blocking the simulation thread. This is why we have the hotkey thread and the hotkey mailslot to move work units from the render thread, which we cannot block, to the hotkey thread, which we can. This allows us to initiate work units that need a suspended core from the render thread in a safe manner.