[JENKINS-37566] - Cleanup issues in Command#createdAt handling

jenkinsci · Nov 21, 2017 · 00c2373 · 00c2373
1 parent 2d5624c
commit 00c2373
Show file tree

Hide file tree

Showing 7 changed files with 48 additions and 16 deletions.
diff --git a/src/main/java/hudson/remoting/Channel.java b/src/main/java/hudson/remoting/Channel.java
@@ -743,7 +743,7 @@ public <T> T export(Class<T> type, T instance) {
         return exportedObjects.type(oid);
     }
 
-    /*package*/ void unexport(int id, Throwable cause) {
+    /*package*/ void unexport(int id, @CheckForNull Throwable cause) {
         unexport(id, cause, true);
     }
 
@@ -753,7 +753,7 @@ public <T> T export(Class<T> type, T instance) {
      * @param cause Stacktrace pf the object creation call
      * @param severeErrorIfMissing Consider missing object as {@code SEVERE} error. {@code FINE} otherwise
      */
-    /*package*/ void unexport(int id, Throwable cause, boolean severeErrorIfMissing) {
+    /*package*/ void unexport(int id, @CheckForNull Throwable cause, boolean severeErrorIfMissing) {
         exportedObjects.unexportByOid(id, cause, severeErrorIfMissing);
     }
 
@@ -1244,9 +1244,8 @@ public String toString() {
      * where the termination was initiated as a nested exception.
      */
     private static final class OrderlyShutdown extends IOException {
-        private OrderlyShutdown(Throwable cause) {
-            super(cause.getMessage());
-            initCause(cause);
+        private OrderlyShutdown(@CheckForNull  Throwable cause) {
+            super(cause);
         }
         private static final long serialVersionUID = 1L;
     }

diff --git a/src/main/java/hudson/remoting/Command.java b/src/main/java/hudson/remoting/Command.java
@@ -45,8 +45,10 @@
 abstract class Command implements Serializable {
     /**
      * This exception captures the stack trace of where the Command object is created.
-     * This is useful for diagnosing the error when command fails to execute on the remote peer. 
+     * This is useful for diagnosing the error when command fails to execute on the remote peer.
+     * {@code null} if the cause is not recorded.
      */
+    @CheckForNull
     public final Exception createdAt;
 
 
@@ -81,6 +83,17 @@ protected Command(boolean recordCreatedAt) {
      * @throws ExecutionException Execution error
      */
     protected abstract void execute(Channel channel) throws ExecutionException;
+
+    /**
+     * Chains the {@link #createdAt} cause.
+     * It will happen if and only if cause recording is enabled.
+     * @param initCause Original Cause. {@code null} causes will be ignored
+     */
+    protected final void chainCause(@CheckForNull Throwable initCause) {
+        if (createdAt != null && initCause != null) {
+            createdAt.initCause(initCause);
+        }
+    }
 
     void writeTo(Channel channel, ObjectOutputStream oos) throws IOException {
         Channel old = Channel.setCurrent(channel);

diff --git a/src/main/java/hudson/remoting/PipeWindow.java b/src/main/java/hudson/remoting/PipeWindow.java
@@ -24,6 +24,9 @@
 package hudson.remoting;
 
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
+import javax.annotation.CheckForNull;
+import javax.annotation.Nonnull;
 import java.io.IOException;
 import java.io.OutputStream;
 import java.util.logging.Logger;
@@ -49,6 +52,12 @@
  * @author Kohsuke Kawaguchi
  */
 abstract class PipeWindow {
+
+    /**
+     * Cause of death.
+     * If not {@code null}, new commands will not be executed.
+     */
+    @CheckForNull
     protected volatile Throwable dead;
 
     /**
@@ -95,19 +104,23 @@ abstract class PipeWindow {
 
     /**
      * Indicates that the remote end has died and all the further send attempt should fail.
+     * @param cause Death cause. If {@code null}, the death will be still considered as dead, but there will be no cause recorded..
      */
-    void dead(Throwable cause) {
-        this.dead = cause;
+    void dead(@CheckForNull Throwable cause) {
+        // We need to record
+        this.dead = cause != null ? cause : new RemotingSystemException("Unknown cause", null) ;
     }
 
     /**
      * If we already know that the remote end had developed a problem, throw an exception.
      * Otherwise no-op.
+     * @throws IOException Pipe is already closed
      */
     protected void checkDeath() throws IOException {
-        if (dead!=null)
+        if (dead != null) {
             // the remote end failed to write.
-            throw (IOException)new IOException("Pipe is already closed").initCause(dead);
+            throw new IOException("Pipe is already closed", dead);
+        }
     }
 
 

diff --git a/src/main/java/hudson/remoting/ProxyOutputStream.java b/src/main/java/hudson/remoting/ProxyOutputStream.java
@@ -443,7 +443,7 @@ private NotifyDeadWriter(Channel channel,Throwable cause, int oid) {
         @Override
         protected void execute(Channel channel) {
             PipeWindow w = channel.getPipeWindow(oid);
-            w.dead(createdAt.getCause());
+            w.dead(createdAt != null ? createdAt.getCause() : null);
         }
 
         public String toString() {

diff --git a/src/main/java/hudson/remoting/ProxyWriter.java b/src/main/java/hudson/remoting/ProxyWriter.java
@@ -474,7 +474,7 @@ private NotifyDeadWriter(Channel channel,Throwable cause, int oid) {
         @Override
         protected void execute(Channel channel) {
             PipeWindow w = channel.getPipeWindow(oid);
-            w.dead(createdAt.getCause());
+            w.dead(createdAt != null ? createdAt.getCause() : null);
         }
 
         public String toString() {

diff --git a/src/main/java/hudson/remoting/Request.java b/src/main/java/hudson/remoting/Request.java
@@ -364,8 +364,9 @@ public void run() {
                     } finally {
                         CURRENT.set(null);
                     }
-                    if(chainCause)
-                        rsp.createdAt.initCause(createdAt);
+                    if(chainCause) {
+                        rsp.chainCause(createdAt);
+                    }
 
                     channel.send(rsp);
                 } catch (IOException e) {

diff --git a/src/main/java/hudson/remoting/UnexportCommand.java b/src/main/java/hudson/remoting/UnexportCommand.java
@@ -23,18 +23,24 @@
  */
 package hudson.remoting;
 
+import javax.annotation.CheckForNull;
+
 /**
  * {@link Command} that unexports an object.
  * @author Kohsuke Kawaguchi
  */
 public class UnexportCommand extends Command {
     private final int oid;
 
-    UnexportCommand(int oid, Throwable cause) {
+    UnexportCommand(int oid, @CheckForNull Throwable cause) {
         this.oid = oid;
-        this.createdAt.initCause(cause);
+        chainCause(cause);
     }
 
+    /**
+     * @deprecated Use {@link #UnexportCommand(int, Throwable)}
+     */
+    @Deprecated
     public UnexportCommand(int oid) {
         this(oid,null);
     }