Merge pull request #66 from svanoort/better-fix-to-exitStatus

Better fix to [JENKINS-25519] -- avoid errors when trying to read exitStatus from durable task before finished writing
jenkinsci · Mar 6, 2018 · 21f570e · 21f570e
2 parents 5133350 + 8660ea5
commit 21f570e
Show file tree

Hide file tree

Showing 3 changed files with 43 additions and 17 deletions.
diff --git a/src/main/java/org/jenkinsci/plugins/durabletask/BourneShellScript.java b/src/main/java/org/jenkinsci/plugins/durabletask/BourneShellScript.java
@@ -140,7 +140,7 @@ public String getScript() {
         FilePath resultFile = c.getResultFile(ws);
         FilePath controlDir = c.controlDir(ws);
         if (capturingOutput) {
-            cmd = String.format("{ while [ -d '%s' -a \\! -f '%s' ]; do touch '%s'; sleep 3; done } & jsc=%s; %s=$jsc '%s' > '%s' 2> '%s'; echo $? > '%s'; wait",
+            cmd = String.format("{ while [ -d '%s' -a \\! -f '%s' ]; do touch '%s'; sleep 3; done } & jsc=%s; %s=$jsc '%s' > '%s' 2> '%s'; echo $? > '%s.tmp'; mv '%s.tmp' '%s'; wait",
                 controlDir,
                 resultFile,
                 logFile,
@@ -149,17 +149,17 @@ public String getScript() {
                 scriptPath,
                 c.getOutputFile(ws),
                 logFile,
-                resultFile);
+                resultFile, resultFile, resultFile);
         } else {
-            cmd = String.format("{ while [ -d '%s' -a \\! -f '%s' ]; do touch '%s'; sleep 3; done } & jsc=%s; %s=$jsc '%s' > '%s' 2>&1; echo $? > '%s'; wait",
+            cmd = String.format("{ while [ -d '%s' -a \\! -f '%s' ]; do touch '%s'; sleep 3; done } & jsc=%s; %s=$jsc '%s' > '%s' 2>&1; echo $? > '%s.tmp'; mv '%s.tmp' '%s'; wait",
                 controlDir,
                 resultFile,
                 logFile,
                 cookieValue,
                 cookieVariable,
                 scriptPath,
                 logFile,
-                resultFile);
+                resultFile, resultFile, resultFile);
         }
         cmd = cmd.replace("$", "$$"); // escape against EnvVars jobEnv in LocalLauncher.launch
 
@@ -207,6 +207,7 @@ private FilePath pidFile(FilePath ws) throws IOException, InterruptedException {
             return controlDir(ws).child("pid");
         }
 
+        // TODO run as one big MasterToSlaveCallable<Integer> to avoid extra network roundtrips
         @Override public Integer exitStatus(FilePath workspace, Launcher launcher, TaskListener listener) throws IOException, InterruptedException {
             Integer status = super.exitStatus(workspace, launcher, listener);
             if (status != null) {

diff --git a/src/main/java/org/jenkinsci/plugins/durabletask/FileMonitoringTask.java b/src/main/java/org/jenkinsci/plugins/durabletask/FileMonitoringTask.java
@@ -24,6 +24,7 @@
 
 package org.jenkinsci.plugins.durabletask;
 
+import com.google.common.io.Files;
 import hudson.EnvVars;
 import hudson.FilePath;
 import hudson.Launcher;
@@ -40,6 +41,7 @@
 import java.io.OutputStream;
 import java.io.RandomAccessFile;
 import java.io.StringWriter;
+import java.nio.charset.Charset;
 import java.util.Collections;
 import java.util.Map;
 import java.util.TreeMap;
@@ -48,6 +50,9 @@
 import java.util.logging.Logger;
 import jenkins.MasterToSlaveFileCallable;
 import org.apache.commons.io.IOUtils;
+import org.jenkinsci.remoting.RoleChecker;
+
+import javax.annotation.CheckForNull;
 
 /**
  * A task which forks some external command and then waits for log and status files to be updated/created.
@@ -160,20 +165,39 @@ private static class WriteLog extends MasterToSlaveFileCallable<Long> {
             }
         }
 
-        // TODO would be more efficient to allow API to consolidate writeLog with exitStatus (save an RPC call)
-        @Override public Integer exitStatus(FilePath workspace, Launcher launcher, TaskListener listener) throws IOException, InterruptedException {
-            FilePath status = getResultFile(workspace);
-            if (status.exists()) {
-                try {
-                    return Integer.parseInt(status.readToString().trim());
-                } catch (NumberFormatException x) {
-                    throw new IOException("corrupted content in " + status + ": " + x, x);
+        /** Avoids excess round-tripping when reading status file. */
+        static class StatusCheck extends MasterToSlaveFileCallable<Integer> {
+            @Override
+            @CheckForNull
+            public Integer invoke(File f, VirtualChannel channel) throws IOException, InterruptedException {
+                if (f.exists() && f.length() > 0) {
+                    try {
+                        String fileString = Files.readFirstLine(f, Charset.defaultCharset());
+                        if (fileString == null || fileString.isEmpty()) {
+                            return null;
+                        } else {
+                            fileString = fileString.trim();
+                            if (fileString.isEmpty()) {
+                                return null;
+                            } else {
+                                return Integer.parseInt(fileString);
+                            }
+                        }
+                    } catch (NumberFormatException x) {
+                        throw new IOException("corrupted content in " + f + ": " + x, x);
+                    }
                 }
-            } else {
                 return null;
             }
         }
 
+        static final StatusCheck STATUS_CHECK_INSTANCE = new StatusCheck();
+
+        @Override public Integer exitStatus(FilePath workspace, Launcher launcher, TaskListener listener) throws IOException, InterruptedException {
+            FilePath status = getResultFile(workspace);
+            return status.act(STATUS_CHECK_INSTANCE);
+        }
+
         @Override public byte[] getOutput(FilePath workspace, Launcher launcher) throws IOException, InterruptedException {
             // TODO could perhaps be more efficient for large files to send a MasterToSlaveFileCallable<byte[]>
             try (InputStream is = getOutputFile(workspace).read()) {

diff --git a/src/main/java/org/jenkinsci/plugins/durabletask/WindowsBatchScript.java b/src/main/java/org/jenkinsci/plugins/durabletask/WindowsBatchScript.java
@@ -60,17 +60,18 @@ public String getScript() {
         BatchController c = new BatchController(ws);
 
         String cmd;
+        String quotedResultFile = quote(c.getResultFile(ws));
         if (capturingOutput) {
-            cmd = String.format("@echo off \r\ncmd /c \"\"%s\"\" > \"%s\" 2> \"%s\"\r\necho %%ERRORLEVEL%% > \"%s\"\r\n",
+            cmd = String.format("@echo off \r\ncmd /c \"\"%s\"\" > \"%s\" 2> \"%s\"\r\necho %%ERRORLEVEL%% > \"%s.tmp\"\r\nmove \"%s.tmp\" \"%s\"\r\n",
                 quote(c.getBatchFile2(ws)),
                 quote(c.getOutputFile(ws)),
                 quote(c.getLogFile(ws)),
-                quote(c.getResultFile(ws)));
+                quotedResultFile, quotedResultFile, quotedResultFile);
         } else {
-            cmd = String.format("@echo off \r\ncmd /c \"\"%s\"\" > \"%s\" 2>&1\r\necho %%ERRORLEVEL%% > \"%s\"\r\n",
+            cmd = String.format("@echo off \r\ncmd /c \"\"%s\"\" > \"%s\" 2>&1\r\necho %%ERRORLEVEL%% > \"%s.tmp\"\r\nmove \"%s.tmp\" \"%s\"\n",
                 quote(c.getBatchFile2(ws)),
                 quote(c.getLogFile(ws)),
-                quote(c.getResultFile(ws)));
+                quotedResultFile, quotedResultFile, quotedResultFile);
         }
         c.getBatchFile1(ws).write(cmd, "UTF-8");
         c.getBatchFile2(ws).write(script, "UTF-8");