Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add timer state code+ut #399

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

add timer state code+ut #399

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
714e029
add code
M-iceberg Oct 25, 2024
ef31a77
Merge branch 'master' into timer_state_impl
M-iceberg Oct 25, 2024
d0d5bca
address comment
M-iceberg Oct 25, 2024
d68b9c8
address comment
M-iceberg Oct 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions win32/devdoc/threadpool_win32.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

@dcristoloveanu dcristoloveanu Oct 25, 2024 •
edited by M-iceberg
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DESTROYING

These states should be described at the top of the .md (Otherwise when reading this, I have no idea when we switch to the state and when we transition out of it) ... #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

**SRS_THREADPOOL_WIN32_07_001: [** If timer state is in destroying state, `threadpool_timer_restart` shall fail and return a non-zero value. **]**
Copy link
Member

@dcristoloveanu dcristoloveanu Oct 25, 2024 •
edited by M-iceberg
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

destroying

I think capital and enclosed by tickmarks was better. #Resolved

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because they are references to states that should be defined in the doc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


**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`. **]**

Expand Down Expand Up @@ -245,15 +245,15 @@ 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. **]**
Copy link
Member

@dcristoloveanu dcristoloveanu Oct 25, 2024 •
edited by M-iceberg
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[** `threadpool_t

I find these specs rather poor.
As a reader I can only try to imagine what we want to achieve.
I think we should describe the states we have and why we have them:

I assume we have something along the lines of:
CREATED - state in which the timer is after threadpool_timer_start was called.
DESTROYING - state in which the timer is after threadpool_timer_destroy was called. A threadpool_timer_restart call after switching to DESTROYING state is failed.
DESTROYED - terminal state for the timer. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


**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. **]**

**SRS_THREADPOOL_WIN32_42_013: [** `threadpool_timer_destroy` shall call `WaitForThreadpoolTimerCallbacks`. **]**

**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`. **]**

Expand Down
15 changes: 15 additions & 0 deletions win32/src/threadpool_win32.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -583,6 +584,7 @@ int threadpool_timer_start(THANDLE(THREADPOOL) threadpool, uint32_t start_delay_
}
else
{
(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;
Expand Down Expand Up @@ -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
Copy link
Member

@dcristoloveanu dcristoloveanu Oct 25, 2024 •
edited by M-iceberg
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is_destroying

Can we have this as a proper enum? With the 3 states we have #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

)
{
LogError("timer is in closing state");
Copy link
Member

@dcristoloveanu dcristoloveanu Oct 25, 2024 •
edited by M-iceberg
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

closing

I thought the states are CREATED/DESTROYING/DESTROYED.
A wild closing appears :-) #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

modified

result = MU_FAILURE;
Copy link
Contributor

@jasmineymlo jasmineymlo Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like we shouldn't return MU_FAILURE if it's due to closing...otherwise will fault...this is breaking change and behaviour is different than what we have previously.
need to confirm with Dan

}
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. ]*/
Expand Down Expand Up @@ -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);
Expand All @@ -670,5 +682,8 @@ void threadpool_timer_destroy(TIMER_INSTANCE_HANDLE timer)

/* 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);
}
}
28 changes: 28 additions & 0 deletions win32/tests/threadpool_win32_ut/threadpool_win32_ut.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -1110,6 +1112,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)
Expand All @@ -1128,6 +1151,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);

Expand Down Expand Up @@ -1201,6 +1225,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
Expand All @@ -1211,10 +1237,12 @@ 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(free(IGNORED_ARG));
STRICT_EXPECTED_CALL(interlocked_exchange(IGNORED_ARG, 0));

// act
threadpool_timer_destroy(timer_instance);
Expand Down
Loading