Skip to content

Commit 86ca7dc

Browse files
nmcclatcheyalxvasilev
authored andcommitted
Fix fatal bug: Threads must copy arguments. (#59)
Threads were taking arguments by reference, rather than creating copies. This has been altered to the correct behavior, which should fix several fatal (and subtle) errors. Adds a regression test that will automatically detect similar failures (with high probability). Recommend immediate update and recompilation of all projects using this library.
1 parent 55a5e49 commit 86ca7dc

File tree

2 files changed

+35
-4
lines changed

2 files changed

+35
-4
lines changed

mingw.thread.h

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -72,18 +72,22 @@ namespace detail
7272
template<class Func, typename... Args>
7373
class ThreadFuncCall
7474
{
75-
typedef std::tuple<Args...> Tuple;
76-
Func mFunc;
75+
using Tuple = std::tuple<typename std::decay<Args>::type...>;
76+
typename std::decay<Func>::type mFunc;
7777
Tuple mArgs;
7878

7979
template <std::size_t... S>
8080
void callFunc(detail::IntSeq<S...>)
8181
{
82-
detail::invoke(std::forward<Func>(mFunc), std::get<S>(std::forward<Tuple>(mArgs)) ...);
82+
// Note: Only called once (per thread)
83+
detail::invoke(std::move(mFunc), std::move(std::get<S>(mArgs)) ...);
8384
}
8485
public:
8586
ThreadFuncCall(Func&& aFunc, Args&&... aArgs)
86-
:mFunc(std::forward<Func>(aFunc)), mArgs(std::forward<Args>(aArgs)...){}
87+
: mFunc(std::forward<Func>(aFunc)),
88+
mArgs(std::forward<Args>(aArgs)...)
89+
{
90+
}
8791

8892
void callFunc()
8993
{

tests/tests.cpp

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -312,6 +312,33 @@ int main()
312312
std::hash<thread::id> hasher;
313313
std::cout << "Hash:\t" << hasher(this_thread::get_id()) << "\n";
314314
}
315+
316+
// Regression test: Thread must copy any argument that is passed by value.
317+
{
318+
std::vector<std::thread> loop_threads;
319+
std::atomic<int> i_vals_touched [4];// { 0, 0, 0, 0 };
320+
for (int i = 0; i < 4; ++i)
321+
i_vals_touched[i].store(0, std::memory_order_relaxed);
322+
for (int i = 0; i < 4; ++i)
323+
{
324+
loop_threads.push_back(std::thread([&](int c)
325+
{
326+
log("For-loop test thread got value: %i", c);
327+
i_vals_touched[c].fetch_add(1, std::memory_order_relaxed);
328+
}, i));
329+
}
330+
for (std::thread & thr : loop_threads)
331+
thr.join();
332+
for (int i = 0; i < 4; ++i)
333+
{
334+
if (i_vals_touched[i] != 1)
335+
{
336+
log("FATAL: Threads are not copying arguments!");
337+
return 1;
338+
}
339+
}
340+
}
341+
315342
std::thread t([](TestMove&& a, const char* b, int c) mutable
316343
{
317344
try

0 commit comments

Comments
 (0)