-
Notifications
You must be signed in to change notification settings - Fork 20
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
Changes from 14 commits
055e9bc
63d95fa
1a24b3b
194bb98
2b5e2f3
2507a99
149c06d
2b1c3e0
a64cfc1
80ac5d4
c3aaf0a
350cb23
0ff47fe
22079d2
86e1bcb
b8477a2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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> | ||
|
||
|
@@ -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_)); | ||
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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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")); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you define a dedicated exception for these? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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() | ||
{ | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. without |
||
sleep(2); | ||
} | ||
} | ||
}; | ||
|
||
|
@@ -1232,6 +1259,7 @@ int main(int argc, char *argv[]) | |
} | ||
catch (std::string e) | ||
{ | ||
return -1; | ||
} | ||
|
||
return 0; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use the initializer list.