Document and annotate object Export/Unexport logic

During last releases we have fixed several issues related to the object export/unexport operations (JENKINS-23271, JENKINS-41852). I decided that it makes sense to improve the method documentation just to make Javadocs more explicit.

The change also adds missing documentation of hudson.remoting.ExportTable.unexportLogSize

docs/configuration.md

@@ -82,6 +82,14 @@ These properties require independent configuration on both sides of the channel.
       <code>SocketTimeoutException</code>
       </td>
     </tr>
+    <tr>
+      <td>hudson.remoting.ExportTable.unexportLogSize</td>
+      <td>1024</td>
+      <td>2.40</td>
+      <td>?</td>
+      <td><a href="https://issues.jenkins-ci.org/browse/JENKINS-20707">JENKINS-20707</a></td>
+      <td>Defines number of entries to be stored in the unexport history, which is being analyzed during the invalid object ID analysis.</td>
+    </tr>
     <tr>
       <td>${PROTOCOL_FULLY_QUALIFIED_NAME}.disabled, 
       where PROTOCOL_FULLY_QUALIFIED_NAME equals 

src/main/java/hudson/remoting/Base64.java

@@ -17,6 +17,7 @@
 
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import javax.annotation.CheckForNull;
+import javax.annotation.Nullable;
 
 /**
  * This class provides encode/decode for RFC 2045 Base64 as
@@ -101,8 +102,9 @@ protected static boolean isBase64(char octect) {
      * Encodes hex octects into Base64
      *
      * @param binaryData Array containing binaryData
-     * @return Encoded Base64 array
+     * @return Encoded Base64 array. {@code null} if the input is null
      */
+    @Nullable
     public static String encode(byte[] binaryData) {
 
         if (binaryData == null)

src/main/java/hudson/remoting/Channel.java

@@ -60,6 +60,7 @@
 import java.util.logging.Level;
 import java.util.logging.LogRecord;
 import java.util.logging.Logger;
+import javax.annotation.Nullable;
 import org.kohsuke.accmod.Restricted;
 import org.kohsuke.accmod.restrictions.NoExternalUse;
 
@@ -613,11 +614,13 @@ private ExecutorService createPipeWriterExecutor() {
     /**
      * {@inheritDoc}
      */
+    @Override
     public <T> T export(Class<T> type, T instance) {
         return export(type, instance, true);
     }
 
     /**
+     * Exports the class instance to the remote side.
      * @param userProxy
      *      If true, the returned proxy will be capable to handle classes
      *      defined in the user classloader as parameters and return values.
@@ -626,10 +629,14 @@ private ExecutorService createPipeWriterExecutor() {
      *      used by {@link RemoteClassLoader}.
      *
      *      To create proxies for objects inside remoting, pass in false. 
+     * @return Reference to the exported instance wrapped by {@link RemoteInvocationHandler}. 
+     *      {@code null} if the input instance is {@code null}.     
      */
-    /*package*/ <T> T export(Class<T> type, T instance, boolean userProxy) {
-        if(instance==null)
+    @Nullable
+    /*package*/ <T> T export(Class<T> type, @CheckForNull T instance, boolean userProxy) {
+        if(instance==null) {
             return null;
+        }
 
         // every so often perform GC on the remote system so that
         // unused RemoteInvocationHandler get released, which triggers
@@ -686,8 +693,9 @@ private ExecutorService createPipeWriterExecutor() {
 
     /**
      * Increase reference count so much to effectively prevent de-allocation.
+     * @param instance Instance to be pinned
      */
-    public void pin(Object instance) {
+    public void pin(@Nonnull Object instance) {
         exportedObjects.pin(instance);
     }
 
@@ -1089,7 +1097,7 @@ public void checkRoles(RoleChecker checker) throws SecurityException {
 
     /**
      * Waits for this {@link Channel} to be closed down, but only up the given milliseconds.
-     *
+     * @param timeout TImeout in milliseconds
      * @throws InterruptedException
      *      If the current thread is interrupted while waiting for the completion.
      * @since 1.299

src/main/java/hudson/remoting/ErrorPropagatingOutputStream.java

@@ -3,6 +3,7 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
+import javax.annotation.CheckForNull;
 
 /**
  * {@link OutputStream} that's connected to an {@link InputStream} somewhere,
@@ -39,5 +40,5 @@
      * @param e
      *      if null, this method behaves exactly like {@link OutputStream#close()}
      */
-    void error(Throwable e) throws IOException;
+    void error(@CheckForNull Throwable e) throws IOException;
 }

src/main/java/hudson/remoting/ExportTable.java

@@ -73,14 +73,17 @@
          * {@code object.getClass().getName()} kept around so that we can see the type even after it
          * gets deallocated.
          */
+        @Nonnull
         private final String objectType;
         /**
          * Where was this object first exported?
          */
+        @Nonnull
         final CreatedAt allocationTrace;
         /**
          * Where was this object unexported?
          */
+        @CheckForNull
         ReleasedAt releaseTrace;
         /**
          * Current reference count.
@@ -92,9 +95,10 @@
         /**
          * This field can be set programmatically to track reference counting
          */
+        @CheckForNull
         private ReferenceCountRecorder recorder;
 
-        Entry(T object, Class<? super T>... interfaces) {
+        Entry(@Nonnull T object, Class<? super T>... interfaces) {
             this.id = iota++;
             this.interfaces = interfaces.clone();
             this.object = object;
@@ -122,7 +126,13 @@ void pin() {
                 referenceCount += Integer.MAX_VALUE/2;
         }
 
-        void release(Throwable callSite) {
+        /**
+         * Releases the entry.
+         * @param callSite 
+         *      Optional location that indicates where the actual call site was that triggered the activity,
+         *      in case it was requested from the other side of the channel.
+         */
+        void release(@CheckForNull Throwable callSite) {
             if (recorder!=null)
                 recorder.onRelease(callSite);
 
@@ -204,7 +214,7 @@ synchronized void addInterface(Class<? super T> clazz) {
          *      Optional location that indicates where the actual call site was that triggered the activity,
          *      in case it was requested from the other side of the channel.
          */
-        Source(Throwable callSite) {
+        Source(@CheckForNull Throwable callSite) {
             super(callSite);
             // force the computation of the stack trace in a Java friendly data structure,
             // so that the call stack can be seen from the heap dump after the fact.
@@ -217,16 +227,18 @@ synchronized void addInterface(Class<? super T> clazz) {
             super(null);
         }
 
+        @Override
         public String toString() {
             return "  Created at "+new Date(timestamp);
         }
     }
 
     static class ReleasedAt extends Source {
-        ReleasedAt(Throwable callSite) {
+        ReleasedAt(@CheckForNull Throwable callSite) {
             super(callSite);
         }
 
+        @Override
         public String toString() {
             return "  Released at "+new Date(timestamp);
         }
@@ -288,19 +300,26 @@ boolean isRecording() {
      * @return
      *      The assigned 'object ID'. If the object is already exported,
      *      it will return the ID already assigned to it.
-     * @param clazz
-     * @param t
+     *      {@code 0} if the input parameter is {@code null}.
+     * @param clazz Class of the object
+     * @param t Class instance
      */
-    synchronized <T> int export(Class<T> clazz, T t) {
+    synchronized <T> int export(@Nonnull Class<T> clazz, @CheckForNull T t) {
         return export(clazz, t,true);
     }
 
     /**
-     * @param clazz
+     * Exports the given object.
+     * @param clazz Class of the object
+     * @param t Object to be exported
      * @param notifyListener
      *      If false, listener will not be notified. This is used to
+     * @return
+     *      The assigned 'object ID'. If the object is already exported,
+     *      it will return the ID already assigned to it.
+     *      {@code 0} if the input parameter is {@code null}.
      */
-    synchronized <T> int export(Class<T> clazz, T t, boolean notifyListener) {
+    synchronized <T> int export(@Nonnull Class<T> clazz, @CheckForNull T t, boolean notifyListener) {
         if(t==null)    return 0;   // bootstrap classloader
 
         Entry e = reverse.get(t);
@@ -319,14 +338,21 @@ boolean isRecording() {
         return e.id;
     }
 
-    /*package*/ synchronized void pin(Object t) {
+    /*package*/ synchronized void pin(@Nonnull Object t) {
         Entry e = reverse.get(t);
         if(e!=null)
             e.pin();
     }
 
-    synchronized @Nonnull
-    Object get(int id) throws ExecutionException {
+    /**
+     * Retrieves object by id.
+     * @param id Object ID
+     * @return Object
+     * @throws ExecutionException The requested ID cannot be found.
+     *      The root cause will be diagnosed by {@link #diagnoseInvalidObjectId(int)}.
+     */
+    @Nonnull
+    synchronized Object get(int id) throws ExecutionException {
         Entry e = table.get(id);
         if(e!=null) return e.object;
 
@@ -347,8 +373,8 @@ synchronized Object getOrNull(int oid) {
         return null;
     }
 
-    synchronized @Nonnull
-    Class[] type(int id) throws ExecutionException {
+    @Nonnull
+    synchronized Class[] type(int id) throws ExecutionException {
         Entry e = table.get(id);
         if(e!=null) return e.getInterfaces();
 
@@ -362,8 +388,11 @@ synchronized Object getOrNull(int oid) {
      * Exported {@link Pipe}s are vulnerable to infinite blocking
      * when the channel is lost and the sender side is cut off. The reader
      * end will not see that the writer has disappeared.
+     * 
+     * @param e Termination error
+     * 
      */
-    void abort(Throwable e) {
+    void abort(@CheckForNull Throwable e) {
         List<Entry<?>> values;
         synchronized (this) {
             values = new ArrayList<Entry<?>>(table.values());
@@ -392,6 +421,7 @@ void abort(Throwable e) {
      * @param id Object ID
      * @return Exception to be thrown
      */
+    @Nonnull
     private synchronized ExecutionException diagnoseInvalidObjectId(int id) {
         Exception cause=null;
 
@@ -410,8 +440,10 @@ private synchronized ExecutionException diagnoseInvalidObjectId(int id) {
 
     /**
      * Removes the exported object from the table.
+     * @param t Object to be unexported. {@code null} instances will be ignored.
+     * @param callSite Stacktrace of the invocation source 
      */
-    synchronized void unexport(Object t, Throwable callSite) {
+    synchronized void unexport(@CheckForNull Object t, Throwable callSite) {
         if(t==null)     return;
         Entry e = reverse.get(t);
         if(e==null) {
@@ -431,12 +463,12 @@ void unexportByOid(Integer oid, Throwable callSite) {
 
     /**
      * Removes the exported object for the specified oid from the table.
-     * @param oid Object ID
+     * @param oid Object ID. If {@code null} the method will do nothing.
      * @param callSite Unexport command caller
      * @param severeErrorIfMissing Consider missing object as {@link #SEVERE} error. {@link #FINE} otherwise
      * @since TODO
      */
-    synchronized void unexportByOid(Integer oid, Throwable callSite, boolean severeErrorIfMissing) {
+    synchronized void unexportByOid(@CheckForNull Integer oid, @CheckForNull Throwable callSite, boolean severeErrorIfMissing) {
         if(oid==null)     return;
         Entry e = table.get(oid);
         if(e==null) {
@@ -451,8 +483,9 @@ synchronized void unexportByOid(Integer oid, Throwable callSite, boolean severeE
 
     /**
      * Dumps the contents of the table to a file.
+     * @throws IOException Output error
      */
-    synchronized void dump(PrintWriter w) throws IOException {
+    synchronized void dump(@Nonnull PrintWriter w) throws IOException {
         for (Entry e : table.values()) {
             e.dump(w);
         }
@@ -462,6 +495,10 @@ synchronized void dump(PrintWriter w) throws IOException {
         return reverse.containsKey(o);
     }
 
+    /**
+     * Defines number of entries to be stored in the unexport history.
+     * @since 2.40
+     */
     public static int UNEXPORT_LOG_SIZE = Integer.getInteger(ExportTable.class.getName()+".unexportLogSize",1024);
 
     private static final Logger LOGGER = Logger.getLogger(ExportTable.class.getName());

src/main/java/hudson/remoting/LocalChannel.java

@@ -91,6 +91,7 @@ public void join() throws InterruptedException {
         // noop
     }
 
+    @Override
     public void join(long timeout) throws InterruptedException {
         // noop
     }

src/main/java/hudson/remoting/RemoteInvocationHandler.java

@@ -130,6 +130,7 @@
     /**
      * Wraps an OID to the typed wrapper.
      */
+    @Nonnull
     public static <T> T wrap(Channel channel, int id, Class<T> type, boolean userProxy, boolean autoUnexportByCaller) {
         ClassLoader cl = type.getClassLoader();
         // if the type is a JDK-defined type, classloader should be for IReadResolve

src/main/java/hudson/remoting/VirtualChannel.java

@@ -24,6 +24,9 @@
 package hudson.remoting;
 
 import java.io.IOException;
+import javax.annotation.CheckForNull;
+import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
 
 /**
  * Virtualized {@link Channel} that allows different implementations.
@@ -81,7 +84,7 @@
 
     /**
      * Waits for this {@link Channel} to be closed down, but only up the given milliseconds.
-     *
+     * @param timeout Timeout in milliseconds
      * @throws InterruptedException
      *      If the current thread is interrupted while waiting for the completion.
      * @since 1.300
@@ -91,18 +94,28 @@
     /**
      * Exports an object for remoting to the other {@link Channel}
      * by creating a remotable proxy.
-     *
+     * The returned reference must be kept if there is ongoing operation on the remote side.
+     * Once it is released, the exported object will be deallocated as well.
+     * Please keep in mind that the object may be also released earlier than expected by JVM
+     * (e.g. see <a href="https://issues.jenkins-ci.org/browse/JENKINS-23271">JENKINS-23271</a>).
+     * 
+     * @param instance 
+     *      Instance to be exported.
+     *      {@code null} instances won't be exported to the remote instance.
      * <p>
      * All the parameters and return values must be serializable.
-     *
+     * @param <T> 
+     *      Type
      * @param type
      *      Interface to be remoted.
      * @return
      *      the proxy object that implements <tt>T</tt>. This object can be transfered
      *      to the other {@link Channel}, and calling methods on it from the remote side
      *      will invoke the same method on the given local <tt>instance</tt> object.
+     *      {@code null} if the input instance is {@code null}.  
      */
-    <T> T export( Class<T> type, T instance);
+    @Nullable
+    <T> T export(@Nonnull Class<T> type, @CheckForNull T instance);
 
     /**
      * Blocks until all the I/O packets sent from remote is fully locally executed, then return.