Skip to content

Commit

Permalink
fix(core): synchronize more shared mutable variables between threads
Browse files Browse the repository at this point in the history 
Less likely to have race condition
  • Loading branch information
@kagikn
kagikn committed Jan 14, 2024
1 parent 6a845f6 commit 67296d3
Show file tree
Hide file tree
Showing 2 changed files with 81 additions and 36 deletions.
38 changes: 20 additions & 18 deletions source/core/DllMain.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ public ref class ScriptHookVDotNet // This is not a static class, so that consol
static array<WinForms::Keys>^ consoleKeyBinding = { WinForms::Keys::F4 };
static unsigned int scriptTimeoutThreshold = 5000;
static bool shouldWarnOfScriptsBuiltAgainstDeprecatedApiWithTicker = true;
static Object^ unloadLock = gcnew Object();
static Object^ keyboardMessageLock = gcnew Object();
static array<bool>^ _keyboardStatePool = gcnew array<bool>(256);
static ModifierKeyState _modKeyState = ModifierKeyState::None;

Expand Down Expand Up @@ -397,7 +397,7 @@ static void ScriptHookVDotNet_ManagedInit()

// Unload previous domain (this unloads all script assemblies too)
{
msclr::lock l(ScriptHookVDotNet::unloadLock);
msclr::lock l(ScriptHookVDotNet::keyboardMessageLock);

if (domain != nullptr)
{
Expand All @@ -411,7 +411,6 @@ static void ScriptHookVDotNet_ManagedInit()
SHVDN::ScriptDomain::Unload(domain);
domain = nullptr;
}

}

// Clear log from previous runs
Expand Down Expand Up @@ -608,7 +607,8 @@ static void ScriptHookVDotNet_ManagedKeyboardMessage(unsigned long keycode, bool
if (alt) keys = keys | WinForms::Keys::Alt;

// Protect against race condition during reload
msclr::lock l(ScriptHookVDotNet::unloadLock);
msclr::lock l(ScriptHookVDotNet::keyboardMessageLock);

SHVDN::Console^ console = ScriptHookVDotNet::console;
if (console != nullptr)
{
Expand Down Expand Up @@ -648,9 +648,9 @@ static void ScriptHookVDotNet_ManagedKeyboardMessage(unsigned long keycode, bool
// solely for detection for recreation of the game session
PVOID sOldGameFiber = nullptr;

HANDLE hClrThread;
HANDLE hClrWaitEvent;
HANDLE hClrContinueEvent;
std::atomic<HANDLE> hClrThread;
std::atomic<HANDLE> hClrWaitEvent;
std::atomic<HANDLE> hClrContinueEvent;

// proper synchronization with event objects would cause a deadlock or timeout (for executing longer than 2 seconds)
// in DllMain, so use a bool variable to tell procedures that the asi wants to get freed
Expand All @@ -662,7 +662,7 @@ std::atomic_bool sClrThreadRequestedToExit(false);
static DWORD ClrThreadProc(LPVOID lparam)
{
// DllMain gets called before ScriptHookV starts script fibers, so wait until getting signaled by ScriptMain
WaitForSingleObject(hClrContinueEvent, INFINITE);
WaitForSingleObject(hClrContinueEvent.load(), INFINITE);

while (!sClrThreadRequestedToExit) {
ScriptHookVDotNet_ManagedInit();
Expand All @@ -672,7 +672,7 @@ static DWORD ClrThreadProc(LPVOID lparam)
// exiting the loop after letting the main fiber execute
while (!sScriptDomainRequestedToReload.load() && !sClrThreadRequestedToExit.load()) {
ScriptHookVDotNet_ManagedTick();
SetEvent(hClrWaitEvent);
SetEvent(hClrWaitEvent.load());
WaitForSingleObject(hClrContinueEvent, INFINITE);
}
}
Expand Down Expand Up @@ -710,14 +710,16 @@ static void ScriptMain()
break;
}

SetEvent(hClrContinueEvent);
SetEvent(hClrContinueEvent.load());
// This call blocks the main thread so GtaThread instances (which ysc or external scripts internally rely on)
// won't be executed except for one for the SHVDN runtime as long as it is executing
WaitForSingleObject(hClrWaitEvent, INFINITE);
WaitForSingleObject(hClrWaitEvent.load(), INFINITE);
scriptWait(0);
}
}

// Keyboard event runs in a thread different from where tick event runs in Script Hook V, so we may want to synchronize
// data between them!
static void ScriptKeyboardMessage(DWORD key, WORD repeats, BYTE scanCode, BOOL isExtended, BOOL isWithAlt, BOOL wasDownBefore, BOOL isUpNow)
{
ScriptHookVDotNet_ManagedKeyboardMessage(
Expand All @@ -740,8 +742,8 @@ BOOL WINAPI DllMain(HMODULE hModule, DWORD fdwReason, LPVOID lpvReserved)
// Register handler for keyboard messages
keyboardHandlerRegister(ScriptKeyboardMessage);

hClrContinueEvent = CreateEvent(NULL, false, false, NULL);
hClrWaitEvent = CreateEvent(NULL, false, false, NULL);
hClrContinueEvent.store(CreateEvent(NULL, false, false, NULL));
hClrWaitEvent.store(CreateEvent(NULL, false, false, NULL));

// Create a seperate thread to run CLR so in .NET runtime won't panic for (false) stack overflow by a cached stack
// limit in .NET runtime
Expand All @@ -758,13 +760,13 @@ BOOL WINAPI DllMain(HMODULE hModule, DWORD fdwReason, LPVOID lpvReserved)

// Gracefully let the CLR thread procedure and the script fiber exit (CloseHandle does not set events)
// Probably these calls do not wake up ClrThreadProc however
SetEvent(hClrContinueEvent);
SetEvent(hClrWaitEvent);
SetEvent(hClrContinueEvent.load());
SetEvent(hClrWaitEvent.load());

// Cleanup events and thread handles so the exe can exit
CloseHandle(hClrContinueEvent);
CloseHandle(hClrWaitEvent);
CloseHandle(hClrThread);
CloseHandle(hClrContinueEvent.load());
CloseHandle(hClrWaitEvent.load());
CloseHandle(hClrThread.load());
// Unregister ScriptHookVDotNet native script
scriptUnregister(hModule);
// Unregister handler for keyboard messages
Expand Down
79 changes: 61 additions & 18 deletions source/core/ScriptDomain.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
using System.Text.RegularExpressions;
using System.Threading.Tasks;
using System.Text;
using System.Collections.ObjectModel;

namespace SHVDN
{
Expand All @@ -37,24 +38,33 @@ public sealed class ScriptDomain : MarshalByRefObject, IDisposable
[DllImport("kernel32.dll")]
private static extern uint GetCurrentThreadId();

private static ScriptDomain s_currentDomain;
// `Dispose` won't be called in this instance because it may be too difficult to call correctly due to how
// separate `AppDomain`s have different static variables, but it's acceptable since `ReaderWriterLockSlim` uses
// `EventWaitHandle`, which uses `SafeHandle`.
private static readonly ReaderWriterLockSlim s_currentDomainPropertyLock = new();

private int _executingThreadId = Thread.CurrentThread.ManagedThreadId;
private Script _executingScript = null;
private List<IntPtr> _pinnedStrings = new();
private List<Script> _runningScripts = new();
private Queue<IScriptTask> _taskQueue = new();
private Dictionary<string, int> _scriptInstances = new();
private SortedList<string, Tuple<string, Type>> _scriptTypes = new();
private readonly List<IntPtr> _pinnedStrings = new();
private readonly List<Script> _runningScripts = new();
private readonly Queue<IScriptTask> _taskQueue = new();
private readonly Dictionary<string, int> _scriptInstances = new();
private readonly SortedList<string, Tuple<string, Type>> _scriptTypes = new();
private bool _recordKeyboardEvents = true;
private bool[] _keyboardState = new bool[256];
private List<Assembly> _scriptApis = new List<Assembly>();
private readonly List<Assembly> _scriptApis = new List<Assembly>();

private unsafe delegate* unmanaged[Cdecl]<IntPtr> _getTlsContext;
private unsafe delegate* unmanaged[Cdecl]<IntPtr, void> _setTlsContext;

private IntPtr _tlsContextOfMainThread;
private uint _gameMainThreadIdUnmanaged;

private ReaderWriterLockSlim _lockForTlsVariables = new ReaderWriterLockSlim();
// Since we read `_pinnedStrings` sequentially to dispose pinned strings and `ConcurrentQueue` doesn't have
// `Clear` method in .NET Framework where the head and tail will be set to a new segment instance,
private readonly object _pinnedStrListLock = new();
private readonly ReaderWriterLockSlim _tlsVariablesLock = new();

/// <summary>
/// The byte array that contains "CELL_EMAIL_BCON", which is used when SHVDN logs unhandled exceptions
Expand All @@ -68,7 +78,7 @@ public sealed class ScriptDomain : MarshalByRefObject, IDisposable
internal unsafe void InitTlsStuffForTlsContextSwitch(IntPtr getTlsContextFunc, IntPtr setTlsContextFunc,
IntPtr tlsAddr, uint threadId)
{
_lockForTlsVariables.EnterWriteLock();
_tlsVariablesLock.EnterWriteLock();
try
{
_getTlsContext = (delegate* unmanaged[Cdecl]<IntPtr>)getTlsContextFunc;
Expand All @@ -78,7 +88,7 @@ internal unsafe void InitTlsStuffForTlsContextSwitch(IntPtr getTlsContextFunc, I
}
finally
{
_lockForTlsVariables.ExitWriteLock();
_tlsVariablesLock.ExitWriteLock();
}
}

Expand Down Expand Up @@ -112,7 +122,33 @@ internal void InitNativeNemoryMembers()
/// <summary>
/// Gets the scripting domain for the current application domain.
/// </summary>
public static ScriptDomain CurrentDomain { get; private set; }
public static ScriptDomain CurrentDomain
{
get
{
s_currentDomainPropertyLock.EnterReadLock();
try
{
return s_currentDomain;
}
finally
{
s_currentDomainPropertyLock.ExitReadLock();
}
}
set
{
s_currentDomainPropertyLock.EnterWriteLock();
try
{
s_currentDomain = value;
}
finally
{
s_currentDomainPropertyLock.ExitWriteLock();
}
}
}

/// <summary>
/// Gets the list of currently running scripts in this script domain. This is used by the console implementation.
Expand Down Expand Up @@ -186,7 +222,7 @@ public void Dispose()

private void DisposeUnmanagedResource()
{
_lockForTlsVariables.Dispose();
_tlsVariablesLock.Dispose();
// Need to free native strings when disposing the script domain
CleanupStrings();
// Need to free unmanaged resources in NativeMemory
Expand Down Expand Up @@ -706,7 +742,7 @@ public void AbortScripts(string filename)
/// <param name="task">The task to execute.</param>
public void ExecuteTaskWithGameThreadTlsContext(IScriptTask task)
{
_lockForTlsVariables.EnterReadLock();
_tlsVariablesLock.EnterReadLock();
try
{
if (_gameMainThreadIdUnmanaged == GetCurrentThreadId())
Expand Down Expand Up @@ -735,7 +771,7 @@ public void ExecuteTaskWithGameThreadTlsContext(IScriptTask task)
}
finally
{
_lockForTlsVariables.ExitReadLock();
_tlsVariablesLock.ExitReadLock();
}
}

Expand Down Expand Up @@ -870,12 +906,15 @@ internal void DoKeyEvent(Keys keys, bool status)
/// </summary>
private void CleanupStrings()
{
foreach (IntPtr handle in _pinnedStrings)
lock (_pinnedStrListLock)
{
Marshal.FreeCoTaskMem(handle);
}
foreach (IntPtr handle in _pinnedStrings)
{
Marshal.FreeCoTaskMem(handle);
}

_pinnedStrings.Clear();
_pinnedStrings.Clear();
}
}
/// <summary>
/// Pins the memory of a string so that it can be used in native calls without worrying about the GC invalidating its pointer.
Expand All @@ -891,7 +930,11 @@ public IntPtr PinString(string str)
return NativeMemory.NullString;
}

_pinnedStrings.Add(handle);
lock (_pinnedStrListLock)
{
_pinnedStrings.Add(handle);
}

return handle;
}

Expand Down

0 comments on commit 67296d3

Please sign in to comment.