String comparison with equal-to operator does not work for c strings, in type_support_common.cpp #703
Comments
It returns Please notice that it's a trick to compare the |
|
The |
The second step fails: rosidl_typesupport_introspection_c
c 0 rosidl_typesupport_introspection_c
cpp 0 rosidl_typesupport_introspection_cppSeems that they are not pointing to the same address in memory. It is |
|
Fails in this test, using TestGenericPublisher// Copyright 2023 Milan Vukov
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
#include <thread>
#include "gtest/gtest.h"
#include "rclcpp/rclcpp.hpp"
// Inspired by an example in https://github.com/ros2/rclcpp/issues/2146
// For this to work, a rclcpp patch from
// https://github.com/mvukov/rules_ros2/pull/117 is required.
TEST(TestGenericPublisher,
WhenPublisherDeletedInThread_EnsureCleanNodeCleanup) {
auto node = rclcpp::Node::make_shared("pub_node");
auto publisher =
node->create_generic_publisher("pub_topic", "std_msgs/String", 10);
auto reset_publisher = std::async(std::launch::async, [&node, &publisher]() {
std::this_thread::sleep_for(std::chrono::milliseconds(200));
publisher.reset();
// New: Create a timer that gets immediately out of scope (should never
// fire) to trigger cleanup of the publisher.
node->create_wall_timer(std::chrono::seconds(1), []() {});
});
rclcpp::executors::SingleThreadedExecutor executor;
executor.add_node(node);
executor.spin_until_future_complete(reset_publisher);
}
int main(int argc, char** argv) {
rclcpp::init(argc, argv);
testing::InitGoogleTest(&argc, argv);
const int result = RUN_ALL_TESTS();
if (!rclcpp::shutdown() || result) {
return EXIT_FAILURE;
}
return EXIT_SUCCESS;
}
|
|
I am afraid that I can't reproduce this issue by rolling in my machine. diff --git a/rclcpp/test/rclcpp/test_generic_pubsub.cpp b/rclcpp/test/rclcpp/test_generic_pubsub.cpp
index f4cef0b7..56b74089 100644
--- a/rclcpp/test/rclcpp/test_generic_pubsub.cpp
+++ b/rclcpp/test/rclcpp/test_generic_pubsub.cpp
@@ -263,3 +263,24 @@ TEST_F(RclcppGenericNodeFixture, generic_publisher_uses_qos)
// It normally takes < 20ms, 5s chosen as "a very long time"
ASSERT_TRUE(wait_for(connected, 5s));
}
+
+
+TEST_F(RclcppGenericNodeFixture,
+ WhenPublisherDeletedInThread_EnsureCleanNodeCleanup) {
+ auto node = rclcpp::Node::make_shared("pub_node");
+ auto publisher =
+ node->create_generic_publisher("pub_topic", "std_msgs/String", 10);
+
+ auto reset_publisher = std::async(std::launch::async, [&node, &publisher]() {
+ std::this_thread::sleep_for(std::chrono::milliseconds(200));
+ publisher.reset();
+
+ // New: Create a timer that gets immediately out of scope (should never
+ // fire) to trigger cleanup of the publisher.
+ node->create_wall_timer(std::chrono::seconds(1), []() {});
+ });
+
+ rclcpp::executors::SingleThreadedExecutor executor;
+ executor.add_node(node);
+ executor.spin_until_future_complete(reset_publisher);
+}
\ No newline at end of filetest log: chenlh rclcpp $ RMW_IMPLEMENTATION=rmw_fastrtps_dynamic_cpp ./test_generic_pubsub__rmw_fastrtps_dynamic_cpp --gtest_filter="*.WhenPublisherDeletedInThread_EnsureCleanNodeCleanup"
Running main() from gmock_main.cc
Note: Google Test filter = *.WhenPublisherDeletedInThread_EnsureCleanNodeCleanup
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from RclcppGenericNodeFixture
[ RUN ] RclcppGenericNodeFixture.WhenPublisherDeletedInThread_EnsureCleanNodeCleanup
[ OK ] RclcppGenericNodeFixture.WhenPublisherDeletedInThread_EnsureCleanNodeCleanup (2291 ms)
[----------] 1 test from RclcppGenericNodeFixture (2291 ms total)
[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (2295 ms total)
[ PASSED ] 1 test.
|
|
This line looks interesting? Could be a compiler difference? |
Yeah, I noticed that, but it's not a big deal. Because it will be assigned with |
|
It is being called two times: type_support_ptr TypeSupportRegistry::get_message_type_support(
const rosidl_message_type_support_t * ros_type_support)
{
auto creator_fun = [&ros_type_support]() -> type_support_ptr
{
std::cerr << "Got: " << &ros_type_support->typesupport_identifier
<< " " << ros_type_support->typesupport_identifier << "\n";
std::cerr << "Expected C: " << &rosidl_typesupport_introspection_c__identifier
<< " " << rosidl_typesupport_introspection_c__identifier << "\n";
std::cerr << "Expected CPP: " << &rosidl_typesupport_introspection_cpp::typesupport_identifier
<< " " << rosidl_typesupport_introspection_cpp::typesupport_identifier << "\n";
if (using_introspection_c_typesupport(ros_type_support->typesupport_identifier)) {
...Got: 0x7f23f96d0d20 rosidl_typesupport_introspection_cpp
Expected C: 0x7f23f96d0f58 rosidl_typesupport_introspection_c
Expected CPP: 0x7f23f96d0f60 rosidl_typesupport_introspection_cpp
Got: 0x55e8fd9cf0d0 rosidl_typesupport_introspection_c
Expected C: 0x7f23f96d0f58 rosidl_typesupport_introspection_c
Expected CPP: 0x7f23f96d0f60 rosidl_typesupport_introspection_cppSomehow there is a memory address mismatch |
|
I think you need to output |
|
Yields same result Got: 0x7f42074c78c8 rosidl_typesupport_introspection_cpp
Expected C: 0x7f42074c78a0 rosidl_typesupport_introspection_c
Expected CPP: 0x7f42074c78c8 rosidl_typesupport_introspection_cpp
Got: 0x563a0a646b28 rosidl_typesupport_introspection_c
Expected C: 0x7f42074c78a0 rosidl_typesupport_introspection_c
Expected CPP: 0x7f42074c78c8 rosidl_typesupport_introspection_cpp |
|
Doing some more digging, I noticed that here Shouldn't it use |
|
We've had this conversation in the past, though I cannot find the issue right at the moment. The short answer is that we have two unanswered questions here:
If we can determine the answer to both of those, then we can make a decision about which way to go. |
|
Proof: bool
using_introspection_c_typesupport(const char * typesupport_identifier)
{
bool equality = typesupport_identifier == rosidl_typesupport_introspection_c__identifier;
return typesupport_identifier == rosidl_typesupport_introspection_c__identifier;
}Thread 1 "generic_publish" hit Breakpoint 1, using_introspection_c_typesupport (typesupport_identifier=0x5555558dab08 "rosidl_typesupport_introspection_c") at external/ros2_rmw_fastrtps/rmw_fastrtps_dynamic_cpp/src/type_support_common.cpp:29
29 in external/ros2_rmw_fastrtps/rmw_fastrtps_dynamic_cpp/src/type_support_common.cpp
(gdb) p typesupport_identifier
$8 = 0x5555558dab08 "rosidl_typesupport_introspection_c"
(gdb) p rosidl_typesupport_introspection_c__identifier
$9 = 0x5555558dab08 "rosidl_typesupport_introspection_c"
(gdb) p equality
$10 = false <-----------------Pointers are correct, but checking equality with equal-to operator results in FALSE. |
|
So the question is: what is different about this situation? Because we don't normally see this problem. |
#include <cstdint>
bool
using_introspection_c_typesupport(const char * typesupport_identifier)
{
auto a = reinterpret_cast<std::uintptr_t>(typesupport_identifier);
auto b = reinterpret_cast<std::uintptr_t>(rosidl_typesupport_introspection_c__identifier);
bool equality = a == b;
return a == b;
}Thread 1 "generic_publish" hit Breakpoint 1, using_introspection_c_typesupport (typesupport_identifier=0x5555558dab08 "rosidl_typesupport_introspection_c") at external/ros2_rmw_fastrtps/rmw_fastrtps_dynamic_cpp/src/type_support_common.cpp:35
35 in external/ros2_rmw_fastrtps/rmw_fastrtps_dynamic_cpp/src/type_support_common.cpp
(gdb) p typesupport_identifier
$1 = 0x5555558dab08 "rosidl_typesupport_introspection_c"
(gdb) p rosidl_typesupport_introspection_c__identifier
$2 = 0x5555558dab08 "rosidl_typesupport_introspection_c"
(gdb) p equality
$3 = false
(gdb) p a
$4 = 93824995928840
(gdb) p b
$5 = 140737344691008values resulting from cast to (gdb) x/s 0x5555558dab08
0x5555558dab08: "rosidl_typesupport_introspection_c"
(gdb) x/s 0x7ffff76fdb40
0x7ffff76fdb40: "rosidl_typesupport_introspection_c" |
|
generic_publisher_tests.tar.gz If anyone is interested to investigate further, I think I have reached my capability... Invoke gdb with: AMENT_PREFIX_PATH="generic_publisher_tests_launch_ament_setup" gdb generic_publisher_tests_impl |
|
It seems it's the build issue(link phase) of Please see the Bazel build parameter file for
A similar demo as below |
|
It's better to build the |
|
Yes, forcing dynamic linking of the test binary did indeed work! Thank you! |
Bug report
Required Info:
Steps to reproduce issue
This problem was discovered in and affected this PR mvukov/rules_ros2#148
Got this error:
'Unknown typesupport identifier, at external/ros2_rmw_fastrtps/rmw_fastrtps_dynamic_cpp/src/type_support_registry.cpp:93'Added some prints here:
Outcome:
Expected behavior
rosidl_typesupport_introspection_c == rosidl_typesupport_introspection_cActual behavior
rosidl_typesupport_introspection_c != rosidl_typesupport_introspection_cImplementation considerations
Use
strcmp:or
std::string().compareUpdate both these lines:
rmw_fastrtps/rmw_fastrtps_dynamic_cpp/src/type_support_common.cpp
Line 28 in b45e735
rmw_fastrtps/rmw_fastrtps_dynamic_cpp/src/type_support_common.cpp
Line 34 in b45e735
The text was updated successfully, but these errors were encountered: