Skip to content

Commit 4e3832f

Browse files
Synchronize changes from 1.6 master branch [ci skip]
1dc9368 Fix some threadpool flaws, including: - Memory leaks - Infinite loop - Race condition - Server/resource shutdown
2 parents d8082dc + 1dc9368 commit 4e3832f

File tree

3 files changed

+82
-19
lines changed

3 files changed

+82
-19
lines changed

Server/mods/deathmatch/logic/CGame.cpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -365,6 +365,9 @@ CGame::~CGame()
365365
// Stop async task scheduler
366366
SAFE_DELETE(m_pAsyncTaskScheduler);
367367

368+
// Shutdown thread pool before destroying resources to prevent tasks being enqueued during shutdown
369+
CThreadPool::getDefaultThreadPool().shutdown();
370+
368371
// Destroy our stuff
369372
SAFE_DELETE(m_pResourceManager);
370373

@@ -423,7 +426,6 @@ CGame::~CGame()
423426
SAFE_DELETE(m_pASE);
424427
SAFE_RELEASE(m_pHqComms);
425428
CSimControl::Shutdown();
426-
CThreadPool::getDefaultThreadPool().shutdown();
427429

428430
// Clear our global pointer
429431
g_pGame = NULL;

Server/mods/deathmatch/logic/CResource.cpp

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -604,11 +604,28 @@ bool CResource::GenerateChecksums()
604604

605605
for (auto& task : checksumTasks)
606606
{
607-
const auto& result = task.get();
608-
if (!result.empty())
607+
try
609608
{
610-
m_strFailureReason = result;
611-
CLogger::LogPrintf(result);
609+
const auto& result = task.get();
610+
if (!result.empty())
611+
{
612+
m_strFailureReason = result;
613+
CLogger::LogPrintf(result);
614+
bOk = false;
615+
}
616+
}
617+
catch (const std::future_error& e)
618+
{
619+
// Became invalid (e.g., during shutdown)
620+
m_strFailureReason = SString("Checksum task failed: %s", e.what());
621+
CLogger::LogPrintf(m_strFailureReason);
622+
bOk = false;
623+
}
624+
catch (const std::exception& e)
625+
{
626+
// Task threw
627+
m_strFailureReason = SString("Checksum error: %s", e.what());
628+
CLogger::LogPrintf(m_strFailureReason);
612629
bOk = false;
613630
}
614631
}

Shared/sdk/SharedUtil.ThreadPool.h

Lines changed: 58 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
#include <vector>
1616
#include <algorithm>
1717
#include <type_traits>
18+
#include <cassert>
1819
#include "SharedUtil.Misc.h"
1920

2021
namespace SharedUtil
@@ -40,8 +41,17 @@ namespace SharedUtil
4041
task = std::move(m_tasks.front());
4142
m_tasks.pop();
4243
}
43-
// Run the task
44-
task(false);
44+
// Run the task (catch exceptions to prevent thread death)
45+
try
46+
{
47+
task(false);
48+
}
49+
catch (...)
50+
{
51+
// Exception is automatically captured by std::packaged_task
52+
// and will be re-thrown when future.get() is called.
53+
// We must catch here to prevent the worker thread from terminating.
54+
}
4555
}
4656
});
4757
}
@@ -51,25 +61,30 @@ namespace SharedUtil
5161
auto enqueue(Func&& f, Args&&... args)
5262
{
5363
#if __cplusplus < 201703L // C++17
54-
using ReturnT = std::result_of_t<Func, Args...>;
64+
using ReturnT = typename std::result_of<Func(Args...)>::type;
5565
#else
5666
using ReturnT = std::invoke_result_t<Func, Args...>;
5767
#endif
5868
auto ff = std::bind(std::forward<Func>(f), std::forward<Args>(args)...);
59-
auto* task = new std::packaged_task<ReturnT()>(ff);
69+
auto task = std::make_shared<std::packaged_task<ReturnT()>>(ff);
6070

6171
// Package the task in a wrapper with a common void result
6272
// plus a skip flag for destruction without running the task
6373
std::packaged_task<void(bool)> resultTask([task](bool skip) {
6474
if (!skip)
6575
(*task)();
66-
delete task;
76+
// task automatically deleted when shared_ptr goes out of scope
6777
});
6878

6979
// Add task to queue and return future
7080
std::future<ReturnT> res = task->get_future();
7181
{
7282
std::unique_lock<std::mutex> lock(m_mutex);
83+
if (m_exit)
84+
{
85+
// Pool is shutting down - reject new tasks
86+
throw std::runtime_error("Cannot enqueue task: thread pool is shutting down");
87+
}
7388
m_tasks.emplace(std::move(resultTask));
7489
}
7590
m_cv.notify_one();
@@ -78,19 +93,36 @@ namespace SharedUtil
7893

7994
void shutdown()
8095
{
81-
if (m_exit)
82-
return;
83-
84-
// Ensure every thread receives the exit state, and discard all remaining tasks.
8596
{
8697
std::unique_lock<std::mutex> lock(m_mutex);
98+
99+
// Already shutting down or shut down
100+
if (m_exit)
101+
return;
102+
87103
m_exit = true;
88104

105+
// Discard all remaining tasks
89106
while (!m_tasks.empty())
90107
{
91-
// Run each task but skip execution of the actual function (-> just delete the task)
108+
// Run each task with skip flag to clean up without executing
92109
auto task = std::move(m_tasks.front());
93-
task(true);
110+
m_tasks.pop(); // Important: Remove from queue to avoid infinite loop
111+
112+
// Execute cleanup outside the critical section to reduce lock contention
113+
lock.unlock();
114+
try
115+
{
116+
task(true); // Cleanup the shared_ptr
117+
}
118+
catch (...)
119+
{
120+
// Exceptions during cleanup indicate a serious bug (e.g., corrupted lambda)
121+
// We cannot propagate this exception as we're mid-shutdown with the lock released.
122+
// In debug builds, this should be logged/asserted.
123+
dassert(false && "Exception during thread pool task cleanup");
124+
}
125+
lock.lock();
94126
}
95127
}
96128

@@ -99,12 +131,24 @@ namespace SharedUtil
99131

100132
// Wait for threads to end
101133
for (std::thread& worker : m_vecThreads)
102-
worker.join();
134+
{
135+
if (worker.joinable())
136+
worker.join();
137+
}
103138
}
104139

105-
~CThreadPool()
140+
~CThreadPool() noexcept
106141
{
107-
shutdown();
142+
try
143+
{
144+
shutdown();
145+
}
146+
catch (...)
147+
{
148+
// Must suppress exceptions to prevent std::terminate().
149+
// This should only happen if mutex operations fail (system error).
150+
dassert(false && "Exception during thread pool destruction");
151+
}
108152
}
109153

110154
static CThreadPool& getDefaultThreadPool()

0 commit comments

Comments
 (0)