Better fix to [JENKINS-25519] -- avoid errors when trying to read exitStatus from durable task before finished writing by svanoort · Pull Request #66 · jenkinsci/durable-task-plugin

svanoort · 2018-03-03T23:59:33Z

Superior version of #65

Proposed solution to error like this, encountered when statusCode file exists but is empty, potentially when created but not written to yet, or not written fully. Was confirmed that previous version removed the errors with the "corrupt" status file content and greatly reduced failure rates.

java.lang.NumberFormatException: For input string: ""
at java.lang.NumberFormatException.forInputString(NumberFormatException.java:65)
at java.lang.Integer.parseInt(Integer.java:592)
at java.lang.Integer.parseInt(Integer.java:615)
at org.jenkinsci.plugins.durabletask.FileMonitoringTask$FileMonitoringController.exitStatus(FileMonitoringTask.java:168)
Caused: java.io.IOException: corrupted content in $SOMEPLACE
at org.jenkinsci.plugins.durabletask.FileMonitoringTask$FileMonitoringController.exitStatus(FileMonitoringTask.java:170)
at org.jenkinsci.plugins.durabletask.BourneShellScript$ShellController.exitStatus(BourneShellScript.java:211)

Variant of JENKINS-25519

abayer

LGTM

svanoort · 2018-03-05T17:27:46Z

@reviewbybees

jglick · 2018-03-05T22:58:44Z

src/main/java/org/jenkinsci/plugins/durabletask/BourneShellScript.java

        FilePath logFile = c.getLogFile(ws);
        FilePath resultFile = c.getResultFile(ws);
+        if (resultFile.exists()) {
+            resultFile.delete();  // Maybe overly cautious, but better safe than sorry, similarly we should make sure no duplicate logfile?


Well, the whole control directory should not exist before we start, or should be empty.

jglick · 2018-03-05T22:59:21Z

src/main/java/org/jenkinsci/plugins/durabletask/BourneShellScript.java

            return controlDir(ws).child("pid");
        }

+        // TODO run as one big MasterToSlaveCallable<Integer> to avoid extra network roundtrips


Irrelevant after #60.

Assuming #60 is fundamentally reliable. We can always remove the comment if it tests clean. And we may have to do yet another emergency fix due to something introduced in #49 before that gets integrated.

jglick · 2018-03-05T22:59:32Z

src/main/java/org/jenkinsci/plugins/durabletask/BourneShellScript.java

                            listener.getLogger().println("still have " + pidFile + " so heartbeat checks unreliable; process may or may not be alive");
                        } else {
-                            listener.getLogger().println("wrapper script does not seem to be touching the log file in " + controlDir);
+                            listener.getLogger().println("s " + controlDir);


Erm, I have no idea where that came from. Best guess It was a hotkeying error somewhere, but fixing now.

jglick · 2018-03-05T23:00:16Z

src/main/java/org/jenkinsci/plugins/durabletask/FileMonitoringTask.java

-                } 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> {


This is just gratuitous merge conflict creation against #60. Better to keep the patch short and to the point.

We have one particular user who has longer latencies and this change likely helps keep them under the timeouts - thus it is hardly "gratuitous."

jglick · 2018-03-05T23:01:24Z

src/main/java/org/jenkinsci/plugins/durabletask/FileMonitoringTask.java

+            public Integer invoke(File f, VirtualChannel channel) throws IOException, InterruptedException {
+                if (f.exists() && f.length() > 0) {
+                    try {
+                        String fileString = Files.readFirstLine(f, Charset.defaultCharset());


Anyway you could just get rid of the exists check and use a single readToString call, catching FileNotFoundException and ignoring.

If it's all agent-local then it doesn't really matter, does it? Also: as a rule of thumb in Java one should not be throwing Exceptions to signal routine and expected conditions. They come with a higher than normal level of baggage.

As opposed to Python where that's the idiomatic way to signal things like an Iterator being done.

Well, packaging all of this up in a Remoting call is a pretty high level of baggage, which would swamp any tiny overhead of Throwable.fillInStackTrace in this case.

jglick · 2018-03-05T23:02:26Z

src/main/java/org/jenkinsci/plugins/durabletask/FileMonitoringTask.java

            }
        }

+        static final StatusCheck STATUS_CHECK_INSTANCE = new StatusCheck();


You know act is doing object allocation anyway, right?

jglick · 2018-03-05T23:02:46Z

src/main/java/org/jenkinsci/plugins/durabletask/WindowsBatchScript.java

+        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",


And PowerShell?

Powershell is caught by the 0-length check -- I have no idea how we'd even begin applying this to it and was planning to ask @gabloe or @jtnord because they know the Windows side of things.

Can at least still write a Batch script though, so I included that.

jglick · 2018-03-06T14:39:33Z

src/main/java/org/jenkinsci/plugins/durabletask/FileMonitoringTask.java

+            public Integer invoke(File f, VirtualChannel channel) throws IOException, InterruptedException {
+                if (f.exists() && f.length() > 0) {
+                    try {
+                        String fileString = Files.readFirstLine(f, Charset.defaultCharset());


Minor note: use of defaultCharset could theoretically cause issues on z/OS. Since we are expecting this file to be ASCII (it should in fact just contain [0-9]+), it is safer and clearer to use StandardCharsets.ASCII.

A really critical point - it looks like this is already broken, per #28 -- I think we'd need to address it in more comprehensive work to get it to play nicely in any case.

…able hang bugs on windows

svanoort added 3 commits March 3, 2018 18:31

Basic linux fix to do atomic write of exitStatus + 0-length check

fce0a64

Attempt atomic write for Batch version

3e5c724

Use a FileCallable for exit status check to avoid roundtrips

031732c

svanoort added the work-in-progress label Mar 3, 2018

svanoort added 2 commits March 4, 2018 08:59

Fix small oopsy

72ab895

Fix trimming of StatusCheck

b90d99e

abayer approved these changes Mar 5, 2018

View reviewed changes

Remove needless TODO

664a6ae

svanoort removed the work-in-progress label Mar 5, 2018

svanoort requested a review from jglick March 5, 2018 17:27

svanoort mentioned this pull request Mar 5, 2018

[JENKINS-25519] If unable to read exitStatus file then wait briefly and retry one more time to allow write to complete #65

Closed

jglick reviewed Mar 5, 2018

View reviewed changes

svanoort added 2 commits March 5, 2018 18:29

Fix one oopsy

989f290

Maybe do not be as cautious

8660ea5

jglick approved these changes Mar 6, 2018

View reviewed changes

svanoort merged commit 21f570e into jenkinsci:master Mar 6, 2018

svanoort deleted the better-fix-to-exitStatus branch March 6, 2018 14:48

jglick added a commit to jglick/durable-task-plugin that referenced this pull request Mar 6, 2018

Merge with jenkinsci#66.

814047d

jglick added a commit to jglick/durable-task-plugin that referenced this pull request Mar 6, 2018

Merged with jenkinsci#66.

761202e

svanoort added a commit to svanoort/durable-task-plugin that referenced this pull request Mar 8, 2018

Fully back out the changes from jenkinsci#66 that introduced inexplic…

8513d0c

…able hang bugs on windows

svanoort mentioned this pull request Mar 8, 2018

[JENKINS-34150] Fix hung Batch steps #68

Merged

jglick mentioned this pull request Jun 25, 2018

[JENKINS-52165] Controller.watch API #60

Merged

Comments

Conversation

svanoort commented Mar 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abayer left a comment

Choose a reason for hiding this comment

Uh oh!

svanoort commented Mar 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jglick Mar 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

svanoort commented Mar 3, 2018 •

edited

Loading

jglick Mar 6, 2018 •

edited

Loading