Skip to content

Commit

Permalink
Adds example to show bug in which cb method is executed after instanc…
Browse files Browse the repository at this point in the history
…e is destroyed.
  • Loading branch information
francocipollone committed Mar 22, 2021
1 parent 50f5793 commit 9d5427f
Show file tree
Hide file tree
Showing 4 changed files with 246 additions and 0 deletions.
15 changes: 15 additions & 0 deletions example/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,21 @@ if (EXISTS "${CMAKE_SOURCE_DIR}/subscriber.cc")
target_link_libraries(subscriber ignition-transport${IGN_TRANSPORT_VER}::core)
endif()

if (EXISTS "${CMAKE_SOURCE_DIR}/publisher_bug_example.cc")
add_executable(publisher_bug_example publisher_bug_example.cc)
target_link_libraries(publisher_bug_example ignition-transport${IGN_TRANSPORT_VER}::core)
endif()

if (EXISTS "${CMAKE_SOURCE_DIR}/subscriber_bug_example.cc")
add_executable(subscriber_bug_example subscriber_bug_example.cc)
target_link_libraries(subscriber_bug_example ignition-transport${IGN_TRANSPORT_VER}::core)
endif()

if (EXISTS "${CMAKE_SOURCE_DIR}/subscriber_bug_workaround_example.cc")
add_executable(subscriber_bug_workaround_example subscriber_bug_workaround_example.cc)
target_link_libraries(subscriber_bug_workaround_example ignition-transport${IGN_TRANSPORT_VER}::core)
endif()

if (EXISTS "${CMAKE_SOURCE_DIR}/subscriber_generic.cc")
add_executable(subscriber_generic subscriber_generic.cc)
target_link_libraries(subscriber_generic
Expand Down
74 changes: 74 additions & 0 deletions example/publisher_bug_example.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
/*
* Copyright (C) 2014 Open Source Robotics Foundation
*
* 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 <atomic>
#include <chrono>
#include <csignal>
#include <iostream>
#include <string>
#include <thread>
#include <ignition/msgs.hh>
#include <ignition/transport.hh>

/// \brief Flag used to break the publisher loop and terminate the program.
static std::atomic<bool> g_terminatePub(false);

//////////////////////////////////////////////////
/// \brief Function callback executed when a SIGINT or SIGTERM signals are
/// captured. This is used to break the infinite loop that publishes messages
/// and exit the program smoothly.
void signal_handler(int _signal)
{
if (_signal == SIGINT || _signal == SIGTERM)
g_terminatePub = true;
}

//////////////////////////////////////////////////
int main(int argc, char **argv)
{
// Install a signal handler for SIGINT and SIGTERM.
std::signal(SIGINT, signal_handler);
std::signal(SIGTERM, signal_handler);

// Create a transport node and advertise a topic.
ignition::transport::Node node;
std::string topic = "/foo";

auto pub = node.Advertise<ignition::msgs::StringMsg>(topic);
if (!pub)
{
std::cerr << "Error advertising topic [" << topic << "]" << std::endl;
return -1;
}

// Prepare the message.
ignition::msgs::StringMsg msg;
unsigned int count{};
// Publish messages at 1Hz.
while (!g_terminatePub)
{
msg.set_data("HELLO" + std::to_string(count));
if (!pub.Publish(msg))
break;

std::cout << "Publishing " << msg.data() << " on topic [" << topic << "]" << std::endl;
count++;
// No loop frequency is set in order to force a big amount of messages being published.
}

return 0;
}
64 changes: 64 additions & 0 deletions example/subscriber_bug_example.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@

/*
* This example tries to show the presence of a bug in which a callback method
* is executed after that the instance of the class that holds it is destroyed.
*
* Execute this after that the publisher is started: `publisher_bug_example`.
*/

#include <iostream>
#include <string>
#include <ignition/msgs.hh>
#include <ignition/transport.hh>

// Subscribes to a topic and prints the value of the message when the callback method is called.
class Monitor {
public:
// Constructs a monitor for the given topic.
// @param topic_name Valid ignition transport topic name.
explicit Monitor(const std::string& topic_name) { node_.Subscribe(topic_name, &Monitor::OnTopicMessage, this); }

~Monitor() {std::cout << "****\n[Monitor] Class Destructor!\n****\n" << std::endl;}

private:
// Ignition subscriber callback, updating state.
//
// @param message Message received.
void OnTopicMessage(const ignition::msgs::StringMsg& message) {
std::cout << "[Callback]" << std::endl;
std::lock_guard<std::mutex> guard(mutex_);
last_message_ = message;
message_count_++;
std::cout << "Msg: " << message.data() << std::endl << std::endl;

}

// Received message count.
int message_count_{0};
// Last ignition message received.
ignition::msgs::StringMsg last_message_{};
// Mutex to synchronize state read/write operations.
mutable std::mutex mutex_{};
// Ignition transport node for subscription.
ignition::transport::Node node_{};
};

//////////////////////////////////////////////////
int main(int argc, char **argv)
{
const std::string topic = "/foo";

// During every loop a Monitor object is created by passing the `topic` to be subscribed to.
// After this, the thread is slept for a certain time, allowing to the callback method to be called several times.
//
// When each iteration finishes, the `ign_monitor` object is destroyed, expecting that the callback method won't be called after
// that, given that it would lead to an undefined behaviour(most likely seg fault).
unsigned int iterations{1};
while(true){
std::cout << "Iteration number: "<< iterations << std::endl;
Monitor ign_monitor{topic};
std::this_thread::sleep_for(std::chrono::milliseconds(1000));
iterations++;
}
return 0;
}
93 changes: 93 additions & 0 deletions example/subscriber_bug_workaround_example.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@

/*
* This example tries to show a workaround for the bug presented in `subscriber_bug_example`.
* The idea is to guarantee the that the instance of the class that holds the cb method is alive until
* the end of the execution.
*
* Execute this after that the publisher is started: `publisher_bug_example`.
*/

#include <iostream>
#include <memory>
#include <string>
#include <ignition/msgs.hh>
#include <ignition/transport.hh>

// Subscribes to a topic and prints the value of the message when the callback method is called.
class Monitor {
public:
// Creates an Monitor.
// @param topic_name Valid ignition transport topic name.
// @returns An instance of Monitor wrapped by std::shared_ptr.
friend std::shared_ptr<Monitor> MakeSharedMonitor(const std::string& topic_name);

~Monitor() {std::cout << "****\n[Monitor] Class Destructor!\n****\n" << std::endl;}

private:

// Constructs a Monitor object.
// @param topic_name Valid ignition transport topic name.
explicit Monitor(const std::string& topic_name) : topic_name_(topic_name) {}

// Subscribes to the ignition transport topic.
// @param A pointer to this instance.
void Initialize(std::shared_ptr<Monitor> self_ptr) {
// A std::shared_ptr to `this` is passed to a instance variable
// in order to guarantee that the Monitor instance hasn't been destroyed at
// the moment that the callback method is executed.
self_ptr_ = self_ptr;
node_.Subscribe(topic_name_, &Monitor::OnTopicMessage, this);
}

// Ignition subscriber callback, updating state.
//
// @param message Message received.
void OnTopicMessage(const ignition::msgs::StringMsg& message) {
std::cout << "[Callback]" << std::endl;
std::lock_guard<std::mutex> guard(mutex_);
last_message_ = message;
message_count_++;
std::cout << "Msg: " << message.data() << std::endl << std::endl;

}

// Topic name.
const std::string topic_name_;
// Holds a pointer to this instance.
// This will cause that the instance of the class won't be deallocated by the SO
// until the execution of the unit is finished.
std::shared_ptr<Monitor> self_ptr_{nullptr};
// Received message count.
int message_count_{0};
// Last ignition message received.
ignition::msgs::StringMsg last_message_{};
// Mutex to synchronize state read/write operations.
mutable std::mutex mutex_{};
// Ignition transport node for subscription.
ignition::transport::Node node_{};
};

std::shared_ptr<Monitor> MakeSharedMonitor(const std::string& topic_name){
auto monitor = std::shared_ptr<Monitor>(new Monitor{topic_name});
monitor->Initialize(monitor);
return monitor;
}

//////////////////////////////////////////////////
int main(int argc, char **argv)
{
const std::string topic = "/foo";

// During every loop a Monitor object is created by passing the `topic` to be subscribed to.
// After this, the thread is slept for a certain time, allowing to the callback method to be called several times.
//
// When each iteration finishes, the `monitor` object isn't destroyed because it holds a shared_ptr to itself.
// This guarantee that the callback methods won't show an undefined behavior.
unsigned int iterations{1};
for (int i = 1 ; i <= 20 ; ++i ){
std::cout << "Iteration number: "<< i << std::endl;
auto monitor = MakeSharedMonitor(topic);
std::this_thread::sleep_for(std::chrono::milliseconds(1000));
}
return 0;
}

0 comments on commit 9d5427f

Please sign in to comment.