Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12739 )
Change subject: [subprocess] use RAW_LOG() in child process ...................................................................... Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/12739/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12739/1//COMMIT_MSG@16 PS1, Line 16: TODO: add a test scenario to simulate the deadlock and make sure : it passes with this patch > Maybe a loop that calls Subprocess::Call() on a non-existent program, and a Yep, it's done: https://gerrit.cloudera.org/#/c/12740/ http://gerrit.cloudera.org:8080/#/c/12739/1/src/kudu/util/signal.cc File src/kudu/util/signal.cc: http://gerrit.cloudera.org:8080/#/c/12739/1/src/kudu/util/signal.cc@43 PS1, Line 43: // We unblock all signal masks since they are inherited. > Maybe note that the use of RAW_LOG is so that the function remains async-si it's almost safe, yep: vsnprintf() might call malloc, it seems. Added the comment. http://gerrit.cloudera.org:8080/#/c/12739/1/src/kudu/util/signal.cc@48 PS1, Line 48: RAW_LOG(FATAL, "sigfillset() failed: [%i]", err); > One of the other nice things that PCHECK/PLOG do is convert errno into a te Right, I specifically omitted that part because of absence of async-signal-safe guaranties for strerror: http://www.man7.org/linux/man-pages/man7/signal-safety.7.html http://gerrit.cloudera.org:8080/#/c/12739/1/src/kudu/util/subprocess.cc File src/kudu/util/subprocess.cc: http://gerrit.cloudera.org:8080/#/c/12739/1/src/kudu/util/subprocess.cc@377 PS1, Line 377: generic > general Done http://gerrit.cloudera.org:8080/#/c/12739/1/src/kudu/util/subprocess.cc@378 PS1, Line 378: In particular, trying : // to log anything via GLOG involves taking a lock on mutex that might be : // copied over from the parent in already locked state, so the child would : // deadlock while doing to log anything using LOG() macro > Nit: reword as "Surprisingly, a call to LOG() locks a mutex that may have b Thanks, that sounds clearer. Done. http://gerrit.cloudera.org:8080/#/c/12739/1/src/kudu/util/subprocess.cc@383 PS1, Line 383: it's not async-signal-safe : // strictly speaking > The async-signal-safe documentation agrees with this, but I don't understan It seems the implementation might call malloc() in some cases. I added the note. http://gerrit.cloudera.org:8080/#/c/12739/1/src/kudu/util/subprocess.cc@385 PS1, Line 385: the > drop 'the' Done -- To view, visit http://gerrit.cloudera.org:8080/12739 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic9dca4ca8b1a6d72c9fc818ea41109c80ace3e39 Gerrit-Change-Number: 12739 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Wed, 13 Mar 2019 05:51:34 +0000 Gerrit-HasComments: Yes
