* Terminate build hooks and substitutes with a TERM signal, not a KILL

  signal.  This is necessary because those processes may have joined
  the BDB environment, so they have to be given a chance to clean up.
  (NIX-85)

3 files changed, 27 insertions, 10 deletions

diff --git a/src/libstore/build.cc b/src/libstore/build.cc
index 368d5751f..90ebeaa79 100644
--- a/src/libstore/build.cc
+++ b/src/libstore/build.cc
@@ -628,7 +628,7 @@ private:
     HookReply tryBuildHook();
 
     /* Synchronously wait for a build hook to finish. */
-    void terminateBuildHook();
+    void terminateBuildHook(bool kill = false);
 
     /* Acquires locks on the output paths and gathers information
        about the build (e.g., the input closures).  During this
@@ -991,6 +991,7 @@ static string readLine(int fd)
 {
     string s;
     while (1) {
+        checkInterrupt();
         char ch;
         ssize_t rd = read(fd, &ch, 1);
         if (rd == -1) {
@@ -1018,6 +1019,7 @@ static void drain(int fd)
 {
     unsigned char buffer[1024];
     while (1) {
+        checkInterrupt();
         ssize_t rd = read(fd, buffer, sizeof buffer);
         if (rd == -1) {
             if (errno != EINTR)
@@ -1124,6 +1126,7 @@ DerivationGoal::HookReply DerivationGoal::tryBuildHook()
     
     /* parent */
     pid.setSeparatePG(true);
+    pid.setKillSignal(SIGTERM);
     logPipe.writeSide.close();
     worker.childStarted(shared_from_this(),
         pid, singleton<set<int> >(logPipe.readSide), false);
@@ -1139,7 +1142,7 @@ DerivationGoal::HookReply DerivationGoal::tryBuildHook()
     try {
         reply = readLine(fromHook.readSide);
     } catch (Error & e) {
-        drain(logPipe.readSide);
+        terminateBuildHook(true);
         throw;
     }
 
@@ -1147,7 +1150,6 @@ DerivationGoal::HookReply DerivationGoal::tryBuildHook()
 
     if (reply == "decline" || reply == "postpone") {
         /* Clean up the child.  !!! hacky / should verify */
-        drain(logPipe.readSide);
         terminateBuildHook();
         return reply == "decline" ? rpDecline : rpPostpone;
     }
@@ -1215,18 +1217,22 @@ DerivationGoal::HookReply DerivationGoal::tryBuildHook()
 }
 
 
-void DerivationGoal::terminateBuildHook()
+void DerivationGoal::terminateBuildHook(bool kill)
 {
     /* !!! drain stdout of hook */
     debug("terminating build hook");
     pid_t savedPid = pid;
-    pid.wait(true);
+    if (kill)
+        pid.kill();
+    else
+        pid.wait(true);
     /* `false' means don't wake up waiting goals, since we want to
-       keep this build slot ourselves (at least if the hook reply XXX. */
+       keep this build slot ourselves. */
     worker.childTerminated(savedPid, false);
     fromHook.readSide.close();
     toHook.writeSide.close();
     fdLogFile.close();
+    drain(logPipe.readSide);
     logPipe.readSide.close();
     deleteTmpDir(true); /* get rid of the hook's temporary directory */
 }
@@ -2048,6 +2054,7 @@ void SubstitutionGoal::tryToRun()
     
     /* parent */
     pid.setSeparatePG(true);
+    pid.setKillSignal(SIGTERM);
     logPipe.writeSide.close();
     worker.childStarted(shared_from_this(),
         pid, singleton<set<int> >(logPipe.readSide), true);
diff --git a/src/libutil/util.cc b/src/libutil/util.cc
index 1576e1e8b..ae5af2492 100644
--- a/src/libutil/util.cc
+++ b/src/libutil/util.cc
@@ -641,6 +641,7 @@ Pid::Pid()
 {
     pid = -1;
     separatePG = false;
+    killSignal = SIGKILL;
 }
 
 
@@ -654,6 +655,7 @@ void Pid::operator =(pid_t pid)
 {
     if (this->pid != pid) kill();
     this->pid = pid;
+    killSignal = SIGKILL; // reset signal to default
 }
 
 
@@ -669,10 +671,10 @@ void Pid::kill()
     
     printMsg(lvlError, format("killing process %1%") % pid);
 
-    /* Send a KILL signal to the child.  If it has its own process
-       group, send the signal to every process in the child process
-       group (which hopefully includes *all* its children). */
-    if (::kill(separatePG ? -pid : pid, SIGKILL) != 0)
+    /* Send the requested signal to the child.  If it has its own
+       process group, send the signal to every process in the child
+       process group (which hopefully includes *all* its children). */
+    if (::kill(separatePG ? -pid : pid, killSignal) != 0)
         printMsg(lvlError, (SysError(format("killing process %1%") % pid).msg()));
 
     /* Wait until the child dies, disregarding the exit status. */
@@ -710,6 +712,12 @@ void Pid::setSeparatePG(bool separatePG)
 }
 
 
+void Pid::setKillSignal(int signal)
+{
+    this->killSignal = signal;
+}
+
+
 void killUser(uid_t uid)
 {
     debug(format("killing all processes running under uid `%1%'") % uid);
diff --git a/src/libutil/util.hh b/src/libutil/util.hh
index 3c4629957..4d284ccfd 100644
--- a/src/libutil/util.hh
+++ b/src/libutil/util.hh
@@ -213,6 +213,7 @@ class Pid
 {
     pid_t pid;
     bool separatePG;
+    int killSignal;
 public:
     Pid();
     ~Pid();
@@ -221,6 +222,7 @@ public:
     void kill();
     int wait(bool block);
     void setSeparatePG(bool separatePG);
+    void setKillSignal(int signal);
 };