From e237c28382ba4622af20504eead0b5e3f83b5190 Mon Sep 17 00:00:00 2001 From: jyelon Date: Sun, 3 Sep 2023 02:01:32 -0400 Subject: [PATCH] Refactor to use LockedWrapper guard --- .../Integration/IntegrationGameModeBase.cpp | 61 +++++++++--------- Source/Integration/IntegrationGameModeBase.h | 10 ++- Source/Integration/LockedWrapper.cpp | 19 ++++++ Source/Integration/LockedWrapper.h | 50 +++++++++++++++ Source/Integration/LuprexSockets.cpp | 62 +++++++++++++++---- Source/Integration/LuprexSockets.hpp | 17 +++-- Source/Integration/engineutil.cpp | 11 ---- 7 files changed, 166 insertions(+), 64 deletions(-) create mode 100644 Source/Integration/LockedWrapper.cpp create mode 100644 Source/Integration/LockedWrapper.h diff --git a/Source/Integration/IntegrationGameModeBase.cpp b/Source/Integration/IntegrationGameModeBase.cpp index 1904153f..8d2b841b 100644 --- a/Source/Integration/IntegrationGameModeBase.cpp +++ b/Source/Integration/IntegrationGameModeBase.cpp @@ -41,10 +41,10 @@ uint32 AIntegrationGameModeBase::Run() continue; } { - FScopeLock lk(&LuprexMutex); - Sockets->Update(); - Luprex.play_invoke_event_update(&Luprex, EngineSeconds); - Sockets->Update(); + FLockedWrapper lockedwrap(LockableWrapper); + Sockets->Update(lockedwrap); + lockedwrap->play_invoke_event_update(lockedwrap.Get(), EngineSeconds); + Sockets->Update(lockedwrap); } } return 0; @@ -52,6 +52,8 @@ uint32 AIntegrationGameModeBase::Run() void AIntegrationGameModeBase::ResetToInitialState() { + FLockedWrapper w(LockableWrapper); + // Shut down the thread and release the ThreadEvent if (Thread != nullptr) { @@ -65,12 +67,16 @@ void AIntegrationGameModeBase::ResetToInitialState() ThreadStopRequested = false; // Release and close all sockets. - Sockets.Reset(); + if (Sockets != nullptr) + { + Sockets->ForceCloseEverything(w); + Sockets.Reset(); + } // Delete the engine. - if (Luprex.release != nullptr) + if (w->release != nullptr) { - Luprex.release(&Luprex); + w->release(w.Get()); } // Reset the clocks. @@ -79,15 +85,15 @@ void AIntegrationGameModeBase::ResetToInitialState() } -void AIntegrationGameModeBase::HandleLuprexConsoleOutput() +void AIntegrationGameModeBase::HandleLuprexConsoleOutput(FLockedWrapper &w) { uint32_t ndata; const char* data; - Luprex.get_outgoing(&Luprex, 0, &ndata, &data); + w->get_outgoing(w.Get(), 0, &ndata, &data); if (ndata == 0) return; std::string_view src(data, ndata); int consumed; std::u16string cps = drvutil::utf8_to_ucs2(src, &consumed); - Luprex.play_sent_outgoing(&Luprex, 0, consumed); + w->play_sent_outgoing(w.Get(), 0, consumed); FString fs(cps.size(), (const UCS2CHAR*)(&cps[0])); ConsoleOutput.Append(fs); } @@ -96,11 +102,11 @@ void AIntegrationGameModeBase::Tick(float DeltaSeconds) { Super::Tick(DeltaSeconds); { - FScopeLock lk(&LuprexMutex); - if (Luprex.engine != nullptr) + FLockedWrapper lockedwrap(LockableWrapper); + if (lockedwrap->engine != nullptr) { EngineSeconds += DeltaSeconds; - HandleLuprexConsoleOutput(); + HandleLuprexConsoleOutput(lockedwrap); } } TArray prints = engineutil::DPrintGetStored(); @@ -121,9 +127,9 @@ void AIntegrationGameModeBase::Tick(float DeltaSeconds) void AIntegrationGameModeBase::ConsoleSendInput(const FString& fs) { - if (Luprex.engine != nullptr) + FLockedWrapper w(LockableWrapper); + if (w->engine != nullptr) { - FScopeLock lk(&LuprexMutex); const TCHAR* fstchar = *fs; if (sizeof(TCHAR) == 2) { @@ -131,7 +137,7 @@ void AIntegrationGameModeBase::ConsoleSendInput(const FString& fs) std::u16string_view fsview((const char16_t*)fstchar, fs.Len()); std::string utf8 = drvutil::utf16_to_utf8(fsview); utf8 = utf8 + "\n"; - Luprex.play_recv_incoming(&Luprex, 0, utf8.size(), utf8.c_str()); + w->play_recv_incoming(w.Get(), 0, utf8.size(), utf8.c_str()); } } } @@ -140,27 +146,26 @@ void AIntegrationGameModeBase::BeginPlay() { Super::BeginPlay(); + FLockedWrapper w(LockableWrapper); + // Sanity checks. checkf(Thread == nullptr, TEXT("There should be no thread here.")); - checkf(Luprex.engine == nullptr, TEXT("There should be no engine here.")); + checkf(w->engine == nullptr, TEXT("There should be no engine here.")); // Make sure we're starting from a clean slate. ResetToInitialState(); // Try to initialize the wrapper. - if (Luprex.play_initialize == nullptr) - { - engineutil::init_wrapper(&Luprex); - } + w.InitWrapper(); // If we failed to initialize the wrapper, print an error message. - if (Luprex.play_initialize == nullptr) + if (w->play_initialize == nullptr) { engineutil::DPrint("Luprex wrapper initialization failed"); } // If wrapper is initialized, try to initialize the luprex engine. - if (Luprex.play_initialize != nullptr) + if (w->play_initialize != nullptr) { drvutil::ostringstream srcpak; std::string srcpakerr = drvutil::package_lua_source("c:\\Luprex", &srcpak); @@ -171,10 +176,10 @@ void AIntegrationGameModeBase::BeginPlay() std::string_view srcpakv = srcpak.view(); char* argv[1]; argv[0] = const_cast("lpxserver"); - Luprex.play_initialize(&Luprex, 1, argv, srcpakv.size(), srcpakv.data(), ""); - if (Luprex.error[0]) + w->play_initialize(w.Get(), 1, argv, srcpakv.size(), srcpakv.data(), ""); + if (w->error[0]) { - engineutil::DPrint(Luprex.error); + engineutil::DPrint(w->error); } else { @@ -183,9 +188,9 @@ void AIntegrationGameModeBase::BeginPlay() } // If we successfully created a luprex engine, create a socket system and a worker thread. - if (Luprex.engine != nullptr) + if (w->engine != nullptr) { - Sockets.Reset(FLpxSockets::Create(&Luprex)); + Sockets.Reset(FLpxSockets::Create(w)); std::string error = Sockets->GetError(); check(error.empty()); ThreadEvent = FPlatformProcess::GetSynchEventFromPool(false); diff --git a/Source/Integration/IntegrationGameModeBase.h b/Source/Integration/IntegrationGameModeBase.h index 35c1c71e..37cbc11c 100644 --- a/Source/Integration/IntegrationGameModeBase.h +++ b/Source/Integration/IntegrationGameModeBase.h @@ -45,7 +45,7 @@ public: TSubclassOf ClassTangibleActor; // Transfer console output from the Luprex engine to unreal. - void HandleLuprexConsoleOutput(); + void HandleLuprexConsoleOutput(FLockedWrapper &w); UPROPERTY() FTangibleManager TangibleManager; @@ -65,11 +65,9 @@ public: // to exit. FEvent* ThreadEvent; - // This critical section guards the use of the EngineWrapper. - FCriticalSection LuprexMutex; - - // The Luprex wrapper and engine. MUST CLAIM LuprexMutex. - EngineWrapper Luprex; + // The Luprex EngineWrapper, with a Mutex to protect it. + // To access it, construct a FLockedWrapper. + FLockableWrapper LockableWrapper; // Luprex socket system. Aside from construction, only touched by Luprex thread. TUniquePtr Sockets; diff --git a/Source/Integration/LockedWrapper.cpp b/Source/Integration/LockedWrapper.cpp new file mode 100644 index 00000000..1d3c0ffe --- /dev/null +++ b/Source/Integration/LockedWrapper.cpp @@ -0,0 +1,19 @@ + +#include "LockedWrapper.h" +#include "engineutil.hpp" + +void FLockedWrapper::InitWrapper() { + if (Lockable.Wrapper.play_initialize != nullptr) { + // Already initialized. + return; + } + void* DLL = FPlatformProcess::GetDllHandle(TEXT("c:\\Luprex\\build\\visual\\luprexlib.dll")); + if (DLL != nullptr) { + using InitFn = void (*)(EngineWrapper*); + InitFn init = (InitFn)FPlatformProcess::GetDllExport(DLL, TEXT("init_engine_wrapper")); + if (init != nullptr) { + init(&Lockable.Wrapper); + Lockable.Wrapper.hook_dprint(engineutil::DPrintHook); + } + } +} \ No newline at end of file diff --git a/Source/Integration/LockedWrapper.h b/Source/Integration/LockedWrapper.h new file mode 100644 index 00000000..57b1f640 --- /dev/null +++ b/Source/Integration/LockedWrapper.h @@ -0,0 +1,50 @@ +#pragma once + +#include "CoreMinimal.h" +#include "enginewrapper.hpp" + +// Class FLockableWrapper +// +// Contains the EngineWrapper and a Mutex to lock it. +// This class has no methods. To access the EngineWrapper, +// construct a FLockedWrapper, and then dereference it +// using operator right arrow. +// +class FLockableWrapper { +private: + FCriticalSection Mutex; + EngineWrapper Wrapper; +public: + friend class FLockedWrapper; +}; + +class FLockedWrapper { +private: + FLockableWrapper& Lockable; + +public: + // The constructor of the FLockedWrapper claims the mutex. + FLockedWrapper(FLockableWrapper& w) : Lockable(w) { + Lockable.Mutex.Lock(); + } + + // The destructor of the FLockedWrapper releases the mutex. + ~FLockedWrapper() { + Lockable.Mutex.Unlock(); + } + + // Initialize the engine wrapper if it's not already. + void InitWrapper(); + + // Operator right arrow accesses the EngineWrapper. + EngineWrapper* operator ->() { + return &Lockable.Wrapper; + } + + // Get a pointer to the enginewrapper. This is not + // very safe because you could keep the pointer after + // the LockedWrapper is destroyed. Don't do that. + EngineWrapper* Get() { + return &Lockable.Wrapper; + } +}; diff --git a/Source/Integration/LuprexSockets.cpp b/Source/Integration/LuprexSockets.cpp index 0893c75b..e8a9045d 100644 --- a/Source/Integration/LuprexSockets.cpp +++ b/Source/Integration/LuprexSockets.cpp @@ -172,7 +172,7 @@ public: FLpxChannel(FLpxSocketsI *lsi, FSocket* sock, int chid, SSL_CTX* ctx, EChanState state); FLpxChannel() : FLpxChannel(nullptr, nullptr, 0, nullptr, CHAN_INACTIVE) {} - ~FLpxChannel() { Close(""); } + ~FLpxChannel() { } }; ///////////////////////////////////////////////////////////////// @@ -187,8 +187,8 @@ public: // Fatal error status. std::string FatalError; - // We don't own the wrapper, we just have a pointer to it. - // We require a guarantee that it outlives us. + // This pointer is NULL except when inside + // one of the methods that accepts a LockedWrapper. EngineWrapper* Luprex; // A general-purpose character buffer. @@ -206,7 +206,7 @@ public: SSL_CTX* ClientSecureCTX; SSL_CTX* ClientInsecureCTX; - FLpxSocketsI(EngineWrapper* w); + FLpxSocketsI(FLockedWrapper &w); virtual ~FLpxSocketsI() override; // Copy the trace to the DPrint output. @@ -226,8 +226,11 @@ public: void HandleNewOutgoingSockets(); void HandleSocketInputOutput(); + // Force Close Everything. + virtual void ForceCloseEverything(FLockedWrapper& w); + // Main update routine. - virtual void Update() override; + virtual void Update(FLockedWrapper &w) override; }; ///////////////////////////////////////////////////////////////// @@ -807,8 +810,11 @@ void FLpxSocketsI::SetError(const std::string& s) } -FLpxSocketsI::FLpxSocketsI(EngineWrapper *w) +FLpxSocketsI::FLpxSocketsI(FLockedWrapper &w) { + // We retain this pointer only so long as we have the wrapper lock. + Luprex = w.Get(); + // This function is nonreentrant. It's not clear whether // this is needed - it may be initialized elsewhere in unreal. // It is also not clear that it's safe to do this in the @@ -816,7 +822,6 @@ FLpxSocketsI::FLpxSocketsI(EngineWrapper *w) // thread). SSL_library_init(); - Luprex = w; ServerCTX = nullptr; ClientSecureCTX = nullptr; ClientInsecureCTX = nullptr; @@ -845,27 +850,52 @@ FLpxSocketsI::FLpxSocketsI(EngineWrapper *w) names = names + " " + name; } HandleListenPorts(); + + // We're losing the wrapper lock, so set the pointer to nullptr. + Luprex = nullptr; +} + +void FLpxSocketsI::ForceCloseEverything(FLockedWrapper& w) +{ + // We retain this pointer only so long as we have the wrapper lock. + Luprex = w.Get(); + + // Close all channels + for (FLpxChannel& chan : Channels) { + chan.Close("Force Close Everything"); + } + + // Delete any channels released by the above. + RemoveInactiveChannels(); + + // All channels should be gone now. + check(Channels.IsEmpty()); + + // We're losing the wrapper lock, so set the pointer to nullptr. + Luprex = nullptr; } FLpxSocketsI::~FLpxSocketsI() { + checkf(Channels.IsEmpty(), TEXT("Must call ForceCloseEverything before destructor")); + if (ServerCTX != nullptr) { SSL_CTX_free(ServerCTX); + ServerCTX = nullptr; } if (ClientSecureCTX != nullptr) { SSL_CTX_free(ClientSecureCTX); + ClientSecureCTX = nullptr; } if (ClientInsecureCTX != nullptr) { SSL_CTX_free(ClientInsecureCTX); + ClientInsecureCTX = nullptr; } - // Cleanup - //for (ChanInfo& chan : chans_) { - // close_channel(chan, ""); - //} + // TODO: Be more thorough. } void FLpxSocketsI::DPrintTrace() @@ -988,13 +1018,19 @@ void FLpxSocketsI::HandleSocketInputOutput() RemoveInactiveChannels(); } -void FLpxSocketsI::Update() +void FLpxSocketsI::Update(FLockedWrapper &w) { + // We retain this pointer only so long as we have the wrapper lock. + Luprex = w.Get(); + HandleNewOutgoingSockets(); HandleSocketInputOutput(); + + // We're losing the wrapper lock, so set the pointer to nullptr. + Luprex = nullptr; } -FLpxSockets* FLpxSockets::Create(EngineWrapper* w) +FLpxSockets* FLpxSockets::Create(FLockedWrapper &w) { return new FLpxSocketsI(w); } diff --git a/Source/Integration/LuprexSockets.hpp b/Source/Integration/LuprexSockets.hpp index c6a00c07..9d5135e9 100644 --- a/Source/Integration/LuprexSockets.hpp +++ b/Source/Integration/LuprexSockets.hpp @@ -1,6 +1,7 @@ #pragma once #include "CoreMinimal.h" +#include "LockedWrapper.h" #include // Class FLpxSockets @@ -33,15 +34,19 @@ protected: public: // Create the Luprex socket system. - static FLpxSockets* Create(EngineWrapper *w); + static FLpxSockets* Create(FLockedWrapper &w); - // Delete the luprex socket system. Cleanly closes all sockets. + // Close all sockets. + virtual void ForceCloseEverything(FLockedWrapper& w) = 0; + + // The update routine relays data into and out of + // luprex via TCP/IP sockets. + virtual void Update(FLockedWrapper& w) = 0; + + // Delete the luprex socket system. + // You must call ForceCloseEverything first. virtual ~FLpxSockets() {} // Obtain any error status report. virtual std::string GetError() = 0; - - // The update routine relays data into and out of - // luprex via TCP/IP sockets. - virtual void Update() = 0; }; \ No newline at end of file diff --git a/Source/Integration/engineutil.cpp b/Source/Integration/engineutil.cpp index a9cbd96f..fcb69e45 100644 --- a/Source/Integration/engineutil.cpp +++ b/Source/Integration/engineutil.cpp @@ -3,17 +3,6 @@ namespace engineutil { -void init_wrapper(EngineWrapper* w) { - void* DLL = FPlatformProcess::GetDllHandle(TEXT("c:\\Luprex\\build\\visual\\luprexlib.dll")); - if (DLL != nullptr) { - using InitFn = void (*)(EngineWrapper*); - InitFn init = (InitFn)FPlatformProcess::GetDllExport(DLL, TEXT("init_engine_wrapper")); - if (init != nullptr) { - init(w); - w->hook_dprint(engineutil::DPrintHook); - } - } -} // The DPrint array. This stores the dprints // until they can be collected by the console implementation.