[JENKINS-38696] - Save the progress

jenkinsci · Jul 7, 2017 · 144922e · 144922e
1 parent 6165d6f
commit 144922e
Show file tree

Hide file tree

Showing 11 changed files with 238 additions and 25 deletions.
diff --git a/pom.xml b/pom.xml
@@ -105,7 +105,8 @@ THE SOFTWARE.
     <dependency>
       <groupId>org.apache.ant</groupId>
       <artifactId>ant</artifactId>
-      <version>1.8.3</version>
+      <!--TODO: bump to 1.10.x when Remoting updates to Java8-->
+      <version>1.9.9</version>
       <scope>test</scope>
     </dependency>
     <dependency>
@@ -324,6 +325,14 @@ THE SOFTWARE.
                   <outputDirectory>target/test-classes</outputDirectory>
                   <destFileName>remoting-test-client-tests.jar</destFileName>
                 </artifactItem>
+                <artifactItem>
+                  <groupId>org.kohsuke</groupId>
+                  <artifactId>file-leak-detector</artifactId>
+                  <version>1.8</version>
+                  <classifier>jar-with-dependencies</classifier>
+                  <outputDirectory>${project.build.directory}/agents</outputDirectory>
+                  <destFileName>file-leak-detector.jar</destFileName>
+                </artifactItem>
               </artifactItems>
             </configuration>
           </execution>
@@ -494,6 +503,7 @@ THE SOFTWARE.
         <configuration>
           <trimStackTrace>false</trimStackTrace> <!-- SUREFIRE-1226 workaround -->
           <rerunFailingTestsCount>4</rerunFailingTestsCount>
+          <argLine>-javaagent:${project.build.directory}\agents\file-leak-detector.jar=http=19999</argLine>
         </configuration>
       </plugin>
       <plugin>

diff --git a/src/main/java/hudson/remoting/Channel.java b/src/main/java/hudson/remoting/Channel.java
@@ -170,7 +170,7 @@ public class Channel implements VirtualChannel, IChannel, Closeable {
     /**
      * {@link ClassLoader}s that are proxies of the remote classloaders.
      */
-    /*package*/ final ImportedClassLoaderTable importedClassLoaders = new ImportedClassLoaderTable(this);
+    /*package*/ final ImportedClassLoaderTable importedClassLoaders;
 
     /**
      * Objects exported via {@link #export(Class, Object)}.
@@ -498,6 +498,7 @@ protected Channel(@Nonnull ChannelBuilder settings, @Nonnull CommandTransport tr
         this.arbitraryCallableAllowed = settings.isArbitraryCallableAllowed();
         this.remoteClassLoadingAllowed = settings.isRemoteClassLoadingAllowed();
         this.underlyingOutput = transport.getUnderlyingStream();
+        this.importedClassLoaders = new ImportedClassLoaderTable(this, settings.isCacheFilesInJarURLConnections());
 
         // JAR Cache resolution
         JarCache effectiveJarCache = settings.getJarCache();
@@ -506,7 +507,7 @@ protected Channel(@Nonnull ChannelBuilder settings, @Nonnull CommandTransport tr
             logger.log(Level.CONFIG, "Using the default JAR Cache: {0}", effectiveJarCache);
         }
         this.jarCache = effectiveJarCache;
-
+                
         this.baseClassLoader = settings.getBaseLoader();
         this.classFilter = settings.getClassFilter();
 

diff --git a/src/main/java/hudson/remoting/ChannelBuilder.java b/src/main/java/hudson/remoting/ChannelBuilder.java
@@ -16,6 +16,7 @@
 import java.io.InputStream;
 import java.io.ObjectOutputStream;
 import java.io.OutputStream;
+import java.net.JarURLConnection;
 import java.net.Socket;
 import java.nio.channels.SocketChannel;
 import java.util.ArrayList;
@@ -54,6 +55,11 @@ public class ChannelBuilder {
     private boolean remoteClassLoadingAllowed = true;
     private final Hashtable<Object,Object> properties = new Hashtable<Object,Object>();
     private ClassFilter filter = ClassFilter.DEFAULT;
+    /**
+     * Manages caching of Files in {@link JarURLConnection}s.
+     * @since TODO
+     */
+    private boolean cacheFilesInJarURLConnections = true;
 
     /**
      * Specify the minimum mandatory parameters.
@@ -112,11 +118,34 @@ public ChannelBuilder withCapability(Capability capability) {
         this.capability = capability;
         return this;
     }
-
+    
     public Capability getCapability() {
         return capability;
     }
+
+    /**
+     * Optionally disables file caching in {@link JarURLConnection}s.
+     * It may be useful for tests and concurrent caches in Windows where the file locks may block cache modification otherwise.
+     * 
+     * @param cacheJarURLConnections {@code false} to disable caching of files in {@link JarURLConnection}s in JAR resources.
+     * @return {@code this}
+     * @since TODO
+     */
+    public ChannelBuilder withFileCachingInJarURLConnections(boolean cacheJarURLConnections) {
+        this.cacheFilesInJarURLConnections = cacheJarURLConnections;
+        return this;
+    }
 
+    /**
+     * Check if files should be cached in {@link JarURLConnection}s.
+     * 
+     * @return {@code true} if files should be cached (default behavior).
+     * @since TODO
+     */
+    public boolean isCacheFilesInJarURLConnections() {
+        return cacheFilesInJarURLConnections;
+    }
+
     /**
      * If non-null, receive the portion of data in <tt>is</tt> before
      * the data goes into the "binary mode". This is useful

diff --git a/src/main/java/hudson/remoting/ImportedClassLoaderTable.java b/src/main/java/hudson/remoting/ImportedClassLoaderTable.java
@@ -38,9 +38,15 @@
 final class ImportedClassLoaderTable {
     final Channel channel;
     final Map<IClassLoader,ClassLoader> classLoaders = new Hashtable<IClassLoader,ClassLoader>();
+    private final boolean cacheFilesInJarURLConnections;
 
     ImportedClassLoaderTable(Channel channel) {
+        this(channel, true);
+    }
+
+    ImportedClassLoaderTable(Channel channel, boolean cacheFilesInJarURLConnections) {
         this.channel = channel;
+        this.cacheFilesInJarURLConnections = cacheFilesInJarURLConnections;
     }
 
     /**
@@ -66,7 +72,7 @@ public synchronized ClassLoader get(@Nonnull IClassLoader classLoaderProxy) {
         if(r==null) {
             // we need to be able to use the same hudson.remoting classes, hence delegate
             // to this class loader.
-            r = RemoteClassLoader.create(channel.baseClassLoader,classLoaderProxy);
+            r = RemoteClassLoader.create(channel.baseClassLoader,classLoaderProxy,cacheFilesInJarURLConnections);
             classLoaders.put(classLoaderProxy,r);
         }
         return r;

diff --git a/src/main/java/hudson/remoting/RemoteClassLoader.java b/src/main/java/hudson/remoting/RemoteClassLoader.java
@@ -50,6 +50,7 @@
 import javax.annotation.CheckForNull;
 
 import static hudson.remoting.Util.*;
+import java.net.JarURLConnection;
 import static java.util.logging.Level.*;
 import javax.annotation.Nonnull;
 
@@ -105,6 +106,8 @@ final class RemoteClassLoader extends URLClassLoader {
      */
     private final Map<String,ClassReference> prefetchedClasses = Collections.synchronizedMap(new HashMap<String,ClassReference>());
 
+    private final boolean cacheFilesInJarURLConnections;
+
     /**
      * Creates a remotable classloader
      * @param parent Parent classloader. Can be {@code null} if there is no delegating classloader
@@ -113,17 +116,34 @@ final class RemoteClassLoader extends URLClassLoader {
      */
     @Nonnull
     public static ClassLoader create(@CheckForNull ClassLoader parent, @Nonnull IClassLoader proxy) {
+        return create(parent, proxy, true);
+    }
+
+    /**
+     * Creates a remotable classloader.
+     * 
+     * @param parent Parent classloader. Can be {@code null} if there is no delegating classloader
+     * @param proxy Classloader proxy instance
+     * @param cacheFilesInJarURLConnections If {@code true}, Files will be cached in {@link JarURLConnection}s.
+     *              It greatly increases performance of resource loading, but it may cause unstable resource locking on platforms like Windows.
+     *              Enabling this option is recommended in production instances.
+     * @return Created classloader
+     * @since TODO
+     */
+    @Nonnull
+    public static ClassLoader create(@CheckForNull ClassLoader parent, @Nonnull IClassLoader proxy, boolean cacheFilesInJarURLConnections) {
         if(proxy instanceof ClassLoaderProxy) {
             // when the remote sends 'RemoteIClassLoader' as the proxy, on this side we get it
             // as ClassLoaderProxy. This means, the so-called remote classloader here is
             // actually our classloader that we exported to the other side.
             return ((ClassLoaderProxy)proxy).cl;
         }
-        return new RemoteClassLoader(parent, proxy);
+        return new RemoteClassLoader(parent, proxy, cacheFilesInJarURLConnections);
     }
 
-    private RemoteClassLoader(@CheckForNull ClassLoader parent, @Nonnull IClassLoader proxy) {
+    private RemoteClassLoader(@CheckForNull ClassLoader parent, @Nonnull IClassLoader proxy, boolean cacheFilesInJarURLConnections) {
         super(new URL[0],parent);
+        this.cacheFilesInJarURLConnections = cacheFilesInJarURLConnections;
         final Channel channel = RemoteInvocationHandler.unwrap(proxy);
         this.channel = channel == null ? null : channel.ref();
         this.underlyingProxy = proxy;
@@ -732,47 +752,62 @@ public static interface IClassLoader {
     }
 
     public static IClassLoader export(@Nonnull ClassLoader cl, Channel local) {
+        boolean cacheFilesInJarURLConnections = true;
         if (cl instanceof RemoteClassLoader) {
             // check if this is a remote classloader from the channel
             final RemoteClassLoader rcl = (RemoteClassLoader) cl;
+            cacheFilesInJarURLConnections = rcl.cacheFilesInJarURLConnections;
             int oid = RemoteInvocationHandler.unwrap(rcl.underlyingProxy, local);
             if(oid!=-1) {
                 return new RemoteIClassLoader(oid,rcl.proxy);
             }
         }
-        return local.export(IClassLoader.class, new ClassLoaderProxy(cl,local), false);
+        return local.export(IClassLoader.class, new ClassLoaderProxy(cl, local, cacheFilesInJarURLConnections), false);
     }
 
     public static void pin(ClassLoader cl, Channel local) {
+        boolean cacheFilesInJarURLConnections = true;
         if (cl instanceof RemoteClassLoader) {
             // check if this is a remote classloader from the channel
             final RemoteClassLoader rcl = (RemoteClassLoader) cl;
+            cacheFilesInJarURLConnections = rcl.cacheFilesInJarURLConnections;
             int oid = RemoteInvocationHandler.unwrap(rcl.proxy, local);
             if(oid!=-1) return;
         }
-        local.pin(new ClassLoaderProxy(cl,local));
+        local.pin(new ClassLoaderProxy(cl, local, cacheFilesInJarURLConnections));
     }
 
     /**
      * Exports and just returns the object ID, instead of obtaining the proxy.
      */
     static int exportId(ClassLoader cl, Channel local) {
-        return local.internalExport(IClassLoader.class, new ClassLoaderProxy(cl, local), false);
+        boolean cacheFilesInJarURLConnections = true;
+        if (cl instanceof RemoteClassLoader) {
+            cacheFilesInJarURLConnections = ((RemoteClassLoader)cl).cacheFilesInJarURLConnections;
+        }
+        return local.internalExport(IClassLoader.class, new ClassLoaderProxy(cl, local, cacheFilesInJarURLConnections), false);
     }
 
     /*package*/ static final class ClassLoaderProxy implements IClassLoader {
         final ClassLoader cl;
         final Channel channel;
+        private final boolean cacheFilesInJarURLConnections;
+
         /**
          * Class names that we've already sent to the other side as pre-fetch.
          */
         private final Set<String> prefetched = new HashSet<String>();
 
         public ClassLoaderProxy(@Nonnull ClassLoader cl, Channel channel) {
+            this(cl, channel, true);
+        }
+
+        public ClassLoaderProxy(@Nonnull ClassLoader cl, Channel channel, boolean cacheFilesInJarURLConnections) {
             assert cl != null;
 
             this.cl = cl;
             this.channel = channel;
+            this.cacheFilesInJarURLConnections = cacheFilesInJarURLConnections;
         }
 
         public byte[] fetchJar(URL url) throws IOException {
@@ -848,9 +883,9 @@ public ClassFile2 fetch4(String className, ClassFile2 referer) throws ClassNotFo
                         if (referer==null && !channel.jarLoader.isPresentOnRemote(sum))
                             // for the class being requested, if the remote doesn't have the jar yet
                             // send the image as well, so as not to require another call to get this class loaded
-                            imageRef = new ResourceImageBoth(urlOfClassFile,sum);
+                            imageRef = new ResourceImageBoth(urlOfClassFile,sum,cacheFilesInJarURLConnections);
                         else // otherwise just send the checksum and save space
-                            imageRef = new ResourceImageInJar(sum,null /* TODO: we need to check if the URL of c points to the expected location of the file */);
+                            imageRef = new ResourceImageInJar(sum,null /* TODO: we need to check if the URL of c points to the expected location of the file */, cacheFilesInJarURLConnections);
 
                         return new ClassFile2(exportId(ecl,channel), imageRef, referer, c, urlOfClassFile);
                     }
@@ -927,9 +962,9 @@ private ResourceFile makeResource(String name, URL resource) throws IOException
                     Checksum sum = channel.jarLoader.calcChecksum(jar);
                     ResourceImageRef ir;
                     if (!channel.jarLoader.isPresentOnRemote(sum))
-                        ir = new ResourceImageBoth(resource,sum);   // remote probably doesn't have
+                        ir = new ResourceImageBoth(resource, sum, cacheFilesInJarURLConnections);   // remote probably doesn't have
                     else
-                        ir = new ResourceImageInJar(sum,null);
+                        ir = new ResourceImageInJar(sum, null, cacheFilesInJarURLConnections);
                     return new ResourceFile(ir,resource);
                 }
             } catch (IllegalArgumentException e) {

diff --git a/src/main/java/hudson/remoting/ResourceImageBoth.java b/src/main/java/hudson/remoting/ResourceImageBoth.java
@@ -11,11 +11,17 @@
  */
 class ResourceImageBoth extends ResourceImageDirect {
     final long sum1,sum2;
+    final boolean useFileCachesInJarUrlConnection;
 
     public ResourceImageBoth(URL resource, Checksum sum) throws IOException {
+        this(resource, sum, true);
+    }
+
+    public ResourceImageBoth(URL resource, Checksum sum, boolean useFileCachesInJarUrlConnection) throws IOException {
         super(resource);
         this.sum1 = sum.sum1;
         this.sum2 = sum.sum2;
+        this.useFileCachesInJarUrlConnection = useFileCachesInJarUrlConnection;
     }
 
     @Override
@@ -28,7 +34,7 @@ Future<byte[]> resolve(Channel channel, String resourcePath) throws IOException,
     Future<URLish> resolveURL(Channel channel, String resourcePath) throws IOException, InterruptedException {
         Future<URL> f = initiateJarRetrieval(channel);
         if (f.isDone()) // prefer using the jar URL if the stuff is already available
-            return new ResourceImageInJar(sum1,sum2,null).resolveURL(channel,resourcePath);
+            return new ResourceImageInJar(sum1,sum2,null,useFileCachesInJarUrlConnection).resolveURL(channel,resourcePath);
         else
             return super.resolveURL(channel, resourcePath);
     }

diff --git a/src/main/java/hudson/remoting/ResourceImageInJar.java b/src/main/java/hudson/remoting/ResourceImageInJar.java
@@ -1,8 +1,10 @@
 package hudson.remoting;
 
 import java.io.IOException;
+import java.io.InputStream;
 import java.net.URL;
 import java.net.URLClassLoader;
+import java.net.URLConnection;
 import java.util.concurrent.ExecutionException;
 import javax.annotation.Nonnull;
 
@@ -27,11 +29,22 @@ class ResourceImageInJar extends ResourceImageRef {
      * the exact path inside the jar file of the resource.
      */
     final String path;
+
+    final boolean useCaches;
 
-    ResourceImageInJar(long sum1, long sum2, String path) {
+    ResourceImageInJar(long sum1, long sum2, String path, boolean useCaches) {
         this.sum1 = sum1;
         this.sum2 = sum2;
         this.path = path;
+        this.useCaches = useCaches;
+    }
+
+    ResourceImageInJar(long sum1, long sum2, String path) {
+        this(sum1, sum2, path, true);
+    }
+
+    ResourceImageInJar(Checksum sum, String path, boolean useCaches) {
+        this(sum.sum1, sum.sum2, path, useCaches);
     }
 
     ResourceImageInJar(Checksum sum, String path) {
@@ -43,8 +56,20 @@ Future<byte[]> resolve(Channel channel, final String resourcePath) throws IOExce
         return new FutureAdapter<byte[],URL>(_resolveJarURL(channel)) {
             @Override
             protected byte[] adapt(URL jar) throws ExecutionException {
+                final URLConnection c;
                 try {
-                    return Util.readFully(toResourceURL(jar,resourcePath).openStream());
+                    URL url = toResourceURL(jar,resourcePath);
+                    c = url.openConnection();
+                    if (!useCaches) {
+                        // Disable caching if requested
+                        c.setUseCaches(false);
+                    }
+                } catch (IOException ex) {
+                    throw new ExecutionException(ex);
+                }
+
+                try(InputStream istream = c.getInputStream()) {
+                    return Util.readFully(istream);
                 } catch (IOException e) {
                     throw new ExecutionException(e);
                 }
@@ -76,6 +101,8 @@ private URL toResourceURL(URL jar, String resourcePath) throws IOException {
                 open jar file (see sun.net.www.protocol.jar.JarURLConnection.JarURLInputStream.close())
                 and leave it open. During unit test, this pins the file, and it prevents the test tear down code
                 from deleting the file.
+            Oleg Nenashev:
+                It does not happen if internal caching is disabled
          */
         return new URL("jar:"+ jar +"!/"+resourcePath);
     }

diff --git a/src/main/java/org/jenkinsci/remoting/nio/NioChannelBuilder.java b/src/main/java/org/jenkinsci/remoting/nio/NioChannelBuilder.java
@@ -88,4 +88,9 @@ public NioChannelBuilder withJarCache(JarCache jarCache) {
     public NioChannelBuilder withClassFilter(ClassFilter filter) {
         return (NioChannelBuilder)super.withClassFilter(filter);
     }
+
+    @Override
+    public NioChannelBuilder withFileCachingInJarURLConnections(boolean cacheJarURLConnections) {
+        return (NioChannelBuilder)super.withFileCachingInJarURLConnections(cacheJarURLConnections);
+    } 
 }