Alexey Serbin has posted comments on this change. Change subject: [util] added Subprocess::GetExitStatus() ......................................................................
Patch Set 5: (2 comments) http://gerrit.cloudera.org:8080/#/c/4648/5/src/kudu/util/subprocess.cc File src/kudu/util/subprocess.cc: PS5, Line 349: // Block all signals for a while: if a signal delivered to the forked : // child before execv(), the parent's signal handler will be executed. : // This is not the expected behavior here. Also, some signal handlers of the : // parent is not async-signal safe (e.g., glog-related handlers). : // In the child's code, the signal handlers are reset to defaults except for : // SIGPIPE, which is ignored. It's a duplicated work since execv() resets : // the handlers as well, but at least it helps to get rid of unexpected : > http://stackoverflow.com/questions/17073877/making-parent-process-wait-till Good find -- for some reason I didn't come up with that solution myself. The only thing to make sure that the signal handlers are already reset when those descriptor are closed. But anyway, that's better than playing with signal masks. OK, let's try to adopt this trick. PS5, Line 349: // Block all signals for a while: if a signal delivered to the forked : // child before execv(), the parent's signal handler will be executed. : // This is not the expected behavior here. Also, some signal handlers of the : // parent is not async-signal safe (e.g., glog-related handlers). : // In the child's code, the signal handlers are reset to defaults except for : // SIGPIPE, which is ignored. It's a duplicated work since execv() resets : // the handlers as well, but at least it helps to get rid of unexpected : > In the case that the original signal was sent to the parent, then the child The latter: the original signal is sent to the child. The fork() call returned success, the child process has started, but until execve() is executed, the child executes parent's signal handler. Right -- here the test sends the signal to the _new_ pid. With this test, the thing can be seen even in debug/release builds, not only TSAN (but in TSAN it leads to an warning which results to error in running the test). -- To view, visit http://gerrit.cloudera.org:8080/4648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic2b16e2a2a53a01982f816b9ee41cc61fd93d4bf Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dinesh Bhat <din...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
