Skip to content

Commit 6ee3331

Browse files
author
Michael Grier
committed
threadpool tests
1 parent 7b3e344 commit 6ee3331

4 files changed

Lines changed: 495 additions & 6 deletions

File tree

‎src/libraries/threadpool/src/Platforms/windows/periodic_timer.cpp‎

Lines changed: 28 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,12 @@ namespace m::threadpool_impl
5454

5555
m_duration = dur;
5656
m_packaged_task.reset();
57+
58+
// Increment m_set_count so that do_stop()'s guard can distinguish
59+
// "never started" / "already stopped" (m_set_count == m_set_count_when_finalized)
60+
// from "currently running" (m_set_count != m_set_count_when_finalized).
61+
m_set_count++;
62+
5763
m_timer.set(
5864
parameters.m_p_ft_due_time, parameters.m_ms_period, parameters.m_ms_window_length);
5965
}
@@ -69,7 +75,10 @@ namespace m::threadpool_impl
6975
{
7076
auto l = std::unique_lock(m_mutex);
7177

72-
// You shouldn't stop a periodic task you haven't started.
78+
// You shouldn't stop a periodic task you haven't started or already stopped.
79+
// m_set_count is incremented each time do_set() is called.
80+
// m_set_count_when_finalized is updated here to the current m_set_count after
81+
// a successful stop, preventing double-stop.
7382
if ((m_set_count == m_set_count_when_finalized) ||
7483
(m_set_count == m_set_count_when_cancelled))
7584
{
@@ -79,6 +88,10 @@ namespace m::threadpool_impl
7988
}
8089

8190
m_timer.cancel();
91+
92+
// Mark this generation as finalized so that a subsequent call to stop()
93+
// correctly detects "already stopped" via the guard above.
94+
m_set_count_when_finalized = m_set_count;
8295
}
8396

8497
void
@@ -142,16 +155,25 @@ namespace m::threadpool_impl
142155
void
143156
periodic_timer::on_tp_timer(PTP_CALLBACK_INSTANCE) noexcept
144157
{
145-
auto l = std::unique_lock(m_mutex);
146-
147-
m_set_count_when_executed = m_set_count;
158+
// Record that this generation of the timer has fired, then release the
159+
// lock *before* invoking the user callback. Holding m_mutex across the
160+
// callback would deadlock if the callback calls stop() (or any other
161+
// member that acquires m_mutex).
162+
{
163+
auto l = std::unique_lock(m_mutex);
164+
m_set_count_when_executed = m_set_count;
165+
}
148166

149167
{
168+
// m_description is immutable after construction – safe to read without lock.
150169
m::thread_description td(m_description);
151170
m_packaged_task();
152171
}
153172

154-
m_packaged_task.reset();
155-
m_re_execution_count++;
173+
{
174+
auto l = std::unique_lock(m_mutex);
175+
m_packaged_task.reset();
176+
m_re_execution_count++;
177+
}
156178
}
157179
} // namespace m::threadpool_impl

‎src/libraries/threadpool/test/test_periodic_timer.cpp‎

Lines changed: 145 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
#include <latch>
1010
#include <span>
1111
#include <string_view>
12+
#include <thread>
1213

1314
#include <m/debugging/dbg_format.h>
1415
#include <m/threadpool/threadpool.h>
@@ -34,3 +35,147 @@ TEST(PeriodicTimer, Test1)
3435
EXPECT_GT(counter, 17);
3536
EXPECT_LT(counter, 22);
3637
}
38+
39+
// ---------------------------------------------------------------------------
40+
// is_set() tests
41+
// ---------------------------------------------------------------------------
42+
43+
TEST(PeriodicTimer, IsSetInitiallyFalse)
44+
{
45+
auto t1 = m::threadpool->create_periodic_timer([]() {});
46+
EXPECT_FALSE(t1->is_set());
47+
}
48+
49+
TEST(PeriodicTimer, IsSetTrueAfterSet)
50+
{
51+
std::latch fired(1);
52+
53+
auto t1 = m::threadpool->create_periodic_timer([&]() {
54+
fired.count_down(); // signal first firing
55+
});
56+
57+
EXPECT_FALSE(t1->is_set());
58+
t1->set(100ms);
59+
EXPECT_TRUE(t1->is_set());
60+
61+
// Wait for at least one callback so we know the timer is operating.
62+
fired.wait();
63+
64+
// Still set (periodic timers keep firing until stopped).
65+
EXPECT_TRUE(t1->is_set());
66+
67+
t1->stop();
68+
t1->wait();
69+
}
70+
71+
// ---------------------------------------------------------------------------
72+
// stop() regression tests (bug: m_set_count was never incremented)
73+
// ---------------------------------------------------------------------------
74+
75+
TEST(PeriodicTimer, StopAfterSetSucceeds)
76+
{
77+
// Regression: before the fix, do_stop() always threw because m_set_count
78+
// was never incremented in do_set(), making the "not started" guard always
79+
// true even on a running timer.
80+
auto t1 = m::threadpool->create_periodic_timer([]() {});
81+
t1->set(100ms);
82+
EXPECT_NO_THROW(t1->stop());
83+
t1->wait();
84+
}
85+
86+
TEST(PeriodicTimer, IsSetFalseAfterStop)
87+
{
88+
auto t1 = m::threadpool->create_periodic_timer([]() {});
89+
t1->set(100ms);
90+
t1->stop();
91+
EXPECT_FALSE(t1->is_set());
92+
t1->wait();
93+
}
94+
95+
TEST(PeriodicTimer, WaitCompletesAfterStop)
96+
{
97+
auto t1 = m::threadpool->create_periodic_timer([]() {});
98+
t1->set(100ms);
99+
t1->stop();
100+
t1->wait(); // must not hang
101+
}
102+
103+
TEST(PeriodicTimer, DoubleStopThrows)
104+
{
105+
// Stopping an already-stopped timer must throw.
106+
auto t1 = m::threadpool->create_periodic_timer([]() {});
107+
t1->set(100ms);
108+
t1->stop();
109+
t1->wait();
110+
EXPECT_ANY_THROW(t1->stop());
111+
}
112+
113+
TEST(PeriodicTimer, StopPreventsFurtherFiring)
114+
{
115+
std::atomic<uintmax_t> counter{};
116+
117+
auto t1 = m::threadpool->create_periodic_timer([&]() { counter.fetch_add(1); });
118+
119+
t1->set(50ms);
120+
std::this_thread::sleep_for(250ms); // let it fire a few times
121+
122+
t1->stop();
123+
t1->wait();
124+
125+
auto const count_at_stop = counter.load();
126+
EXPECT_GT(count_at_stop, 0u);
127+
128+
// After stop + wait no further callbacks should arrive.
129+
std::this_thread::sleep_for(200ms);
130+
EXPECT_EQ(counter.load(), count_at_stop);
131+
}
132+
133+
// ---------------------------------------------------------------------------
134+
// Deadlock regression: callback must not hold internal mutex when invoked
135+
// ---------------------------------------------------------------------------
136+
137+
TEST(PeriodicTimer, CallbackCanCallIsSetWithoutDeadlock)
138+
{
139+
// Before the fix, on_tp_timer called user code while holding m_mutex.
140+
// do_is_set() on Windows delegates to ::IsThreadpoolTimerSet() (no m_mutex),
141+
// but the test demonstrates that is_set() is callable from the callback.
142+
std::latch checked(1);
143+
m::periodic_timer* timer_ptr = nullptr;
144+
145+
auto t1 = m::threadpool->create_periodic_timer([&]() {
146+
if (timer_ptr != nullptr)
147+
{
148+
(void)timer_ptr->is_set();
149+
checked.count_down();
150+
}
151+
});
152+
153+
timer_ptr = t1.get();
154+
t1->set(50ms);
155+
checked.wait(); // block until callback has run and checked is_set()
156+
157+
t1->stop();
158+
t1->wait();
159+
}
160+
161+
TEST(PeriodicTimer, StopCanBeCalledFromCallbackWithoutDeadlock)
162+
{
163+
// Regression: on_tp_timer held m_mutex across the user callback.
164+
// stop() also takes m_mutex, so calling stop() from the callback would
165+
// deadlock. After the fix the callback is invoked with the lock released.
166+
std::latch stopped(1);
167+
std::unique_ptr<m::periodic_timer> t1;
168+
169+
t1 = m::threadpool->create_periodic_timer([&]() {
170+
// Stop from within the callback – must not deadlock.
171+
if (stopped.try_wait())
172+
return; // already stopped on a previous re-entry
173+
174+
t1->stop();
175+
stopped.count_down();
176+
});
177+
178+
t1->set(50ms);
179+
stopped.wait(); // block until callback has called stop()
180+
t1->wait();
181+
}

‎src/libraries/threadpool/test/test_timer.cpp‎

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
#include <latch>
1010
#include <span>
1111
#include <string_view>
12+
#include <thread>
1213

1314
#include <m/debugging/dbg_format.h>
1415
#include <m/threadpool/threadpool.h>
@@ -321,3 +322,86 @@ TEST(Timer, EnsureDestructionDelayed)
321322
t1.reset();
322323
EXPECT_TRUE(ran);
323324
}
325+
326+
// ---------------------------------------------------------------------------
327+
// is_set() tests
328+
// ---------------------------------------------------------------------------
329+
330+
TEST(Timer, IsSetInitiallyFalse)
331+
{
332+
auto t1 = m::threadpool->create_timer([]() {});
333+
EXPECT_FALSE(t1->is_set());
334+
}
335+
336+
TEST(Timer, IsSetTrueAfterSetWithLongDuration)
337+
{
338+
// Set a 60-second timer; is_set() must return true before it fires.
339+
auto t1 = m::threadpool->create_timer([]() {});
340+
t1->set(60s);
341+
EXPECT_TRUE(t1->is_set());
342+
t1->cancel();
343+
t1->wait();
344+
}
345+
346+
TEST(Timer, IsSetFalseAfterCancel)
347+
{
348+
// On Windows, ::IsThreadpoolTimerSet reflects whether SetThreadpoolTimer
349+
// was called with a non-NULL due time and not yet cancelled. Even after
350+
// a one-shot timer fires naturally it keeps returning true. Only an
351+
// explicit cancel() (which calls SetThreadpoolTimer(NULL)) clears it.
352+
auto t1 = m::threadpool->create_timer([]() {});
353+
t1->set(60s);
354+
EXPECT_TRUE(t1->is_set());
355+
t1->cancel();
356+
t1->wait();
357+
EXPECT_FALSE(t1->is_set());
358+
}
359+
360+
// ---------------------------------------------------------------------------
361+
// cancel() tests
362+
// ---------------------------------------------------------------------------
363+
364+
TEST(Timer, CancelPreventsCallback)
365+
{
366+
std::atomic<bool> ran{false};
367+
368+
auto t1 = m::threadpool->create_timer([&]() {
369+
ran.store(true, std::memory_order_release);
370+
});
371+
372+
// Set a long-duration timer then cancel it before it can fire.
373+
t1->set(30s);
374+
t1->cancel();
375+
// wait() drains any in-flight dispatch before we check the flag.
376+
t1->wait();
377+
378+
EXPECT_FALSE(ran.load(std::memory_order_acquire));
379+
}
380+
381+
// ---------------------------------------------------------------------------
382+
// wait() tests
383+
// ---------------------------------------------------------------------------
384+
385+
TEST(Timer, WaitAfterCancelDrainsRunningCallback)
386+
{
387+
// cancel() followed by wait() must block until any currently-executing
388+
// callback completes. timer::wait() without cancel() only waits for
389+
// actively-running callbacks; this test uses a latch so we know the
390+
// callback has started before we call cancel() + wait().
391+
std::latch started(2);
392+
std::atomic<bool> ran{false};
393+
394+
auto t1 = m::threadpool->create_timer([&]() {
395+
started.arrive_and_wait(); // signal: callback has begun
396+
std::this_thread::sleep_for(50ms); // stay running for a moment
397+
ran.store(true, std::memory_order_release);
398+
});
399+
400+
t1->set(0s);
401+
started.arrive_and_wait(); // wait until callback is executing
402+
403+
t1->cancel();
404+
t1->wait(); // must not return until the running callback completes
405+
406+
EXPECT_TRUE(ran.load(std::memory_order_acquire));
407+
}

0 commit comments

Comments
 (0)