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

Use ctrl-c to stop processes in Windows #278

Merged
merged 6 commits into from
Jan 31, 2020
Merged

Use ctrl-c to stop processes in Windows #278

merged 6 commits into from
Jan 31, 2020

Conversation

piraka9011
Copy link
Contributor

Send a ctrl-c event to a process with GenerateConsoleCtrlEvent.

Used in gracefully terminating processes in E2E tests.

Signed-off-by: Anas Abou Allaban allabana@amazon.com

Use ctrl-c to stop process in windows
b735ba9 
Signed-off-by: Anas Abou Allaban <allabana@amazon.com>
@piraka9011
Copy link
Contributor Author

Windows Build Status

@zmichaels11
Copy link
Contributor

zmichaels11 commented Jan 31, 2020 •
edited
Loading

@ros2/aws-oncall - please run this CI job
Gist: https://gist.githubusercontent.com/zmichaels11/4ece7929c2e0c955278ff60405a2686a/raw/7c53ee8736e870ce596e97b776a9840750099ad7/ros2.repos
BUILD args: --packages-up-to rosbag2_tests rosbag2_test_common
TEST args: --packages-select rosbag2_tests rosbag2_test_common
Job: ci_launcher

@piraka9011
Copy link
Contributor Author

@zmichaels11 I ran this with rosbag2_test_common as well

@zmichaels11
Copy link
Contributor

zmichaels11 commented Jan 31, 2020 •
edited
Loading

I'm going to try and remove the windows-specific code and see if it still passes...
Edit:

  • Windows Build Status

@zmichaels11
Assert that bagfile split at least once on Windows
3bf6223 
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11
Copy link
Contributor

It looks like the dtor is still not guaranteed, so I modified the flaky test on windows to pass if the bagfile split. This is within the acceptable criteria for splitting due to there only being a minimum guarantee on bagfile sizes.

  • Windows Build Status

@zmichaels11
Copy link
Contributor

zmichaels11 commented Jan 31, 2020 •
edited
Loading

Build passed on windows on first try.
Just for completeness:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

piraka9011
piraka9011 commented Jan 31, 2020
View reviewed changes
rosbag2_tests/test/rosbag2_tests/test_rosbag2_record_end_to_end.cpp
@@ -333,8 +333,15 @@ TEST_F(RecordFixture, record_end_to_end_with_splitting_splits_bagfile) {

const auto bag_path = rcpputils::fs::path(root_bag_path_) / bag_name.str();

metadata.relative_file_paths.push_back(bag_path.string());
// There is not any guarantee that the bagfile split expected_split times
// due to possible io sync delays. Instead, assert that the bagfile split
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch on the io sync stuff tho.

@zmichaels11
Fix grammar
216f8e1 
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11
Delete leftover bagfiles if they exist on test SetUp
94d549c 
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11
Copy link
Contributor

zmichaels11 commented Jan 31, 2020 •
edited
Loading

Trying to verify stability;

  • Windows Build Status --retest-until-fail 10

@zmichaels11
Check if db exists before trying to open it
5c066b1 
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11
Copy link
Contributor

zmichaels11 commented Jan 31, 2020 •
edited
Loading

Test still failed after clearing on SetUp, so the bagfile is somehow created after the test starts but before ros2bag begins recording. I think this might be the waitfordb function, which tries to open the db. SqliteWrapper does not check if the db exists before opening it.

Edit:
If this is the cause of the rosbag2_tests regression, then the root cause was a change in the IO speed. I looked through past nightly builds and these tests occasionally failed but were hidden by rerun-tests-until-pass 10.

@zmichaels11
Copy link
Contributor

zmichaels11 commented Jan 31, 2020 •
edited
Loading

I'm pretty sure that was it since the last pass failed elsewhere.
This time it hung at the Each(Pointee(Field section, which I flagged as a memory issue in a different PR.
Applying that change here:

  • Windows Build Status

@zmichaels11
Copy link
Contributor

zmichaels11 commented Jan 31, 2020 •
edited
Loading

Ok it looks like the root cause was both due to ros2 bag command running faster and the memory access issue.
Rerunning CI with retest-until-fail 25

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

zmichaels11
zmichaels11 reviewed Jan 31, 2020
View reviewed changes
rosbag2_test_common/include/rosbag2_test_common/process_execution_helpers_windows.hpp
EXPECT_THAT(exit_code, Eq(259));

ASSERT_THAT(exit_code, Eq(259));
GenerateConsoleCtrlEvent(CTRL_C_EVENT, handle.process_info.dwThreadId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: This actually didn't send SIGINT on windows...

zmichaels11
zmichaels11 reviewed Jan 31, 2020
View reviewed changes
rosbag2_tests/test/rosbag2_tests/record_fixture.hpp
rosbag2_storage_plugins::SqliteWrapper
db(database_path_, rosbag2_storage::storage_interfaces::IOFlag::READ_ONLY);
return;
if (rcpputils::fs::exists(rcpputils::fs::path{database_path_})) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This line was the cause of flakiness;
SqliteWrapper does not check if a file exists before opening the Db. The result is that the db is implicitly created if it does not exist.

This was referenced Jan 31, 2020
Closed
Closed
emersonknapp
emersonknapp approved these changes Jan 31, 2020
View reviewed changes
Copy link
Collaborator

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for digging into this. make sure to note the file creation stuff in the merge commit

@zmichaels11
Replace Each(Pointee(Field with for-each loop
99bac85 
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@piraka9011 piraka9011 merged commit 1c63b6e into ros2:master Jan 31, 2020
@thomas-moulard thomas-moulard deleted the windows-tests branch February 4, 2020 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zmichaels11 zmichaels11 zmichaels11 approved these changes

@emersonknapp emersonknapp emersonknapp approved these changes

@Karsten1987 Karsten1987 Awaiting requested review from Karsten1987

@thomas-moulard thomas-moulard Awaiting requested review from thomas-moulard

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@piraka9011 @zmichaels11 @emersonknapp