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.

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

Fix subprocess handling #48

Merged
merged 16 commits into from
Mar 7, 2019
110 changes: 69 additions & 41 deletions src/ypspur_ros.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@
#include <tf/transform_listener.h>

#include <signal.h>
#include <string.h>
#include <sys/ipc.h>
#include <sys/msg.h>
#include <sys/types.h>
#include <sys/wait.h>

Expand Down Expand Up @@ -625,55 +628,78 @@ class YpspurRosNode
{
if (i > 0 || YP::YPSpur_initex(key_) < 0)
{
ROS_WARN("launching ypspur-coordinator");
pid_ = fork();
if (pid_ == 0)
std::vector<std::string> args;
args.push_back(ypspur_bin_);
args.push_back(std::string("-d"));
args.push_back(port_);
args.push_back(std::string("--admask"));
args.push_back(ad_mask);
args.push_back(std::string("--msq-key"));
args.push_back(std::to_string(key_));
Copy link
Member

Choose a reason for hiding this comment

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

use the initializer list.

if (digital_input_enable_)
args.push_back(std::string("--enable-get-digital-io"));
if (simulate_)
args.push_back(std::string("--without-device"));
if (param_file_.size() > 0)
{
std::vector<std::string> args;
Copy link
Member Author

Choose a reason for hiding this comment

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

Doing string processing after fork causes a kind of dead lock on strict environments like Alpine.

args.push_back(ypspur_bin_);
args.push_back(std::string("-d"));
args.push_back(port_);
args.push_back(std::string("--admask"));
args.push_back(ad_mask);
args.push_back(std::string("--msq-key"));
args.push_back(std::to_string(key_));
if (digital_input_enable_)
args.push_back(std::string("--enable-get-digital-io"));
if (simulate_)
args.push_back(std::string("--without-device"));
if (param_file_.size() > 0)
{
args.push_back(std::string("-p"));
args.push_back(param_file_);
}
args.push_back(std::string("-p"));
args.push_back(param_file_);
}

char **argv = new char *[args.size() + 1];
Copy link
Member

Choose a reason for hiding this comment

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

Where is it released?

for (unsigned int i = 0; i < args.size(); i++)
{
argv[i] = new char[args[i].size() + 1];
memcpy(argv[i], args[i].c_str(), args[i].size());
argv[i][args[i].size()] = 0;
}
argv[args.size()] = nullptr;

const char **argv = new const char *[args.size() + 1];
for (unsigned int i = 0; i < args.size(); i++)
argv[i] = args[i].c_str();
argv[args.size()] = nullptr;
int msq = msgget(key_, 0666 | IPC_CREAT);
msgctl(msq, IPC_RMID, nullptr);

execvp(ypspur_bin_.c_str(), const_cast<char **>(argv));
ROS_WARN("launching ypspur-coordinator");
pid_ = fork();
if (pid_ == -1)
{
const int err = errno;
ROS_ERROR("failed to fork process: %s", strerror(err));
throw(std::string("failed to fork process"));
Copy link
Member

Choose a reason for hiding this comment

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

Can you define a dedicated exception for these?
std::string could throw an exception during use.

Copy link
Member Author

Choose a reason for hiding this comment

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

Many other part has same problem. I will fix them in separate PR. #51

Copy link
Member

Choose a reason for hiding this comment

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

fair enough.

}
else if (pid_ == 0)
{
execvp(ypspur_bin_.c_str(), argv);
ROS_ERROR("failed to start ypspur-coordinator");
throw(std::string("failed to start ypspur-coordinator"));
}
sleep(2);
if (YP::YPSpur_initex(key_) < 0)
for (int i = 4; i >= 0; i--)
{
ROS_ERROR("failed to init libypspur");
throw(std::string("failed to init libypspur"));
int status;
if (waitpid(pid_, &status, WNOHANG) == pid_)
{
ROS_ERROR("ypspur-coordinator dead immediately");
throw(std::string("ypspur-coordinator dead immediately"));
}
else if (i == 0)
{
ROS_ERROR("failed to init libypspur");
throw(std::string("failed to init libypspur"));
}
ros::WallDuration(1).sleep();
if (YP::YPSpur_initex(key_) >= 0)
break;
}
}
double test_v, test_w;
double ret;
boost::atomic<bool> done(false);
auto get_vel_thread = [&test_v, &test_w, &ret, &done]
auto get_vel_thread = [&ret, &done]
{
double test_v, test_w;
ret = YP::YPSpur_get_vel(&test_v, &test_w);
done = true;
};
boost::thread spur_test = boost::thread(get_vel_thread);
boost::chrono::milliseconds span(100);
boost::this_thread::sleep_for(span);
ros::WallDuration(0.1).sleep();
if (!done)
{
// There is no way to kill thread safely in C++11
Expand Down Expand Up @@ -724,6 +750,15 @@ class YpspurRosNode
}
~YpspurRosNode()
{
if (pid_ > 0)
{
ROS_INFO("killing ypspur-coordinator (%d)", (int)pid_);
kill(pid_, SIGINT);
int status;
waitpid(pid_, &status, WNOHANG);
ROS_INFO("ypspur-coordinator is killed (status: %d)", status);
}
ros::shutdown();
}
void spin()
{
Expand Down Expand Up @@ -1210,14 +1245,6 @@ class YpspurRosNode
}
}
ROS_INFO("ypspur_ros main loop terminated");
ros::shutdown();
ros::spin();
if (pid_ > 0)
{
ROS_INFO("killing ypspur-coordinator (%d)", (int)pid_);
kill(pid_, SIGINT);
Copy link
Member Author

Choose a reason for hiding this comment

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

without wait, child process goes zombie.

sleep(2);
}
}
};

Expand All @@ -1232,6 +1259,7 @@ int main(int argc, char *argv[])
}
catch (std::string e)
{
return -1;
}

return 0;
Expand Down