From 714e0295a2d5e9b18da09e247576954521a08dc6 Mon Sep 17 00:00:00 2001 From: Bingyi Yang Date: Thu, 24 Oct 2024 18:00:30 -0700 Subject: [PATCH 1/3] add code --- win32/devdoc/threadpool_win32.md | 6 ++--- win32/src/threadpool_win32.c | 14 ++++++++++ .../threadpool_win32_ut/threadpool_win32_ut.c | 26 +++++++++++++++++++ 3 files changed, 43 insertions(+), 3 deletions(-) diff --git a/win32/devdoc/threadpool_win32.md b/win32/devdoc/threadpool_win32.md index b0be6739..f6baca03 100644 --- a/win32/devdoc/threadpool_win32.md +++ b/win32/devdoc/threadpool_win32.md @@ -215,7 +215,7 @@ MOCKABLE_FUNCTION(, int, threadpool_timer_restart, TIMER_INSTANCE_HANDLE, timer, **SRS_THREADPOOL_WIN32_42_019: [** If `timer` is `NULL`, `threadpool_timer_restart` shall fail and return a non-zero value. **]** -If timer state is `DESTROYING`, `threadpool_timer_restart` shall fail and return a non-zero value. +**SRS_THREADPOOL_WIN32_07_001: [** If timer state is in destroying state, `threadpool_timer_restart` shall fail and return a non-zero value. **]** **SRS_THREADPOOL_WIN32_42_022: [** `threadpool_timer_restart` shall call `SetThreadpoolTimer`, passing negative `start_delay_ms` as `pftDueTime`, `timer_period_ms` as `msPeriod`, and 0 as `msWindowLength`. **]** @@ -245,7 +245,7 @@ MOCKABLE_FUNCTION(, void, threadpool_timer_destroy, TIMER_INSTANCE_HANDLE, timer **SRS_THREADPOOL_WIN32_42_011: [** If `timer` is `NULL`, `threadpool_timer_destroy` shall fail and return. **]** -`threadpool_timer_destroy` shall switch the state to `DESTROYING`. +**SRS_THREADPOOL_WIN32_07_002: [** `threadpool_timer_destroy` shall indicate the timer is in destroying state. **]** **SRS_THREADPOOL_WIN32_42_012: [** `threadpool_timer_destroy` shall call `SetThreadpoolTimer` with `NULL` for `pftDueTime` and 0 for `msPeriod` and `msWindowLength` to cancel ongoing timers. **]** @@ -253,7 +253,7 @@ MOCKABLE_FUNCTION(, void, threadpool_timer_destroy, TIMER_INSTANCE_HANDLE, timer **SRS_THREADPOOL_WIN32_42_014: [** `threadpool_timer_destroy` shall call `CloseThreadpoolTimer`. **]** -`threadpool_timer_destroy` shall switch the state to `DESTROYED`. +**SRS_THREADPOOL_WIN32_07_003: [** `threadpool_timer_destroy` shall indicate the timer is destroyed. **]** **SRS_THREADPOOL_WIN32_42_015: [** `threadpool_timer_destroy` shall free all resources in `timer`. **]** diff --git a/win32/src/threadpool_win32.c b/win32/src/threadpool_win32.c index 855a522e..54c9fc29 100644 --- a/win32/src/threadpool_win32.c +++ b/win32/src/threadpool_win32.c @@ -42,6 +42,7 @@ typedef struct TIMER_INSTANCE_TAG PTP_TIMER timer; THREADPOOL_WORK_FUNCTION work_function; void* work_function_context; + volatile_atomic int32_t is_destroying; } TIMER_INSTANCE; typedef struct THREADPOOL_TAG @@ -583,6 +584,7 @@ int threadpool_timer_start(THANDLE(THREADPOOL) threadpool, uint32_t start_delay_ } else { + timer_temp->is_destroying = 0; timer_temp->timer = tp_timer; timer_temp->work_function = work_function; timer_temp->work_function_context = work_function_context; @@ -625,6 +627,14 @@ int threadpool_timer_restart(TIMER_INSTANCE_HANDLE timer, uint32_t start_delay_m timer, start_delay_ms, timer_period_ms); result = MU_FAILURE; } + else if( + /* Codes_SRS_THREADPOOL_WIN32_07_001: [ If timer state is in destroying state, threadpool_timer_restart shall fail and return a non - zero value. ]*/ + interlocked_add(&timer->is_destroying, 0) == 1 + ) + { + LogError("timer is in closing state"); + result = MU_FAILURE; + } else { /* Codes_SRS_THREADPOOL_WIN32_42_022: [ threadpool_timer_restart shall call SetThreadpoolTimer, passing negative start_delay_ms as pftDueTime, timer_period_ms as msPeriod, and 0 as msWindowLength. ]*/ @@ -661,6 +671,8 @@ void threadpool_timer_destroy(TIMER_INSTANCE_HANDLE timer) } else { + /* Codes_SRS_THREADPOOL_WIN32_07_002: [ threadpool_timer_destroy shall indicate the timer is in destroying state. ]*/ + (void)interlocked_exchange(&timer->is_destroying, 1); /* Codes_SRS_THREADPOOL_WIN32_42_012: [ threadpool_timer_destroy shall call SetThreadpoolTimer with NULL for pftDueTime and 0 for msPeriod and msWindowLength to cancel ongoing timers. ]*/ /* Codes_SRS_THREADPOOL_WIN32_42_013: [ threadpool_timer_destroy shall call WaitForThreadpoolTimerCallbacks. ]*/ threadpool_internal_cancel_timer_and_wait(timer->timer); @@ -668,6 +680,8 @@ void threadpool_timer_destroy(TIMER_INSTANCE_HANDLE timer) /* Codes_SRS_THREADPOOL_WIN32_42_014: [ threadpool_timer_destroy shall call CloseThreadpoolTimer. ]*/ CloseThreadpoolTimer(timer->timer); + /* Codes_SRS_THREADPOOL_WIN32_07_003: [ threadpool_timer_destroy shall indicate the timer is destroyed. ]*/ + (void)interlocked_exchange(&timer->is_destroying, 0); /* Codes_SRS_THREADPOOL_WIN32_42_015: [ threadpool_timer_destroy shall free all resources in timer. ]*/ free(timer); } diff --git a/win32/tests/threadpool_win32_ut/threadpool_win32_ut.c b/win32/tests/threadpool_win32_ut/threadpool_win32_ut.c index 0e4cf4e8..f6e06ecb 100644 --- a/win32/tests/threadpool_win32_ut/threadpool_win32_ut.c +++ b/win32/tests/threadpool_win32_ut/threadpool_win32_ut.c @@ -1110,6 +1110,27 @@ TEST_FUNCTION(threadpool_timer_restart_with_NULL_timer_fails) ASSERT_ARE_NOT_EQUAL(int, 0, result); } +/* Tests_SRS_THREADPOOL_WIN32_07_001: [ If timer state is in destroying state, threadpool_timer_restart shall fail and return a non - zero value. ]*/ +TEST_FUNCTION(threadpool_timer_restart_with_timer_is_closing_fails) +{ + // arrange + THANDLE(THREADPOOL) threadpool = NULL; + PTP_TIMER_CALLBACK test_timer_callback; + PVOID test_timer_callback_context; + PTP_TIMER ptp_timer; + TIMER_INSTANCE_HANDLE timer_instance; + test_create_threadpool_and_start_timer(42, 2000, (void*)0x4243, &threadpool, &ptp_timer, &test_timer_callback, &test_timer_callback_context, &timer_instance); + + STRICT_EXPECTED_CALL(interlocked_add(IGNORED_ARG, 0)).SetReturn(1); + + // act + int result = threadpool_timer_restart(timer_instance, 43, 1000); + + // assert + ASSERT_ARE_EQUAL(char_ptr, umock_c_get_expected_calls(), umock_c_get_actual_calls()); + ASSERT_ARE_NOT_EQUAL(int, 0, result); +} + /* Tests_SRS_THREADPOOL_WIN32_42_022: [ threadpool_timer_restart shall call SetThreadpoolTimer, passing negative start_delay_ms as pftDueTime, timer_period_ms as msPeriod, and 0 as msWindowLength. ]*/ /* Tests_SRS_THREADPOOL_WIN32_42_023: [ threadpool_timer_restart shall succeed and return 0. ]*/ TEST_FUNCTION(threadpool_timer_restart_succeeds) @@ -1128,6 +1149,7 @@ TEST_FUNCTION(threadpool_timer_restart_succeeds) filetime_expected.dwHighDateTime = ularge_due_time.HighPart; filetime_expected.dwLowDateTime = ularge_due_time.LowPart; + STRICT_EXPECTED_CALL(interlocked_add(IGNORED_ARG, 0)); STRICT_EXPECTED_CALL(mocked_SetThreadpoolTimer(IGNORED_ARG, &filetime_expected, 1000, 0)) .ValidateArgumentValue_pti(&ptp_timer); @@ -1201,6 +1223,8 @@ TEST_FUNCTION(threadpool_timer_destroy_with_NULL_timer_fails) /* Tests_SRS_THREADPOOL_WIN32_42_013: [ threadpool_timer_destroy shall call WaitForThreadpoolTimerCallbacks. ]*/ /* Tests_SRS_THREADPOOL_WIN32_42_014: [ threadpool_timer_destroy shall call CloseThreadpoolTimer. ]*/ /* Tests_SRS_THREADPOOL_WIN32_42_015: [ threadpool_timer_destroy shall free all resources in timer. ]*/ +/* Tests_SRS_THREADPOOL_WIN32_07_002: [ threadpool_timer_destroy shall indicate the timer is in destroying state. ]*/ +/* Tests_SRS_THREADPOOL_WIN32_07_003: [ threadpool_timer_destroy shall indicate the timer is destroyed. ]*/ TEST_FUNCTION(threadpool_timer_destroy_succeeds) { // arrange @@ -1211,9 +1235,11 @@ TEST_FUNCTION(threadpool_timer_destroy_succeeds) TIMER_INSTANCE_HANDLE timer_instance; test_create_threadpool_and_start_timer(42, 2000, (void*)0x4243, &threadpool, &ptp_timer, &test_timer_callback, &test_timer_callback_context, &timer_instance); + STRICT_EXPECTED_CALL(interlocked_exchange(IGNORED_ARG, 1)); STRICT_EXPECTED_CALL(mocked_SetThreadpoolTimer(ptp_timer, NULL, 0, 0)); STRICT_EXPECTED_CALL(mocked_WaitForThreadpoolTimerCallbacks(ptp_timer, TRUE)); STRICT_EXPECTED_CALL(mocked_CloseThreadpoolTimer(ptp_timer)); + STRICT_EXPECTED_CALL(interlocked_exchange(IGNORED_ARG, 0)); STRICT_EXPECTED_CALL(free(IGNORED_ARG)); // act From d0d5bca25ef1a50ae9c28856a9d8579f28b280ea Mon Sep 17 00:00:00 2001 From: Bingyi Yang Date: Thu, 24 Oct 2024 18:18:29 -0700 Subject: [PATCH 2/3] address comment --- win32/src/threadpool_win32.c | 2 +- win32/tests/threadpool_win32_ut/threadpool_win32_ut.c | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/win32/src/threadpool_win32.c b/win32/src/threadpool_win32.c index 54c9fc29..6cef6249 100644 --- a/win32/src/threadpool_win32.c +++ b/win32/src/threadpool_win32.c @@ -584,7 +584,7 @@ int threadpool_timer_start(THANDLE(THREADPOOL) threadpool, uint32_t start_delay_ } else { - timer_temp->is_destroying = 0; + (void)interlocked_exchange(&timer_temp->is_destroying, 0); timer_temp->timer = tp_timer; timer_temp->work_function = work_function; timer_temp->work_function_context = work_function_context; diff --git a/win32/tests/threadpool_win32_ut/threadpool_win32_ut.c b/win32/tests/threadpool_win32_ut/threadpool_win32_ut.c index f6e06ecb..6cd24737 100644 --- a/win32/tests/threadpool_win32_ut/threadpool_win32_ut.c +++ b/win32/tests/threadpool_win32_ut/threadpool_win32_ut.c @@ -990,6 +990,7 @@ TEST_FUNCTION(threadpool_timer_start_succeeds) .CaptureArgumentValue_pfnti(&test_timer_callback) .CaptureArgumentValue_pv(&test_timer_callback_context) .CaptureReturn(&ptp_timer); + STRICT_EXPECTED_CALL(interlocked_exchange(IGNORED_ARG, 0)); STRICT_EXPECTED_CALL(mocked_SetThreadpoolTimer(IGNORED_ARG, &filetime_expected, 2000, 0)) .ValidateArgumentValue_pti(&ptp_timer); @@ -1030,6 +1031,7 @@ TEST_FUNCTION(threadpool_timer_start_with_NULL_work_function_context_succeeds) .CaptureArgumentValue_pfnti(&test_timer_callback) .CaptureArgumentValue_pv(&test_timer_callback_context) .CaptureReturn(&ptp_timer); + STRICT_EXPECTED_CALL(interlocked_exchange(IGNORED_ARG, 0)); STRICT_EXPECTED_CALL(mocked_SetThreadpoolTimer(IGNORED_ARG, &filetime_expected, 2000, 0)) .ValidateArgumentValue_pti(&ptp_timer); From d68b9c854a160bffe858a25cbbdb0b931331685c Mon Sep 17 00:00:00 2001 From: Bingyi Yang Date: Thu, 24 Oct 2024 18:25:01 -0700 Subject: [PATCH 3/3] address comment --- win32/src/threadpool_win32.c | 5 +++-- win32/tests/threadpool_win32_ut/threadpool_win32_ut.c | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/win32/src/threadpool_win32.c b/win32/src/threadpool_win32.c index 6cef6249..cc1c28b8 100644 --- a/win32/src/threadpool_win32.c +++ b/win32/src/threadpool_win32.c @@ -680,9 +680,10 @@ void threadpool_timer_destroy(TIMER_INSTANCE_HANDLE timer) /* Codes_SRS_THREADPOOL_WIN32_42_014: [ threadpool_timer_destroy shall call CloseThreadpoolTimer. ]*/ CloseThreadpoolTimer(timer->timer); - /* Codes_SRS_THREADPOOL_WIN32_07_003: [ threadpool_timer_destroy shall indicate the timer is destroyed. ]*/ - (void)interlocked_exchange(&timer->is_destroying, 0); /* Codes_SRS_THREADPOOL_WIN32_42_015: [ threadpool_timer_destroy shall free all resources in timer. ]*/ free(timer); + + /* Codes_SRS_THREADPOOL_WIN32_07_003: [ threadpool_timer_destroy shall indicate the timer is destroyed. ]*/ + (void)interlocked_exchange(&timer->is_destroying, 0); } } diff --git a/win32/tests/threadpool_win32_ut/threadpool_win32_ut.c b/win32/tests/threadpool_win32_ut/threadpool_win32_ut.c index 6cd24737..a98540b6 100644 --- a/win32/tests/threadpool_win32_ut/threadpool_win32_ut.c +++ b/win32/tests/threadpool_win32_ut/threadpool_win32_ut.c @@ -1241,8 +1241,8 @@ TEST_FUNCTION(threadpool_timer_destroy_succeeds) STRICT_EXPECTED_CALL(mocked_SetThreadpoolTimer(ptp_timer, NULL, 0, 0)); STRICT_EXPECTED_CALL(mocked_WaitForThreadpoolTimerCallbacks(ptp_timer, TRUE)); STRICT_EXPECTED_CALL(mocked_CloseThreadpoolTimer(ptp_timer)); - STRICT_EXPECTED_CALL(interlocked_exchange(IGNORED_ARG, 0)); STRICT_EXPECTED_CALL(free(IGNORED_ARG)); + STRICT_EXPECTED_CALL(interlocked_exchange(IGNORED_ARG, 0)); // act threadpool_timer_destroy(timer_instance);