Fix leaks in rcl_action unit tests #442

prajakta-gokhale · 2019-05-18T00:21:37Z

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

LGTM

prajakta-gokhale · 2019-05-18T00:34:03Z

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

Gist file: https://gist.github.com/prajakta-gokhale/690a43cdf41e68a5670f9cac9ffa1ed7
Additional BUILD flags: --packages-up-to rcl_action
Additional TEST flags: --packages-select rcl_action
CI Job: ci_launcher

thomas-moulard · 2019-05-18T00:41:16Z

Linux
Linux-aarch64
macOS
Windows

jacobperron · 2019-05-18T01:23:10Z

rcl/src/rcl/init.c

@@ -179,7 +179,13 @@ rcl_shutdown(rcl_context_t * context)
    return rcl_convert_rmw_ret_to_rcl_ret(rmw_ret);
  }

-  rcl_ret_t rcl_ret = rcl_logging_fini();
+  rcl_ret_t rcl_ret = rcl_context_fini(context);


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.

rcl_action/test/rcl_action/test_graph.cpp

prajakta-gokhale · 2019-05-20T18:33:11Z

Addressed @jacobperron feedback.

dirk-thomas · 2019-05-20T20:05:24Z

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

thomas-moulard · 2019-05-20T20:07:18Z

Linux
Linux-aarch64
macOS
Windows

rcl_action/test/rcl_action/test_graph.cpp

Fix memory leaks detected by AddressSanitizer from rcl_action unit tests. Signed-off-by: Prajakta Gokhale <prajaktg@amazon.com>

jacobperron · 2019-05-20T20:47:57Z

Linux
Linux-aarch64
macOS
Windows

jacobperron · 2019-05-20T21:14:50Z

Linux failures are unrelated. Looking into it.

jacobperron · 2019-05-20T21:39:51Z

Re-triggered after fixing unrelated regression.

thomas-moulard approved these changes May 18, 2019

View reviewed changes

jacobperron reviewed May 18, 2019

View reviewed changes

rcl_action/test/rcl_action/test_graph.cpp Outdated Show resolved Hide resolved

prajakta-gokhale force-pushed the fix-rcl-action-leaks branch 2 times, most recently from 690fa75 to 6a80a9a Compare May 19, 2019 22:37

dirk-thomas requested a review from jacobperron May 20, 2019 20:05

jacobperron reviewed May 20, 2019

View reviewed changes

rcl_action/test/rcl_action/test_graph.cpp Outdated Show resolved Hide resolved

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>

prajakta-gokhale force-pushed the fix-rcl-action-leaks branch from 6a80a9a to 27bc14a Compare May 20, 2019 20:21

dirk-thomas requested a review from jacobperron May 20, 2019 20:39

jacobperron approved these changes May 20, 2019

View reviewed changes

jacobperron merged commit 3806aa2 into ros2:master May 20, 2019

prajakta-gokhale deleted the fix-rcl-action-leaks branch May 20, 2019 22:07

jacobperron mentioned this pull request Jul 10, 2019

[YAML Parser] Support parameter value parsing #471

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix leaks in rcl_action unit tests #442

Fix leaks in rcl_action unit tests #442

prajakta-gokhale commented May 18, 2019

thomas-moulard left a comment

prajakta-gokhale commented May 18, 2019 •

edited

Loading

thomas-moulard commented May 18, 2019

jacobperron May 18, 2019

prajakta-gokhale commented May 20, 2019

dirk-thomas commented May 20, 2019

thomas-moulard commented May 20, 2019

jacobperron commented May 20, 2019 •

edited

Loading

jacobperron commented May 20, 2019

jacobperron commented May 20, 2019

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

Fix leaks in rcl_action unit tests #442

Fix leaks in rcl_action unit tests #442

Conversation

prajakta-gokhale commented May 18, 2019

thomas-moulard left a comment

Choose a reason for hiding this comment

prajakta-gokhale commented May 18, 2019 • edited Loading

thomas-moulard commented May 18, 2019

jacobperron May 18, 2019

Choose a reason for hiding this comment

prajakta-gokhale commented May 20, 2019

dirk-thomas commented May 20, 2019

thomas-moulard commented May 20, 2019

jacobperron commented May 20, 2019 • edited Loading

jacobperron commented May 20, 2019

jacobperron commented May 20, 2019

prajakta-gokhale commented May 18, 2019 •

edited

Loading

jacobperron commented May 20, 2019 •

edited

Loading