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

[IR] Implementation of QoS Features #1

Closed
wants to merge 19 commits into from
Closed

[IR] Implementation of QoS Features #1

wants to merge 19 commits into from

Conversation

mm318
Copy link

@mm318 mm318 commented Mar 20, 2019

Implementation of QoS features in the rclcpp package.

ross-desmond and others added 17 commits March 15, 2019 10:47
@ross-desmond @mm318
Add interfaces for events in memory_strategy
065a966 
Signed-off-by: Miaofei <[email protected]>
@mm318
refactor waitables
6ddded4 
Signed-off-by: Miaofei <[email protected]>
@mm318
add event callbacks to publisher, subscriber, client, service
a078e8e 
Signed-off-by: Miaofei <[email protected]>
@mm318
fix some ros2 build issues
1a4ccaa 
Signed-off-by: Miaofei <[email protected]>
@mm318
update client-facing API
e1ad5c2 
Signed-off-by: Miaofei <[email protected]>
@mm318
improve usability of the SubscriptionOptions and PublisherOptions cla…
cc8bb54 
…sses

Signed-off-by: Miaofei <[email protected]>
@sloretz @mm318
Attempt to fix cppcheck (ros2#646)
afca6f4 
Signed-off-by: Shane Loretz <[email protected]>
Signed-off-by: Miaofei <[email protected]>
@mm318
Add a method to the LifecycleNode class to get the logging interface (r…
ec4476e 
…os2#652)

There are getters for the other interfaces, but the logging interface
appears to have been overlooked.

Signed-off-by: Michael Jeronimo <[email protected]>
Signed-off-by: Miaofei <[email protected]>
@hidmic @mm318
update to use separated action types (ros2#601)
40ba3c6 
* match renamed action types

* fix action type casting

* rename type/field to use correct term

* rename custom GoalID type to avoid naming collision, update types using unique_identifier_msgs

* remove obsolete comments

* change signature of set_succeeded / set_canceled

* change signature of     on_terminal_state_(uuid_, result_msg);set_succeeded / set_canceled

* change signature of set_aborted

* change signature of publish_feedback

* update another test

Signed-off-by: Miaofei <[email protected]>
@jacobperron @mm318
Add Doxyfile for rclcpp_action
6821fa6 
Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Miaofei <[email protected]>
@jacobperron @mm318
Add documentation to rclcpp_action
b7cdf8f 
Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Miaofei <[email protected]>
@emersonknapp @mm318
Don't hardcode int64_t for duration type representations (ros2#648)
22a6484 
In LLVM's `libcxx`, `int64_t` doesn't match chrono literals. See example below. To compile, run  `clang++-6.0 -stdlib=libc++ -std=c++14 TEST.cpp`

```
using namespace std::chrono_literals;

template<typename RatioT = std::milli>
bool
wait_for_service(
   std::chrono::duration<int64_t, RatioT> timeout
)
{
   return timeout == std::chrono::nanoseconds(0);
}

int main() {
   wait_for_service(2s);
   return 0;
}

```

Result of compilation
```
TEST.cpp:6:1: note: candidate template ignored: could not match 'long' against 'long long'
wait_for_service(
```

Signed-off-by: Emerson Knapp <[email protected]>
Signed-off-by: Steven! Ragnarök <[email protected]>
Signed-off-by: Miaofei <[email protected]>
@pbaughman @mm318
Fix test_time_source test (ros2#639)
80e3d48 
* Fix flakey test

Signed-off-by: Pete Baughman <[email protected]>

* Fix lint and uncrustify issues

Signed-off-by: Pete Baughman <[email protected]>
Signed-off-by: Miaofei <[email protected]>
@mm318
fix lint errors
bcabadc 
Signed-off-by: Miaofei <[email protected]>
@mm318
apply uncrustify
993788f 
Signed-off-by: Miaofei <[email protected]>
@mm318
fix wait for service memory leak bug (ros2#656)
a77d110 
* when call wait for service in an while loop, the event will be make forever and never release
* fix it by: creating it when we need

Signed-off-by: reed-lau <[email protected]>
Signed-off-by: Miaofei <[email protected]>
Merge remote-tracking branch 'upstream/master' into qos
8d94f1b 
Signed-off-by: Emerson Knapp <[email protected]>
emersonknapp
emersonknapp reviewed Mar 26, 2019
View reviewed changes
rclcpp/include/rclcpp/node.hpp
@@ -142,76 +142,27 @@ class Node : public std::enable_shared_from_this<Node>
/// Create and return a Publisher.
/**
* \param[in] topic_name The topic for this publisher to publish on.
* \param[in] qos_history_depth The depth of the publisher message queue.
Copy link

Choose a reason for hiding this comment

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

Reminder: we need to add back in the pre-existing create_publisher and create_subscriber APIs, and to mark them as deprecated

emersonknapp
emersonknapp reviewed Mar 26, 2019
View reviewed changes
rclcpp/include/rclcpp/publisher_options.hpp

RCLCPP_PUBLIC
const QOSLifespanEventCallbackType &
lifespan_callback() const
Copy link

Choose a reason for hiding this comment

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

Reminder: remove all instance of lifespan_callback

@emersonknapp
Copy link

Whoops commenting on the wrong review, I'll close this one to reduce confusion

@mm318
Copy link
Author

mm318 commented Mar 26, 2019

The other review (#2) is qos-min vs. master, but maybe it should have been qos-min vs qos_reviewed.

@emersonknapp
Copy link

Changed the base on qos-min

thomas-moulard pushed a commit that referenced this pull request May 15, 2019
Stop accessing deleted parameter information
44bd87b 
When undeclaring a parameter, the current implementation
acccesses already freed memory. This commit contains the
minimal change to solve this isue.

Before this fix:

Running main() from ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest_main.cc
Note: Google Test filter = TestNode.set_parameter_undeclared_parameters_not_allowed
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from TestNode
[ RUN      ] TestNode.set_parameter_undeclared_parameters_not_allowed
=================================================================
==14634==ERROR: AddressSanitizer: heap-use-after-free on address 0x614000008070 at pc 0x7f90d5eb2733 bp 0x7ffc79e38d00 sp 0x7ffc79e384a8
READ of size 12 at 0x614000008070 thread T0
    #0 0x7f90d5eb2732  (/usr/lib/x86_64-linux-gnu/libasan.so.4+0x79732)
    #1 0x555823664ce8 in void std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char*>(char*, char*, std::forward_iterator_tag) (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rclcpp/test_node+0xface8)
    #2 0x7f90d57c70d3 in rclcpp::Parameter::Parameter(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, rclcpp::ParameterValue const&) ../../src/ros2/rclcpp/rclcpp/src/rclcpp/parameter.cpp:49
    #3 0x7f90d577fbe6 in rclcpp::node_interfaces::NodeParameters::set_parameters_atomically(std::vector<rclcpp::Parameter, std::allocator<rclcpp::Parameter> > const&) ../../src/ros2/rclcpp/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp:499
    #4 0x7f90d573d8b9 in rclcpp::Node::set_parameters_atomically(std::vector<rclcpp::Parameter, std::allocator<rclcpp::Parameter> > const&) ../../src/ros2/rclcpp/rclcpp/src/rclcpp/node.cpp:271
    #5 0x7f90d573d4ac in rclcpp::Node::set_parameter(rclcpp::Parameter const&) ../../src/ros2/rclcpp/rclcpp/src/rclcpp/node.cpp:259
    #6 0x5558235e1c0d in TestNode_set_parameter_undeclared_parameters_not_allowed_Test::TestBody() ../../src/ros2/rclcpp/rclcpp/test/test_node.cpp:694
    #7 0x5558236fc2cf in void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest.cc:2447
    #8 0x5558236eed3b in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest.cc:2483
    ros2#9 0x55582369c94b in testing::Test::Run() ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest.cc:2522
    ros2#10 0x55582369dd76 in testing::TestInfo::Run() ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest.cc:2703
    ros2#11 0x55582369e91a in testing::TestCase::Run() ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest.cc:2825
    ros2#12 0x5558236b9a2b in testing::internal::UnitTestImpl::RunAllTests() ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest.cc:5216
    ros2#13 0x5558236fed74 in bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest.cc:2447
    ros2#14 0x5558236f1004 in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest.cc:2483
    ros2#15 0x5558236b67bf in testing::UnitTest::Run() ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest.cc:4824
    ros2#16 0x555823689d0e in RUN_ALL_TESTS() ../../../install-asan/gtest_vendor/src/gtest_vendor/include/gtest/gtest.h:2370
    ros2#17 0x555823689c54 in main ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest_main.cc:36
    ros2#18 0x7f90d4768b96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)
    ros2#19 0x5558235b07d9 in _start (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rclcpp/test_node+0x467d9)

0x614000008070 is located 48 bytes inside of 416-byte region [0x614000008040,0x6140000081e0)
freed by thread T0 here:
    #0 0x7f90d5f1a2d0 in operator delete(void*) (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xe12d0)
    #1 0x7f90d57abd0f in __gnu_cxx::new_allocator<std::_Rb_tree_node<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, rclcpp::node_interfaces::ParameterInfo> > >::deallocate(std::_Rb_tree_node<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, rclcpp::node_interfaces::ParameterInfo> >*, unsigned long) /usr/include/c++/7/ext/new_allocator.h:125
    #2 0x7f90d57a8908 in std::allocator_traits<std::allocator<std::_Rb_tree_node<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, rclcpp::node_interfaces::ParameterInfo> > > >::deallocate(std::allocator<std::_Rb_tree_node<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, rclcpp::node_interfaces::ParameterInfo> > >&, std::_Rb_tree_node<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, rclcpp::node_interfaces::ParameterInfo> >*, unsigned long) /usr/include/c++/7/bits/alloc_traits.h:462
    #3 0x7f90d57a133c in std::_Rb_tree<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, rclcpp::node_interfaces::ParameterInfo>, std::_Select1st<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, rclcpp::node_interfaces::ParameterInfo> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, rclcpp::node_interfaces::ParameterInfo> > >::_M_put_node(std::_Rb_tree_node<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, rclcpp::node_interfaces::ParameterInfo> >*) /usr/include/c++/7/bits/stl_tree.h:592
    #4 0x7f90d5796abd in std::_Rb_tree<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, rclcpp::node_interfaces::ParameterInfo>, std::_Select1st<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, rclcpp::node_interfaces::ParameterInfo> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, rclcpp::node_interfaces::ParameterInfo> > >::_M_drop_node(std::_Rb_tree_node<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, rclcpp::node_interfaces::ParameterInfo> >*) /usr/include/c++/7/bits/stl_tree.h:659
    #5 0x7f90d579d3d4 in std::_Rb_tree<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, rclcpp::node_interfaces::ParameterInfo>, std::_Select1st<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, rclcpp::node_interfaces::ParameterInfo> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, rclcpp::node_interfaces::ParameterInfo> > >::_M_erase_aux(std::_Rb_tree_const_iterator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, rclcpp::node_interfaces::ParameterInfo> >) /usr/include/c++/7/bits/stl_tree.h:2477
    #6 0x7f90d5792213 in std::_Rb_tree<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, rclcpp::node_interfaces::ParameterInfo>, std::_Select1st<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, rclcpp::node_interfaces::ParameterInfo> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, rclcpp::node_interfaces::ParameterInfo> > >::erase[abi:cxx11](std::_Rb_tree_iterator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, rclcpp::node_interfaces::ParameterInfo> >) /usr/include/c++/7/bits/stl_tree.h:1125
    #7 0x7f90d5789f68 in std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, rclcpp::node_interfaces::ParameterInfo, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, rclcpp::node_interfaces::ParameterInfo> > >::erase[abi:cxx11](std::_Rb_tree_iterator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, rclcpp::node_interfaces::ParameterInfo> >) (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/install-asan/rclcpp/lib/librclcpp.so+0x491f68)
    #8 0x7f90d577fb2a in rclcpp::node_interfaces::NodeParameters::set_parameters_atomically(std::vector<rclcpp::Parameter, std::allocator<rclcpp::Parameter> > const&) ../../src/ros2/rclcpp/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp:497
    ros2#9 0x7f90d573d8b9 in rclcpp::Node::set_parameters_atomically(std::vector<rclcpp::Parameter, std::allocator<rclcpp::Parameter> > const&) ../../src/ros2/rclcpp/rclcpp/src/rclcpp/node.cpp:271
    ros2#10 0x7f90d573d4ac in rclcpp::Node::set_parameter(rclcpp::Parameter const&) ../../src/ros2/rclcpp/rclcpp/src/rclcpp/node.cpp:259
    ros2#11 0x5558235e1c0d in TestNode_set_parameter_undeclared_parameters_not_allowed_Test::TestBody() ../../src/ros2/rclcpp/rclcpp/test/test_node.cpp:694
    ros2#12 0x5558236fc2cf in void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest.cc:2447
    ros2#13 0x5558236eed3b in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest.cc:2483
    ros2#14 0x55582369c94b in testing::Test::Run() ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest.cc:2522
    ros2#15 0x55582369dd76 in testing::TestInfo::Run() ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest.cc:2703
    ros2#16 0x55582369e91a in testing::TestCase::Run() ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest.cc:2825
    ros2#17 0x5558236b9a2b in testing::internal::UnitTestImpl::RunAllTests() ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest.cc:5216
    ros2#18 0x5558236fed74 in bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest.cc:2447
    ros2#19 0x5558236f1004 in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest.cc:2483
    ros2#20 0x5558236b67bf in testing::UnitTest::Run() ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest.cc:4824
    ros2#21 0x555823689d0e in RUN_ALL_TESTS() ../../../install-asan/gtest_vendor/src/gtest_vendor/include/gtest/gtest.h:2370
    ros2#22 0x555823689c54 in main ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest_main.cc:36
    ros2#23 0x7f90d4768b96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)

previously allocated by thread T0 here:
    #0 0x7f90d5f19458 in operator new(unsigned long) (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xe0458)
    #1 0x7f90d57aceba in __gnu_cxx::new_allocator<std::_Rb_tree_node<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, rclcpp::node_interfaces::ParameterInfo> > >::allocate(unsigned long, void const*) /usr/include/c++/7/ext/new_allocator.h:111
    #2 0x7f90d57a9c4c in std::allocator_traits<std::allocator<std::_Rb_tree_node<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, rclcpp::node_interfaces::ParameterInfo> > > >::allocate(std::allocator<std::_Rb_tree_node<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, rclcpp::node_interfaces::ParameterInfo> > >&, unsigned long) (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/install-asan/rclcpp/lib/librclcpp.so+0x4b1c4c)
    #3 0x7f90d57a3f6e in std::_Rb_tree<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, rclcpp::node_interfaces::ParameterInfo>, std::_Select1st<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, rclcpp::node_interfaces::ParameterInfo> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, rclcpp::node_interfaces::ParameterInfo> > >::_M_get_node() (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/install-asan/rclcpp/lib/librclcpp.so+0x4abf6e)
    #4 0x7f90d5799770 in std::_Rb_tree_node<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, rclcpp::node_interfaces::ParameterInfo> >* std::_Rb_tree<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, rclcpp::node_interfaces::ParameterInfo>, std::_Select1st<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, rclcpp::node_interfaces::ParameterInfo> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, rclcpp::node_interfaces::ParameterInfo> > >::_M_create_node<std::piecewise_construct_t const&, std::tuple<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&>, std::tuple<> >(std::piecewise_construct_t const&, std::tuple<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&>&&, std::tuple<>&&) (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/install-asan/rclcpp/lib/librclcpp.so+0x4a1770)
    #5 0x7f90d578f382 in std::_Rb_tree_iterator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, rclcpp::node_interfaces::ParameterInfo> > std::_Rb_tree<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, rclcpp::node_interfaces::ParameterInfo>, std::_Select1st<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, rclcpp::node_interfaces::ParameterInfo> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, rclcpp::node_interfaces::ParameterInfo> > >::_M_emplace_hint_unique<std::piecewise_construct_t const&, std::tuple<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&>, std::tuple<> >(std::_Rb_tree_const_iterator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, rclcpp::node_interfaces::ParameterInfo> >, std::piecewise_construct_t const&, std::tuple<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&>&&, std::tuple<>&&) /usr/include/c++/7/bits/stl_tree.h:2398
    #6 0x7f90d5788eba in std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, rclcpp::node_interfaces::ParameterInfo, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, rclcpp::node_interfaces::ParameterInfo> > >::operator[](std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) /usr/include/c++/7/bits/stl_map.h:493
    #7 0x7f90d577bc88 in __declare_parameter_common(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, rclcpp::ParameterValue const&, rcl_interfaces::msg::ParameterDescriptor_<std::allocator<void> > const&, std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, rclcpp::node_interfaces::ParameterInfo, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, rclcpp::node_interfaces::ParameterInfo> > >&, std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, rclcpp::ParameterValue, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, rclcpp::ParameterValue> > > const&, std::function<rcl_interfaces::msg::SetParametersResult_<std::allocator<void> > (std::vector<rclcpp::Parameter, std::allocator<rclcpp::Parameter> > const&)>, rcl_interfaces::msg::ParameterEvent_<std::allocator<void> >*) ../../src/ros2/rclcpp/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp:250
    #8 0x7f90d577c4f7 in rclcpp::node_interfaces::NodeParameters::declare_parameter(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, rclcpp::ParameterValue const&, rcl_interfaces::msg::ParameterDescriptor_<std::allocator<void> > const&) ../../src/ros2/rclcpp/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp:287
    ros2#9 0x7f90d573d1ec in rclcpp::Node::declare_parameter(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, rclcpp::ParameterValue const&, rcl_interfaces::msg::ParameterDescriptor_<std::allocator<void> > const&) ../../src/ros2/rclcpp/rclcpp/src/rclcpp/node.cpp:241
    ros2#10 0x555823645b6b in auto rclcpp::Node::declare_parameter<int>(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, int const&, rcl_interfaces::msg::ParameterDescriptor_<std::allocator<void> > const&) (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rclcpp/test_node+0xdbb6b)
    ros2#11 0x5558235e14ee in TestNode_set_parameter_undeclared_parameters_not_allowed_Test::TestBody() ../../src/ros2/rclcpp/rclcpp/test/test_node.cpp:689
    ros2#12 0x5558236fc2cf in void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest.cc:2447
    ros2#13 0x5558236eed3b in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest.cc:2483
    ros2#14 0x55582369c94b in testing::Test::Run() ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest.cc:2522
    ros2#15 0x55582369dd76 in testing::TestInfo::Run() ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest.cc:2703
    ros2#16 0x55582369e91a in testing::TestCase::Run() ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest.cc:2825
    ros2#17 0x5558236b9a2b in testing::internal::UnitTestImpl::RunAllTests() ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest.cc:5216
    ros2#18 0x5558236fed74 in bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest.cc:2447
    ros2#19 0x5558236f1004 in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest.cc:2483
    ros2#20 0x5558236b67bf in testing::UnitTest::Run() ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest.cc:4824
    ros2#21 0x555823689d0e in RUN_ALL_TESTS() ../../../install-asan/gtest_vendor/src/gtest_vendor/include/gtest/gtest.h:2370
    ros2#22 0x555823689c54 in main ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest_main.cc:36
    ros2#23 0x7f90d4768b96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)

SUMMARY: AddressSanitizer: heap-use-after-free (/usr/lib/x86_64-linux-gnu/libasan.so.4+0x79732)
Shadow bytes around the buggy address:
  0x0c287fff8fb0: 00 00 00 00 00 00 00 00 00 00 00 00 fa fa fa fa
  0x0c287fff8fc0: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
  0x0c287fff8fd0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c287fff8fe0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c287fff8ff0: fd fd fd fd fd fd fd fd fd fd fd fd fa fa fa fa
=>0x0c287fff9000: fa fa fa fa fa fa fa fa fd fd fd fd fd fd[fd]fd
  0x0c287fff9010: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c287fff9020: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c287fff9030: fd fd fd fd fd fd fd fd fd fd fd fd fa fa fa fa
  0x0c287fff9040: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c287fff9050: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==14634==ABORTING

After this fix:

Running main() from ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest_main.cc
Note: Google Test filter = TestNode.set_parameter_undeclared_parameters_not_allowed
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from TestNode
[ RUN      ] TestNode.set_parameter_undeclared_parameters_not_allowed
[       OK ] TestNode.set_parameter_undeclared_parameters_not_allowed (51 ms)
[----------] 1 test from TestNode (51 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (51 ms total)
[  PASSED  ] 1 test.

=================================================================
==8511==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 64 byte(s) in 1 object(s) allocated from:
    #0 0x7f49af480458 in operator new(unsigned long) (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xe0458)
    #1 0x7f49aecb6b21 in rclcpp::NodeOptions::get_rcl_node_options() const ../../src/ros2/rclcpp/rclcpp/src/rclcpp/node_options.cpp:84
    #2 0x7f49aeca1e63 in rclcpp::Node::Node(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, rclcpp::NodeOptions const&) ../../src/ros2/rclcpp/rclcpp/src/rclcpp/node.cpp:115
    #3 0x7f49aeca1a34 in rclcpp::Node::Node(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, rclcpp::NodeOptions const&) ../../src/ros2/rclcpp/rclcpp/src/rclcpp/node.cpp:103
    #4 0x55ee8e2e4473 in void __gnu_cxx::new_allocator<rclcpp::Node>::construct<rclcpp::Node, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, rclcpp::NodeOptions&>(rclcpp::Node*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&&, rclcpp::NodeOptions&) (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rclcpp/test_no
de+0x10e473)
    #5 0x55ee8e2de58f in void std::allocator_traits<std::allocator<rclcpp::Node> >::construct<rclcpp::Node, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, rclcpp::NodeOptions&>(std::allocator<rclcpp::Node>&, rclcpp::Node*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&&, rclcpp::NodeOptions&) (/home/ANT.AMAZON.C
OM/tmoulard/ros2_ws/build-asan/rclcpp/test_node+0x10858f)
    #6 0x55ee8e2da516 in std::_Sp_counted_ptr_inplace<rclcpp::Node, std::allocator<rclcpp::Node>, (__gnu_cxx::_Lock_policy)2>::_Sp_counted_ptr_inplace<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, rclcpp::NodeOptions&>(std::allocator<rclcpp::Node>, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&&, rclcpp::NodeOp
tions&) (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rclcpp/test_node+0x104516)
    #7 0x55ee8e2d25b2 in std::__shared_count<(__gnu_cxx::_Lock_policy)2>::__shared_count<rclcpp::Node, std::allocator<rclcpp::Node>, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, rclcpp::NodeOptions&>(std::_Sp_make_shared_tag, rclcpp::Node*, std::allocator<rclcpp::Node> const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::alloc
ator<char> >&&, rclcpp::NodeOptions&) (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rclcpp/test_node+0xfc5b2)
    #8 0x55ee8e2c84da in std::__shared_ptr<rclcpp::Node, (__gnu_cxx::_Lock_policy)2>::__shared_ptr<std::allocator<rclcpp::Node>, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, rclcpp::NodeOptions&>(std::_Sp_make_shared_tag, std::allocator<rclcpp::Node> const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&&, rcl
cpp::NodeOptions&) (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rclcpp/test_node+0xf24da)
    ros2#9 0x55ee8e2bfd60 in std::shared_ptr<rclcpp::Node>::shared_ptr<std::allocator<rclcpp::Node>, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, rclcpp::NodeOptions&>(std::_Sp_make_shared_tag, std::allocator<rclcpp::Node> const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&&, rclcpp::NodeOptions&) (/home/ANT.AM
AZON.COM/tmoulard/ros2_ws/build-asan/rclcpp/test_node+0xe9d60)
    ros2#10 0x55ee8e2b6b18 in std::shared_ptr<rclcpp::Node> std::allocate_shared<rclcpp::Node, std::allocator<rclcpp::Node>, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, rclcpp::NodeOptions&>(std::allocator<rclcpp::Node> const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&&, rclcpp::NodeOptions&) (/home/ANT.AMAZ
ON.COM/tmoulard/ros2_ws/build-asan/rclcpp/test_node+0xe0b18)
    ros2#11 0x55ee8e2ad89e in std::shared_ptr<rclcpp::Node> std::make_shared<rclcpp::Node, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, rclcpp::NodeOptions&>(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&&, rclcpp::NodeOptions&) (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rclcpp/test_node+0xd789e)
    ros2#12 0x55ee8e245438 in TestNode_set_parameter_undeclared_parameters_not_allowed_Test::TestBody() ../../src/ros2/rclcpp/rclcpp/test/test_node.cpp:607
    ros2#13 0x55ee8e35d111 in void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest.cc:2447
    ros2#14 0x55ee8e34fb7d in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest.cc:2483
    ros2#15 0x55ee8e2fd78d in testing::Test::Run() ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest.cc:2522
    ros2#16 0x55ee8e2febb8 in testing::TestInfo::Run() ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest.cc:2703
    ros2#17 0x55ee8e2ff75c in testing::TestCase::Run() ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest.cc:2825
    ros2#18 0x55ee8e31a86d in testing::internal::UnitTestImpl::RunAllTests() ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest.cc:5216
    ros2#19 0x55ee8e35fbb6 in bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest.cc:2447
    ros2#20 0x55ee8e351e46 in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest.cc:2483
    ros2#21 0x55ee8e317601 in testing::UnitTest::Run() ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest.cc:4824
    ros2#22 0x55ee8e2eab50 in RUN_ALL_TESTS() ../../../install-asan/gtest_vendor/src/gtest_vendor/include/gtest/gtest.h:2370
    ros2#23 0x55ee8e2eaa96 in main ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest_main.cc:36
    ros2#24 0x7f49adccfb96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)

SUMMARY: AddressSanitizer: 64 byte(s) leaked in 1 allocation(s).

Note: after this fix, get_rcl_node_options() still leak. This is a
problem independent from this issue and will be solved independently.

Signed-off-by: Thomas Moulard <[email protected]>
emersonknapp pushed a commit that referenced this pull request Aug 3, 2021
@nnmm @wjwwood
@jacobperron @Karsten1987
Add generic publisher and generic subscription for serialized messages (
95adde2 
ros2#1452)

* Copying files from rosbag2

The generic_* files are from rosbag2_transport
typesupport_helpers incl. test is from rosbag2_cpp
memory_management.hpp is from rosbag2_test_common
test_pubsub.cpp was renamed from test_rosbag2_node.cpp from rosbag2_transport

Signed-off-by: Nikolai Morin <[email protected]>

* Rebrand into rclcpp_generic

Add package.xml, CMakeLists.txt, Doxyfile, README.md and CHANGELOG.rst
Rename namespaces
Make GenericPublisher and GenericSubscription self-contained by storing shared library
New create() methods that return shared pointers
Add docstrings
Include only what is needed
Make linters & tests pass

Signed-off-by: Nikolai Morin <[email protected]>

* Review feedback

* Delete CHANGELOG.rst
* Enable cppcheck
* Remove all references to rosbag2/ros2bag

Signed-off-by: Nikolai Morin <[email protected]>

* Move rclpp_generic into rclcpp

Signed-off-by: Nikolai Morin <[email protected]>

* Rename namespace rclcpp_generic to rclcpp::generic

Signed-off-by: Nikolai Morin <[email protected]>

* Free 'create' functions instead of static functions in class

Signed-off-by: Nikolai Morin <[email protected]>

* Remove 'generic' subdirectory and namespace hierarchy

Signed-off-by: Nikolai Morin <[email protected]>

* Order includes according to style guide

Signed-off-by: Nikolai Morin <[email protected]>

* Remove extra README.md

Signed-off-by: Nikolai Morin <[email protected]>

* Also add brief to class docs

Signed-off-by: Nikolai Morin <[email protected]>

* Make ament_index_cpp a build_depend

Signed-off-by: Nikolai Morin <[email protected]>

* Add to rclcpp.hpp

Signed-off-by: Nikolai Morin <[email protected]>

* Remove memory_management, use rclcpp::SerializedMessage in GenericPublisher::publish

Signed-off-by: Nikolai Morin <[email protected]>

* Clean up the typesupport_helpers

Signed-off-by: Nikolai Morin <[email protected]>

* Use make_shared, add UnimplementedError

Signed-off-by: Nikolai Morin <[email protected]>

* Add more comments, make member variable private, remove unnecessary include

Signed-off-by: Nikolai Morin <[email protected]>

* Apply suggestions from code review

Co-authored-by: William Woodall <[email protected]>
Signed-off-by: Nikolai Morin <[email protected]>

* Rename test

Signed-off-by: Nikolai Morin <[email protected]>

* Update copyright and remove ament_target_dependencies for test

Signed-off-by: Nikolai Morin <[email protected]>

* Accept PublisherOptions and SubscriptionOptions

Signed-off-by: Nikolai Morin <[email protected]>

* Remove target_include_directories

Signed-off-by: Nikolai Morin <[email protected]>

* Add explanatory comment to SubscriptionBase

Signed-off-by: Nikolai Morin <[email protected]>

* Use kSolibPrefix and kSolibExtension from rcpputils

Signed-off-by: Nikolai Morin <[email protected]>

* Fix downstream build failure by making ament_index_cpp a build_export_depend

Signed-off-by: Nikolai Morin <[email protected]>

* Use path_for_library(), fix documentation nitpicks

Signed-off-by: Nikolai Morin <[email protected]>

* Improve error handling in get_typesupport_handle

Signed-off-by: Nikolai Morin <[email protected]>

* Accept SubscriptionOptions in GenericSubscription

Signed-off-by: Nikolai Morin <[email protected]>

* Make use of PublisherOptions in GenericPublisher

Signed-off-by: Nikolai Morin <[email protected]>

* Document typesupport_helpers

Signed-off-by: Nikolai Morin <[email protected]>

* Improve documentation

Signed-off-by: Nikolai Morin <[email protected]>

* Use std::function instead of function pointer

Co-authored-by: William Woodall <[email protected]>
Signed-off-by: Nikolai Morin <[email protected]>

* Minimize vertical whitespace

Signed-off-by: Nikolai Morin <[email protected]>

* Add TODO for callback with message info

Signed-off-by: Nikolai Morin <[email protected]>

* Link issue in TODO

Signed-off-by: Nikolai Morin <[email protected]>

* Add missing include for functional

Signed-off-by: nnmm <[email protected]>

* Fix compilation

Signed-off-by: Jacob Perron <[email protected]>

* Fix lint

Signed-off-by: Jacob Perron <[email protected]>

* Address review comments (#1)

* fix redefinition of default template arguments

Signed-off-by: Karsten Knese <[email protected]>

* address review comments

Signed-off-by: Karsten Knese <[email protected]>

* rename test executable

Signed-off-by: Karsten Knese <[email protected]>

* add functionality to lifecycle nodes

Signed-off-by: Karsten Knese <[email protected]>

* Refactor typesupport helpers

* Make extract_type_identifier function private
* Remove unused extract_type_and_package function
* Update unit tests

Signed-off-by: Jacob Perron <[email protected]>

* Remove note about ament from classes

This comment only applies to the free functions.

Signed-off-by: Jacob Perron <[email protected]>

* Fix formatting

Co-authored-by: Karsten Knese <[email protected]>

* Fix warning

Possible loss of data from double to rcutils_duration_value_t

Signed-off-by: Jacob Perron <[email protected]>

* Add missing visibility macros

Signed-off-by: Jacob Perron <[email protected]>

Co-authored-by: William Woodall <[email protected]>
Co-authored-by: Jacob Perron <[email protected]>
Co-authored-by: Karsten Knese <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emersonknapp emersonknapp emersonknapp left review comments

@dabonnie dabonnie Awaiting requested review from dabonnie

@nburek nburek Awaiting requested review from nburek

@ross-desmond ross-desmond Awaiting requested review from ross-desmond

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

Successfully merging this pull request may close these issues.
7 participants
@mm318 @emersonknapp @ross-desmond @sloretz @hidmic @jacobperron @pbaughman