[JENKINS-37566] - FindBugs Cleanup. Part #1 (#109)

* [JENKINS-37566] - FindBugs: Unclosed stream in hudson.remoting.Capability

* [JENKINS-37566] - FindBugs: Stream not closed on Exception path in InitializeJarCacheMain#copyFile

* [JENKINS-37566] - Fix Util#makeResource and remove obsolete hack

* Extra Util#makeResource() polishing

* Deprecate obsolete Util#mkdirs()

* Exceptional case in setLastModifiedTime

* Handle exception case during temp file deletion in FileSystemJarCache

* Synchronize ProxyWriter#closed in write()

* Synchronization of ProxyWriter#channel

* Get rid of the obsolete collection in Channel#properties, fix synchronization

* Checksum#calculateFor() - no need to put if absent since there is a check above

* UWF_UNWRITTEN_FIELD in ExportTable$Entry

* NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE in JnlpAgentEndpointResolver.
Now we use a more agressive check

* UWF_FIELD_NOT_INITIALIZED_IN_CONSTRUCTOR in org.jenkinsci.remoting.nio.NioChannelHub$NioTransport.abort

* UPM_UNCALLED_PRIVATE_METHOD - hudson.remoting.ChunkedOutputStream.frameSize()

* FindBugs: Revert the stream closure at emoting Capability, just a design to be reworked

* Unrealistic NP_NULL_ON_SOME_PATH in NioChannelHub

* EI_EXPOSE_REP in DiagnosedStreamCorruptionException (diagnostics code)

* SE_TRANSIENT_FIELD_NOT_RESTORED in PreloadJarTask, RemoteClassLoader, RemoteInvocationHandler and UserRequest

* Leftover false-positive UG_SYNC_SET_UNSYNC_GET in Channel

* EI_EXPOSE_REP in ChannelBuilder#properties

* Suppress DMI_NONSERIALIZABLE_OBJECT_WRITTEN in ClassLoaderHolder#writeObject()

* Better handling of Streams in hudson.remoting.Capability (still weird)

* Fix the Util#makeResource() behavior for nested folders

* FileSystemJarCache: tmp file may be renamed

* JENKINS-37566 - Modification of Channel#dumpDiagnostics() actually is not required after the merge of #122

* Disable DP_DO_INSIDE_DO_PRIVILEGED as per feeback from @kohsue in #118

* Fix the FindBugs filter

* [NP_NULL_ON_SOME_PATH] - Prevent NPE in ExportTable#diagnoseInvalidObjectId()

* [RV_RETURN_VALUE_IGNORED_BAD_PRACTICE] - Propagate exceptions when FileSystemJarCache#retrieve() fails to retrieve the target

* [DP_CREATE_CLASSLOADER_INSIDE_DO_PRIVILEGED] - Ignore the warning as per discussion in #118

* [VO_VOLATILE_INCREMENT] - Suppress warning in Channel#send()

* FindBugs - Which#jarFile() should not suppress all exceptions

* FindBugs - Suppress URF_UNREAD_FIELD in Pipewindow#Real

* [JENKINS-37566] - Channel#properties should be a ConcurrentHashMap

Reason - prevent possible delays and deadlocks in the getter method.
Addresses the feedback from @stephenc

* [JENKINS-37566] - DiagnosedStreamCorruptionException readAhead/readBack handlers should use a more efficient clone() command

* [JENKINS-37566] - Suppress UG_SYNC_SET_UNSYNC_GET after switching to the ConcurrentHashMap

* [JENKINS-37566] - hudson.remoting.Util should be tolerant against InvalidPathException

* [JENKINS-37566] - make ProxyWriter more asynchronous (follow-up to @stephenc’s review)

* [JENKINS-37566] - Add a comment about race condition as suggested by @stephenc

src/findbugs/excludeFilter.xml

@@ -8,13 +8,19 @@
   </Match>
 
   <Match>
-    <Or>
-      <!--We do not want do break API by converting potentially usefull classes-->
-      <Bug pattern="SIC_INNER_SHOULD_BE_STATIC_ANON"/>
-    </Or>
+    <!--We do not want do break API by converting potentially usefull classes-->
+    <Bug pattern="SIC_INNER_SHOULD_BE_STATIC_ANON"/>
   </Match>
 
-
+  <Match>
+    <!-- TODO: On hold due to the discussion in PR #118 -->
+    <Bug pattern="DP_DO_INSIDE_DO_PRIVILEGED"/>
+  </Match>
+  <Match>
+    <!-- TODO: On hold due to the discussion in PR #118 -->
+    <Bug pattern="DP_CREATE_CLASSLOADER_INSIDE_DO_PRIVILEGED"/>
+  </Match>
+
   <!--TODO: reconsider commented filters after cleanup-->
   <!-- ignore all low-level (priority=2,3) issues except serialization problems -->
   <!--<Match>

src/main/java/hudson/remoting/AbstractByteArrayCommandTransport.java

@@ -6,6 +6,7 @@
 import java.io.ObjectOutputStream;
 import java.util.logging.Level;
 import java.util.logging.Logger;
+import javax.annotation.Nonnull;
 
 /**
  * {@link CommandTransport} that works with {@code byte[]} instead of command object.
@@ -35,7 +36,7 @@ public abstract class AbstractByteArrayCommandTransport extends CommandTransport
      * 
      * In this subtype, we pass in {@link ByteArrayReceiver} that uses byte[] instead of {@link Command}
      */
-    public abstract void setup(ByteArrayReceiver receiver);
+    public abstract void setup(@Nonnull ByteArrayReceiver receiver);
 
     public static interface ByteArrayReceiver {
         /**

src/main/java/hudson/remoting/Capability.java

@@ -1,5 +1,6 @@
 package hudson.remoting;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import hudson.remoting.Channel.Mode;
 import java.io.IOException;
 import java.io.InputStream;
@@ -108,22 +109,30 @@ public boolean supportsProxyWriter2_35() {
         return (mask & MASK_PROXY_WRITER_2_35) != 0;
     }
 
+    //TODO: ideally preamble handling needs to be reworked in order to avoid FB suppression
     /**
      * Writes out the capacity preamble.
      */
     void writePreamble(OutputStream os) throws IOException {
         os.write(PREAMBLE);
-        ObjectOutputStream oos = new ObjectOutputStream(Mode.TEXT.wrap(os));
-        oos.writeObject(this);
-        oos.flush();
+        try (ObjectOutputStream oos = new ObjectOutputStream(Mode.TEXT.wrap(os)) {
+            @Override
+            public void close() throws IOException {
+                flush();
+                // TODO: Cannot invoke the private clear() method, but GC well do it for us. Not worse than the original solution
+                // Here the code does not close the proxied stream OS on completion
+            }
+        }) {
+            oos.writeObject(this);
+            oos.flush();
+        }
     }
 
     /**
      * The opposite operation of {@link #writePreamble(OutputStream)}.
      */
     public static Capability read(InputStream is) throws IOException {
-        try {
-            ObjectInputStream ois = new ObjectInputStream(Mode.TEXT.wrap(is)) {
+        try (ObjectInputStream ois = new ObjectInputStream(Mode.TEXT.wrap(is)) {
                 // during deserialization, only accept Capability to protect ourselves
                 // from malicious payload. Allow java.lang.String so that
                 // future versions of Capability can send more complex data structure.
@@ -136,7 +145,12 @@ protected Class<?> resolveClass(ObjectStreamClass desc) throws IOException, Clas
                         return super.resolveClass(desc);
                     throw new SecurityException("Rejected: "+n);
                 }
-            };
+
+                @Override
+                public void close() throws IOException {
+                    // Do not close the stream since we continue reading from the input stream "is" 
+                }   
+            }) {
             return (Capability)ois.readObject();
         } catch (ClassNotFoundException e) {
             throw (Error)new NoClassDefFoundError(e.getMessage()).initCause(e);

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

@@ -23,6 +23,7 @@
  */
 package hudson.remoting;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import edu.umd.cs.findbugs.annotations.SuppressWarnings;
 import hudson.remoting.CommandTransport.CommandReceiver;
 import hudson.remoting.PipeWindow.Key;
@@ -46,12 +47,14 @@
 import java.lang.ref.WeakReference;
 import java.net.URL;
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.Date;
 import java.util.Hashtable;
 import java.util.Locale;
 import java.util.Map;
 import java.util.Vector;
 import java.util.WeakHashMap;
+import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.TimeUnit;
@@ -291,7 +294,7 @@ public class Channel implements VirtualChannel, IChannel, Closeable {
     /**
      * Property bag that contains application-specific stuff.
      */
-    private final Hashtable<Object,Object> properties = new Hashtable<Object,Object>();
+    private final ConcurrentHashMap<Object,Object> properties = new ConcurrentHashMap<>();
 
     /**
      * Proxy to the remote {@link Channel} object.
@@ -662,6 +665,7 @@ private ExecutorService createPipeWriterExecutor() {
      * This is the lowest layer of abstraction in {@link Channel}.
      * {@link Command}s are executed on a remote system in the order they are sent.
      */
+    @SuppressFBWarnings(value = "VO_VOLATILE_INCREMENT", justification = "The method is synchronized, no other usages. See https://sourceforge.net/p/findbugs/bugs/1032/")
     /*package*/ synchronized void send(Command cmd) throws IOException {
         if(outClosed!=null)
             throw new ChannelClosedException(outClosed);
@@ -1343,8 +1347,7 @@ public void dumpDiagnostics(@Nonnull PrintWriter w) throws IOException {
         w.printf("  Last command sent=%s%n", new Date(lastCommandSentAt));
         w.printf("  Last command received=%s%n", new Date(lastCommandReceivedAt));
 
-        // TODO: Update after the merge to 3.x branch, where the Hashtable is going to be replaced as a part of
-        // https://github.com/jenkinsci/remoting/pull/109
+        // TODO: Synchronize when Hashtable gets replaced by a modern collection.
         w.printf("  Pending calls=%d%n", pendingCalls.size());
     }
 
@@ -1413,6 +1416,7 @@ public void close(@CheckForNull Throwable diagnosis) throws IOException {
         // termination is done by CloseCommand when we received it.
     }
 
+    //TODO: ideally waitForProperty() methods should get rid of the notify-driven implementation
     /**
      * Gets the application specific property set by {@link #setProperty(Object, Object)}.
      * These properties are also accessible from the remote channel via {@link #getRemoteProperty(Object)}.
@@ -1421,7 +1425,13 @@ public void close(@CheckForNull Throwable diagnosis) throws IOException {
      * This mechanism can be used for one side to discover contextual objects created by the other JVM
      * (as opposed to executing {@link Callable}, which cannot have any reference to the context
      * of the remote {@link Channel}.
+     * @param key Key
+     * @return The property or {@code null} if there is no property for the specified key
      */
+    @Override
+    @SuppressFBWarnings(value = "UG_SYNC_SET_UNSYNC_GET", 
+            justification = "setProperty() is synchronized in order to notify waitForProperty() methods" +
+                            "No problem with this method since it is a ConcurrentHashMap")
     public Object getProperty(Object key) {
         return properties.get(key);
     }
@@ -1459,10 +1469,13 @@ public <T> T waitForProperty(ChannelProperty<T> key) throws InterruptedException
 
     /**
      * Sets the property value on this side of the channel.
-     * 
+     * @param key Property key
+     * @param value Value to set. {@code null} removes the existing entry without adding a new one.
+     * @return Old property value or {@code null} if it was not set
      * @see #getProperty(Object)
      */
-    public synchronized Object setProperty(Object key, Object value) {
+    @CheckForNull
+    public synchronized Object setProperty(@Nonnull Object key, @CheckForNull Object value) {
         Object old = value!=null ? properties.put(key, value) : properties.remove(key);
         notifyAll();
         return old;

src/main/java/hudson/remoting/ChannelBuilder.java

@@ -21,6 +21,7 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.Hashtable;
 import java.util.List;
 import java.util.Map;
@@ -316,7 +317,7 @@ public <T> ChannelBuilder withProperty(ChannelProperty<T> key, T value) {
      * @since 2.47
      */
     public Map<Object,Object> getProperties() {
-        return properties;
+        return Collections.unmodifiableMap(properties);
     }
 
     /**

src/main/java/hudson/remoting/ChunkedOutputStream.java

@@ -30,10 +30,6 @@ public ChunkedOutputStream(int frameSize, OutputStream base) {
         this.base = base;
     }
 
-    private int frameSize() {
-        return buf.length;
-    }
-
     /**
      * How many more bytes can our buffer take?
      */

src/main/java/hudson/remoting/ClassLoaderHolder.java

@@ -1,5 +1,6 @@
 package hudson.remoting;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import hudson.remoting.RemoteClassLoader.IClassLoader;
 import org.jenkinsci.remoting.SerializableOnlyOverRemoting;
 
@@ -40,6 +41,8 @@ private void readObject(ObjectInputStream ois) throws IOException, ClassNotFound
         classLoader = proxy==null ? null : getChannelForSerialization().importedClassLoaders.get(proxy);
     }
 
+    @SuppressFBWarnings(value = "DMI_NONSERIALIZABLE_OBJECT_WRITTEN", 
+            justification = "RemoteClassLoader.export() produces a serializable wrapper class")
     private void writeObject(ObjectOutputStream oos) throws IOException {
         if (classLoader==null)
             oos.writeObject(null);

src/main/java/hudson/remoting/DiagnosedStreamCorruptionException.java

@@ -1,8 +1,11 @@
 package hudson.remoting;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import java.io.PrintWriter;
 import java.io.StreamCorruptedException;
 import java.io.StringWriter;
+import java.util.Arrays;
+import javax.annotation.Nonnull;
 
 /**
  * Signals a {@link StreamCorruptedException} with some additional diagnostic information.
@@ -11,10 +14,15 @@
  */
 public class DiagnosedStreamCorruptionException extends StreamCorruptedException {
     private final Exception diagnoseFailure;
+
+    @Nonnull
     private final byte[] readBack;
+
+    @Nonnull
     private final byte[] readAhead;
 
-    DiagnosedStreamCorruptionException(Exception cause, Exception diagnoseFailure, byte[] readBack, byte[] readAhead) {
+    DiagnosedStreamCorruptionException(Exception cause, Exception diagnoseFailure, 
+            @Nonnull byte[] readBack, @Nonnull byte[] readAhead) {
         initCause(cause);
         this.diagnoseFailure = diagnoseFailure;
         this.readBack = readBack;
@@ -25,12 +33,14 @@ public Exception getDiagnoseFailure() {
         return diagnoseFailure;
     }
 
+    @Nonnull
     public byte[] getReadBack() {
-        return readBack;
+        return readBack.clone();
     }
-
+
+    @Nonnull
     public byte[] getReadAhead() {
-        return readAhead;
+        return readAhead.clone();
     }
 
     @Override

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

@@ -97,9 +97,12 @@ private final class Entry<T> {
          */
         private int referenceCount;
 
+        //TODO: cleanup this mess?
         /**
-         * This field can be set programmatically to track reference counting
+         * This field can be set programmatically to track reference counting.
+         * Please note that value unset is not thread-safe.
          */
+        @SuppressFBWarnings(value = "UWF_UNWRITTEN_FIELD", justification = "Old System script magic")
         @CheckForNull
         private ReferenceCountRecorder recorder;
 
@@ -460,13 +463,16 @@ private synchronized ExecutionException diagnoseInvalidObjectId(int id) {
         Exception cause=null;
 
         if (!unexportLog.isEmpty()) {
-            for (Entry e : unexportLog) {
+            for (Entry<?> e : unexportLog) {
                 if (e.id==id)
                     cause = new Exception("Object was recently deallocated\n"+Util.indent(e.dump()), e.releaseTrace);
             }
-            if (cause==null)
-                cause = new Exception("Object appears to be deallocated at lease before "+
-                    new Date(unexportLog.get(0).releaseTrace.timestamp));
+            if (cause==null) {
+                // If there is no cause available, create an artificial cause and use the last unexport entry as an estimated release time if possible
+                final ReleasedAt releasedAt = unexportLog.get(0).releaseTrace;
+                final Date releasedBefore = releasedAt != null ? new Date(releasedAt.timestamp) : new Date();
+                cause = new Exception("Object appears to be deallocated at lease before "+ releasedBefore);
+            }
         }
 
         return new ExecutionException("Invalid object ID "+id+" iota="+iota, cause);

src/main/java/hudson/remoting/FileSystemJarCache.java

@@ -6,6 +6,8 @@
 import java.io.FileOutputStream;
 import java.io.IOException;
 import java.net.URL;
+import java.nio.file.Files;
+import java.nio.file.attribute.FileTime;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
@@ -69,9 +71,12 @@ protected URL lookInCache(Channel channel, long sum1, long sum2) throws IOExcept
         File jar = map(sum1, sum2);
         if (jar.exists()) {
             LOGGER.log(Level.FINER, String.format("Jar file cache hit %16X%16X",sum1,sum2));
-            if (touch)  jar.setLastModified(System.currentTimeMillis());
-            if (notified.add(new Checksum(sum1,sum2)))
+            if (touch)  {
+                Files.setLastModifiedTime(jar.toPath(), FileTime.fromMillis(System.currentTimeMillis()));
+            }
+            if (notified.add(new Checksum(sum1,sum2))) {
                 getJarLoader(channel).notifyJarPresence(sum1,sum2);
+            }
             return jar.toURI().toURL();
         }
         return null;
@@ -93,7 +98,7 @@ protected URL retrieve(Channel channel, long sum1, long sum2) throws IOException
                     "Cached file checksum mismatch: %s%nExpected: %s%n Actual: %s",
                     target.getAbsolutePath(), expected, actual
             ));
-            target.delete();
+            Files.delete(target.toPath());
             synchronized (checksumsByPath) {
                 checksumsByPath.remove(target.getCanonicalPath());
             }
@@ -137,7 +142,7 @@ protected URL retrieve(Channel channel, long sum1, long sum2) throws IOException
 
                 return target.toURI().toURL();
             } finally {
-                tmp.delete();
+                Files.deleteIfExists(tmp.toPath());
             }
         } catch (IOException e) {
             throw (IOException)new IOException("Failed to write to "+target).initCause(e);

src/main/java/hudson/remoting/InitializeJarCacheMain.java

@@ -62,23 +62,12 @@ public static void main(String[] argv) throws Exception {
      * our own from scratch.
      */
     private static void copyFile(File src, File dest) throws Exception {
-        FileInputStream input = null;
-        FileOutputStream output = null;
-        try {
-            input = new FileInputStream(src);
-            output = new FileOutputStream(dest);
+        try (FileInputStream input = new FileInputStream(src); FileOutputStream output = new FileOutputStream(dest)) {
             byte[] buf = new byte[1024 * 1024];
             int bytesRead;
             while ((bytesRead = input.read(buf)) > 0) {
                 output.write(buf, 0, bytesRead);
             }
-        } finally {
-            if (input != null) {
-                input.close();
-            }
-            if (output != null) {
-                output.close();
-            }
         }
     }
 }

src/main/java/hudson/remoting/PipeWindow.java

@@ -23,6 +23,7 @@
  */
 package hudson.remoting;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import java.io.IOException;
 import java.io.OutputStream;
 import java.util.logging.Logger;
@@ -155,6 +156,8 @@ public int hashCode() {
         }
     }
 
+    //TODO: Consider rework and cleanup of the fields
+    @SuppressFBWarnings(value = "URF_UNREAD_FIELD", justification = "Legacy implementation")
     static class Real extends PipeWindow {
         private final int initial;
         private int available;