Skip to content

Fix leaks in rcl_action unit tests#442

Merged
jacobperron merged 1 commit intoros2:masterfrom
aws-ros-dev:fix-rcl-action-leaks
May 20, 2019
Merged

Fix leaks in rcl_action unit tests#442
jacobperron merged 1 commit intoros2:masterfrom
aws-ros-dev:fix-rcl-action-leaks

Conversation

@prajakta-gokhale
Copy link
Copy Markdown

@prajakta-gokhale prajakta-gokhale commented May 18, 2019

Fix memory leaks detected by AddressSanitizer from rcl_action unit tests.

Before the fix:

Running main() from /home/ANT.AMAZON.COM/prajaktg/ros2_ws/install-asan/gtest_vendor/src/gtest_vendor/src/gtest_main.cc
[==========] Running 4 tests from 2 test cases.
[----------] Global test environment set-up.
[----------] 1 test from TestActionClientBaseFixture
[ RUN      ] TestActionClientBaseFixture.test_action_client_init_fini
[       OK ] TestActionClientBaseFixture.test_action_client_init_fini (17 ms)
[----------] 1 test from TestActionClientBaseFixture (17 ms total)

[----------] 3 tests from TestActionClientFixture
[ RUN      ] TestActionClientFixture.test_action_client_is_valid
[       OK ] TestActionClientFixture.test_action_client_is_valid (13 ms)
[ RUN      ] TestActionClientFixture.test_action_client_get_action_name
[       OK ] TestActionClientFixture.test_action_client_get_action_name (10 ms)
[ RUN      ] TestActionClientFixture.test_action_client_get_options
[       OK ] TestActionClientFixture.test_action_client_get_options (10 ms)
[----------] 3 tests from TestActionClientFixture (33 ms total)

[----------] Global test environment tear-down
[==========] 4 tests from 2 test cases ran. (50 ms total)
[  PASSED  ] 4 tests.

=================================================================
==12786==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 312 byte(s) in 3 object(s) allocated from:
    ...snip...

Indirect leak of 312 byte(s) in 3 object(s) allocated from:
    ...snip...

SUMMARY: AddressSanitizer: 1184 byte(s) leaked in 12 allocation(s).

After the fix:

Running main() from /home/ANT.AMAZON.COM/prajaktg/ros2_ws/install-asan/gtest_vendor/src/gtest_vendor/src/gtest_main.cc
[==========] Running 4 tests from 2 test cases.
[----------] Global test environment set-up.
[----------] 1 test from TestActionClientBaseFixture
[ RUN      ] TestActionClientBaseFixture.test_action_client_init_fini
[       OK ] TestActionClientBaseFixture.test_action_client_init_fini (18 ms)
[----------] 1 test from TestActionClientBaseFixture (18 ms total)

[----------] 3 tests from TestActionClientFixture
[ RUN      ] TestActionClientFixture.test_action_client_is_valid
[       OK ] TestActionClientFixture.test_action_client_is_valid (11 ms)
[ RUN      ] TestActionClientFixture.test_action_client_get_action_name
[       OK ] TestActionClientFixture.test_action_client_get_action_name (12 ms)
[ RUN      ] TestActionClientFixture.test_action_client_get_options
[       OK ] TestActionClientFixture.test_action_client_get_options (10 ms)
[----------] 3 tests from TestActionClientFixture (34 ms total)

[----------] Global test environment tear-down
[==========] 4 tests from 2 test cases ran. (52 ms total)
[  PASSED  ] 4 tests.

All tests pass after the fixes:

Running tests...
Test project /home/ANT.AMAZON.COM/prajaktg/ros2_ws/build-asan/rcl_action
      Start  1: test_action_client
 1/15 Test  #1: test_action_client ............................   Passed    0.18 sec
      Start  2: test_action_communication__rmw_fastrtps_cpp
 2/15 Test  #2: test_action_communication__rmw_fastrtps_cpp ...   Passed    0.41 sec
      Start  3: test_action_interaction__rmw_fastrtps_cpp
 3/15 Test  #3: test_action_interaction__rmw_fastrtps_cpp .....   Passed    0.18 sec
      Start  4: test_graph__rmw_fastrtps_cpp
 4/15 Test  #4: test_graph__rmw_fastrtps_cpp ..................   Passed    3.31 sec
      Start  5: test_action_server
 5/15 Test  #5: test_action_server ............................   Passed   10.33 sec
      Start  6: test_goal_handle
 6/15 Test  #6: test_goal_handle ..............................   Passed    0.13 sec
      Start  7: test_goal_state_machine
 7/15 Test  #7: test_goal_state_machine .......................   Passed    0.13 sec
      Start  8: test_types
 8/15 Test  #8: test_types ....................................   Passed    0.13 sec
      Start  9: test_names
 9/15 Test  #9: test_names ....................................   Passed    0.12 sec
      Start 10: copyright
10/15 Test #10: copyright .....................................   Passed    0.30 sec
      Start 11: cppcheck
11/15 Test #11: cppcheck ......................................   Passed    0.66 sec
      Start 12: cpplint
12/15 Test #12: cpplint .......................................   Passed    1.76 sec
      Start 13: lint_cmake
13/15 Test #13: lint_cmake ....................................   Passed    0.29 sec
      Start 14: uncrustify
14/15 Test #14: uncrustify ....................................   Passed    1.01 sec
      Start 15: xmllint
15/15 Test #15: xmllint .......................................   Passed    0.40 sec

100% tests passed, 0 tests failed out of 15

Signed-off-by: Prajakta Gokhale prajaktg@amazon.com

thomas-moulard
thomas-moulard approved these changes May 18, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@thomas-moulard thomas-moulard left a comment

Choose a reason for hiding this comment

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

LGTM

@prajakta-gokhale
Copy link
Copy Markdown
Author

prajakta-gokhale commented May 18, 2019 •
edited
Loading

@thomas-moulard - please run the following CI job:

@thomas-moulard
Copy link
Copy Markdown
Contributor

thomas-moulard commented May 18, 2019

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

jacobperron
jacobperron reviewed May 18, 2019
View reviewed changes
rcl/src/rcl/init.c Outdated
}

rcl_ret_t rcl_ret = rcl_logging_fini();
rcl_ret_t rcl_ret = rcl_context_fini(context);
Copy link
Copy Markdown
Member

@jacobperron jacobperron May 18, 2019

Choose a reason for hiding this comment

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

I'm not sure about this change. I believe it is not assumed that rcl_shutdown will take care of this memory cleanup. Other places in our code are handling the context finalization alongside rcl_shutdown, e.g.

rcl/rcl/test/rcl/test_node.cpp

Lines 133 to 134 in 0c4c743

ASSERT_EQ(RCL_RET_OK, rcl_shutdown(&context));
ASSERT_EQ(RCL_RET_OK, rcl_context_fini(&context));

It also seems contrary to the documentation of rcl_context_t: http://docs.ros2.org/latest/api/rcl/structrcl__context__t.html

There is a valid state between shutdown and finalization.

jacobperron
jacobperron reviewed May 18, 2019
View reviewed changes
@prajakta-gokhale prajakta-gokhale force-pushed the fix-rcl-action-leaks branch 2 times, most recently from 690fa75 to 6a80a9a Compare May 19, 2019 22:37
@prajakta-gokhale
Copy link
Copy Markdown
Author

prajakta-gokhale commented May 20, 2019

Addressed @jacobperron feedback.

@dirk-thomas
Copy link
Copy Markdown
Member

dirk-thomas commented May 20, 2019

@thomas-moulard please rerun CI with the latest state.

@dirk-thomas dirk-thomas requested a review from jacobperron May 20, 2019 20:05
@thomas-moulard
Copy link
Copy Markdown
Contributor

thomas-moulard commented May 20, 2019

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

jacobperron
jacobperron reviewed May 20, 2019
View reviewed changes
Fix leaks in rcl_action unit tests
27bc14a 
Fix memory leaks detected by AddressSanitizer from rcl_action unit
tests.

Signed-off-by: Prajakta Gokhale <prajaktg@amazon.com>
@dirk-thomas dirk-thomas requested a review from jacobperron May 20, 2019 20:39
@jacobperron
Copy link
Copy Markdown
Member

jacobperron commented May 20, 2019 •
edited
Loading

  • Linux Build Status
  • Linux-aarch64Build Status
  • macOS Build Status
  • Windows Build Status

@jacobperron
Copy link
Copy Markdown
Member

jacobperron commented May 20, 2019

Linux failures are unrelated. Looking into it.

@jacobperron
Copy link
Copy Markdown
Member

jacobperron commented May 20, 2019

Re-triggered after fixing unrelated regression.

@jacobperron jacobperron merged commit 3806aa2 into ros2:master May 20, 2019
@prajakta-gokhale prajakta-gokhale deleted the fix-rcl-action-leaks branch May 20, 2019 22:07
@jacobperron jacobperron mentioned this pull request Jul 10, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jacobperron jacobperron jacobperron approved these changes

+1 more reviewer

@thomas-moulard thomas-moulard thomas-moulard approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@prajakta-gokhale @thomas-moulard @dirk-thomas @jacobperron