Add test for possible race in guard condition notification (2)#281
Add test for possible race in guard condition notification (2)#281jmachowinski wants to merge 3 commits into
Conversation
Signed-off-by: Miguel Company <miguelcompany@eprosima.com>
…times Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
|
Pulls: #281, ros2/rmw_fastrtps#895
|
|
Tick the box to add this pull request to the merge queue (same as
|
|
@jmachowinski this closes #280? |
|
No, this is just a tmp thing that allows me to use CI without manually setting up a GIST etc. Might be better way to do this.... |
|
looks like CI warnings are unrelated... |
wjwwood
left a comment
wjwwood
left a comment
There was a problem hiding this comment.
a few small comments, but lgtm
| std::array<guard_condition_ptr_t, number_of_guard_conditions> guard_condition_ptrs; | ||
|
|
||
| // Create the guard conditions | ||
| std::vector<bool> guard_conditions_triggered(number_of_guard_conditions, false); |
There was a problem hiding this comment.
Anytime I see vector<bool>: https://isocpp.org/blog/2012/11/on-vectorbool
nitpick: could this be array<bool, number_of_guard_conditions>?
| // Clean up: destroy guard conditions and wait set | ||
| for (size_t i = 0; i < number_of_guard_conditions; ++i) { | ||
| rmw_ret_t ret = rmw_destroy_guard_condition(guard_condition_ptrs[i]); | ||
| ASSERT_EQ(ret, RMW_RET_OK) << rcutils_get_error_string().str; | ||
| } | ||
|
|
||
| { | ||
| rmw_ret_t ret = rmw_destroy_wait_set(wait_set); | ||
| ASSERT_EQ(ret, RMW_RET_OK) << rcutils_get_error_string().str; | ||
| } |
There was a problem hiding this comment.
Careful with these. If something throws in this function, these will never get cleaned up. A scopedexit-like object works well in these situations.
If any of the ASSERT_* statements fail, then it will cause problems here. Also for the thread that may never be joined in that case.
| guard_conditions_triggered = std::vector<bool>(number_of_guard_conditions, false); | ||
|
|
||
| // Prepare input arguments for rmw_wait | ||
| rmw_time_t timeout_argument = {0, 100000000}; // 100ms |
There was a problem hiding this comment.
You are waiting 100ms for rmw_wait and having the trigger thread sleep for 10ms. In heavily loaded CI environments (like Windows), a 10ms sleep can easily stretch well beyond 100ms. If the thread wakes up late, the test will falsely fail.
I would suggest you increase the timeout significantly (e.g., 1 or 2 seconds) to prevent flaky CI failures. The test will still run quickly because rmw_wait returns immediately once the conditions are triggered and the timeout only acts as an upper bound.
4 participants
Description
Same as #280
I can't push to the source repos and therefore opened this PR to iterate faster on the windows CI failure.