Mailing-List: contact commits-help@felix.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@felix.apache.org
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: svn commit: r1587613 - in
 /felix/trunk/deploymentadmin/deploymentadmin/src:
 main/java/org/apache/felix/deploymentadmin/
 test/java/org/apache/felix/deploymentadmin/
Date: Tue, 15 Apr 2014 15:32:44 -0000
To: commits@felix.apache.org
From: jawi@apache.org
Message-Id: <20140415153244.4DC7E23889EA@eris.apache.org>

Author: jawi
Date: Tue Apr 15 15:32:43 2014
New Revision: 1587613

URL: http://svn.apache.org/r1587613
Log:
FELIX-4486: fix possible thread leakage:

- replaced the ExplodingOutputtingInputStream with a much simpler 
  ContentCopyingJarInputStream that levarages the JarInputStream to copy
  entries while they are read;
- added some test cases to verify the new implementation.


Added:
    felix/trunk/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/ContentCopyingJarInputStream.java   (with props)
    felix/trunk/deploymentadmin/deploymentadmin/src/test/java/org/apache/felix/deploymentadmin/ContentCopyingJarInputStreamTest.java   (with props)
Removed:
    felix/trunk/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/ExplodingOutputtingInputStream.java
    felix/trunk/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/OutputtingInputStream.java
    felix/trunk/deploymentadmin/deploymentadmin/src/test/java/org/apache/felix/deploymentadmin/ExplodingOutputtingInputStreamTest.java
Modified:
    felix/trunk/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/DeploymentAdminImpl.java
    felix/trunk/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/Utils.java

Added: felix/trunk/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/ContentCopyingJarInputStream.java
URL: http://svn.apache.org/viewvc/felix/trunk/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/ContentCopyingJarInputStream.java?rev=1587613&view=auto
==============================================================================
--- felix/trunk/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/ContentCopyingJarInputStream.java (added)
+++ felix/trunk/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/ContentCopyingJarInputStream.java Tue Apr 15 15:32:43 2014
@@ -0,0 +1,187 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.felix.deploymentadmin;
+
+import java.io.Closeable;
+import java.io.File;
+import java.io.FileOutputStream;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
+import java.io.PrintWriter;
+import java.util.jar.JarFile;
+import java.util.jar.JarInputStream;
+import java.util.jar.Manifest;
+import java.util.zip.GZIPOutputStream;
+import java.util.zip.ZipEntry;
+
+/**
+ * Provides a custom {@link JarInputStream} that copies all entries read from the original 
+ * {@link InputStream} to a given directory and index file. It does this by tracking the
+ * common usage of the {@link JarInputStream} API. For each entry that is read it streams
+ * all read bytes to a separate file compressing it on the fly. The caller does not notice
+ * anything, although it might be that the {@link #read(byte[], int, int)} is blocked for
+ * a little while during the writing of the file contents.
+ * <p>
+ * This implementation replaces the old <tt>ExplodingOutputtingInputStream</tt> that used
+ * at least two threads and was difficult to understand and maintain. See FELIX-4486.
+ * </p>
+ */
+class ContentCopyingJarInputStream extends JarInputStream
+{
+    private static final String MANIFEST_FILE = JarFile.MANIFEST_NAME;
+
+    private final File m_contentDir;
+
+    private PrintWriter m_indexFileWriter;
+    /** Used to copy the contents of the *next* entry. */
+    private OutputStream m_entryOS;
+
+    public ContentCopyingJarInputStream(InputStream in, File indexFile, File contentDir) throws IOException
+    {
+        super(in);
+
+        m_contentDir = contentDir;
+
+        m_indexFileWriter = new PrintWriter(new FileWriter(indexFile));
+        m_entryOS = null;
+
+        // the manifest of the JAR is already read by JarInputStream, so we need to write this one as well...
+        Manifest manifest = getManifest();
+        if (manifest != null)
+        {
+            copyManifest(manifest);
+        }
+    }
+
+    public void close() throws IOException
+    {
+        closeCopy();
+        closeIndex();
+        super.close();
+    }
+
+    public void closeEntry() throws IOException
+    {
+        closeCopy();
+        super.closeEntry();
+    }
+
+    public ZipEntry getNextEntry() throws IOException
+    {
+        closeCopy();
+
+        ZipEntry entry = super.getNextEntry();
+        if (entry != null)
+        {
+            File current = new File(m_contentDir, entry.getName());
+            if (!entry.isDirectory())
+            {
+                addToIndex(entry.getName());
+
+                m_entryOS = createOutputStream(current);
+            }
+        }
+
+        return entry;
+    }
+
+    public int read(byte[] b, int off, int len) throws IOException
+    {
+        int r = super.read(b, off, len);
+        if (m_entryOS != null)
+        {
+            if (r > 0)
+            {
+                m_entryOS.write(b, off, r);
+            }
+            else
+            {
+                closeCopy();
+            }
+        }
+        return r;
+    }
+
+    private void addToIndex(String name) throws IOException
+    {
+        m_indexFileWriter.println(name);
+        m_indexFileWriter.flush();
+    }
+
+    private void closeCopy()
+    {
+        closeSilently(m_entryOS);
+        m_entryOS = null;
+    }
+
+    private void closeIndex()
+    {
+        closeSilently(m_indexFileWriter);
+        m_indexFileWriter = null;
+    }
+
+    private void closeSilently(Closeable resource)
+    {
+        try
+        {
+            if (resource != null)
+            {
+                resource.close();
+            }
+        }
+        catch (IOException e)
+        {
+            // Ignore, not much we can do about this...
+        }
+    }
+
+    /**
+     * Creates a verbatim copy of the manifest, when it is read from the original JAR.
+     */
+    private void copyManifest(Manifest manifest) throws IOException
+    {
+        addToIndex(MANIFEST_FILE);
+
+        OutputStream os = createOutputStream(new File(m_contentDir, MANIFEST_FILE));
+        try
+        {
+            manifest.write(os);
+        }
+        finally
+        {
+            closeSilently(os);
+        }
+    }
+
+    private OutputStream createOutputStream(File file) throws IOException
+    {
+        File parent = file.getParentFile();
+        if (parent != null)
+        {
+            parent.mkdirs();
+        }
+        if (!file.createNewFile())
+        {
+            throw new IOException("Attempt to overwrite file: " + file);
+        }
+        return new GZIPOutputStream(new FileOutputStream(file));
+    }
+}

Propchange: felix/trunk/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/ContentCopyingJarInputStream.java
------------------------------------------------------------------------------
    svn:eol-style = native

Modified: felix/trunk/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/DeploymentAdminImpl.java
URL: http://svn.apache.org/viewvc/felix/trunk/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/DeploymentAdminImpl.java?rev=1587613&r1=1587612&r2=1587613&view=diff
==============================================================================
--- felix/trunk/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/DeploymentAdminImpl.java (original)
+++ felix/trunk/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/DeploymentAdminImpl.java Tue Apr 15 15:32:43 2014
@@ -131,8 +131,8 @@ public class DeploymentAdminImpl impleme
         return m_packageAdmin;
     }
 
-    public DeploymentPackage installDeploymentPackage(InputStream input) throws DeploymentException {
-        if (input == null) {
+    public DeploymentPackage installDeploymentPackage(InputStream sourceInput) throws DeploymentException {
+        if (sourceInput == null) {
             throw new IllegalArgumentException("Inputstream may not be null");
         }
 
@@ -163,7 +163,6 @@ public class DeploymentAdminImpl impleme
                 tempIndex = new File(tempPackage, PACKAGEINDEX_FILE);
                 tempContents = new File(tempPackage, PACKAGECONTENTS_DIR);
                 tempContents.mkdirs();
-                input = new ExplodingOutputtingInputStream(input, tempIndex, tempContents);
             }
             catch (IOException e) {
                 m_log.log(LogService.LOG_ERROR, "Error writing package to disk", e);
@@ -171,7 +170,7 @@ public class DeploymentAdminImpl impleme
             }
 
             try {
-                jarInput = new JarInputStream(input);
+                jarInput = new ContentCopyingJarInputStream(sourceInput, tempIndex, tempContents);
                 
                 if (jarInput.getManifest() == null) {
                     m_log.log(LogService.LOG_ERROR, "Stream does not contain a valid deployment package: missing manifest!");
@@ -206,31 +205,12 @@ public class DeploymentAdminImpl impleme
                 verifySourcePackage(source);
             }
 
-            // To keep track whether or not we're masking an exception during the close of the input stream...
-            boolean installFailed = false;
-            
             try {
                 m_session = new DeploymentSessionImpl(source, target, createInstallCommandChain(), this);
                 m_session.call(false /* ignoreExceptions */);
             }
             catch (DeploymentException de) {
-                installFailed = true;
                 throw de;
-            } finally {
-                try {
-                    // make sure we've read until the end-of-stream, so the explodingoutput-wrapper can process all bytes
-                    Utils.readUntilEndOfStream(input);
-
-                    // note that calling close on this stream will wait until asynchronous processing is done
-                    input.close();
-                }
-                catch (IOException e) {
-                    m_log.log(LogService.LOG_ERROR, "Could not close stream properly", e);
-                    // Do not mask out any originally thrown exceptions...
-                    if (!installFailed) {
-                        throw new DeploymentException(DeploymentException.CODE_OTHER_ERROR, "Could not close stream properly", e);
-                    }
-                }
             }
 
             String dpInstallBaseDirectory = PACKAGE_DIR + File.separator + dpSymbolicName;
@@ -277,6 +257,7 @@ public class DeploymentAdminImpl impleme
                 	succeeded = false;
                 }
             }
+
     	    sendCompleteEvent(source, target, succeeded);
             m_semaphore.release();
         }

Modified: felix/trunk/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/Utils.java
URL: http://svn.apache.org/viewvc/felix/trunk/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/Utils.java?rev=1587613&r1=1587612&r2=1587613&view=diff
==============================================================================
--- felix/trunk/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/Utils.java (original)
+++ felix/trunk/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/Utils.java Tue Apr 15 15:32:43 2014
@@ -33,12 +33,15 @@ import java.util.ArrayList;
 import java.util.Iterator;
 import java.util.List;
 import java.util.jar.Attributes;
+import java.util.jar.JarFile;
 import java.util.jar.Manifest;
 import java.util.jar.Attributes.Name;
 import java.util.zip.GZIPInputStream;
 import java.util.zip.GZIPOutputStream;
 
 public class Utils {
+    private static final String MANIFEST_NAME = JarFile.MANIFEST_NAME;
+
     public static Manifest readManifest(File manifestFile) throws IOException {
         InputStream is = null;
         Manifest mf = null;
@@ -51,14 +54,6 @@ public class Utils {
         }
         return mf;
     }
-    
-    public static void readUntilEndOfStream(InputStream is) throws IOException {
-        byte[] buffer = new byte[1024];
-        int c = is.read(buffer);
-        while (c != -1) {
-            c = is.read(buffer);
-        }
-    }
 
     public static boolean replace(File target, File source) {
         return delete(target, true /* deleteRoot */) && rename(source, target);
@@ -153,7 +148,7 @@ public class Utils {
 
         for (Iterator i = result.iterator(); i.hasNext();) {
             String targetFile = (String) i.next();
-            if (!"META-INF/MANIFEST.MF".equals(targetFile) && !resultManifest.getEntries().containsKey(targetFile)) {
+            if (!MANIFEST_NAME.equals(targetFile) && !resultManifest.getEntries().containsKey(targetFile)) {
                 i.remove();
             }
         }
@@ -193,7 +188,7 @@ public class Utils {
             }
         }
 
-        GZIPOutputStream outputStream = new GZIPOutputStream(new FileOutputStream(new File(target, "META-INF/MANIFEST.MF")));
+        GZIPOutputStream outputStream = new GZIPOutputStream(new FileOutputStream(new File(target, MANIFEST_NAME)));
         try {
             resultManifest.write(outputStream);
         } finally {

Added: felix/trunk/deploymentadmin/deploymentadmin/src/test/java/org/apache/felix/deploymentadmin/ContentCopyingJarInputStreamTest.java
URL: http://svn.apache.org/viewvc/felix/trunk/deploymentadmin/deploymentadmin/src/test/java/org/apache/felix/deploymentadmin/ContentCopyingJarInputStreamTest.java?rev=1587613&view=auto
==============================================================================
--- felix/trunk/deploymentadmin/deploymentadmin/src/test/java/org/apache/felix/deploymentadmin/ContentCopyingJarInputStreamTest.java (added)
+++ felix/trunk/deploymentadmin/deploymentadmin/src/test/java/org/apache/felix/deploymentadmin/ContentCopyingJarInputStreamTest.java Tue Apr 15 15:32:43 2014
@@ -0,0 +1,254 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.felix.deploymentadmin;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.util.jar.JarEntry;
+import java.util.jar.JarFile;
+import java.util.jar.JarInputStream;
+import java.util.jar.JarOutputStream;
+import java.util.jar.Manifest;
+import java.util.zip.GZIPInputStream;
+import java.util.zip.ZipEntry;
+
+import junit.framework.TestCase;
+
+/**
+ * Test cases for {@link ContentCopyingJarInputStream}.
+ */
+public class ContentCopyingJarInputStreamTest extends TestCase
+{
+    private static final String MANIFEST_NAME = JarFile.MANIFEST_NAME;
+    private static final String INDEX_NAME = "META-INF/INDEX.LIST";
+    
+    private File m_tempDir;
+    private File m_jarFile;
+
+    /**
+     * Tests that we can copy a simple {@link JarInputStream}. 
+     */
+    public void testCopyEmptyJarOk() throws Exception
+    {
+        createEmptyJar();
+
+        assertJarContents(null);
+    }
+
+    /**
+     * Tests that we can copy a simple {@link JarInputStream}. 
+     */
+    public void testCopyJarWithIndexAndWithManifestOk() throws Exception
+    {
+        Manifest man = createManifest();
+
+        createJar(man, true /* includeIndex */);
+
+        assertJarContents(man);
+    }
+
+    /**
+     * Tests that we can copy a {@link JarInputStream} even if it does not contains a manifest file. 
+     */
+    public void testCopyJarWithIndexAndWithoutManifestOk() throws Exception
+    {
+        Manifest man = null;
+
+        createJar(man, true /* includeIndex */);
+
+        assertJarContents(man);
+    }
+
+    /**
+     * Tests that we can copy a simple {@link JarInputStream}. 
+     */
+    public void testCopyJarWithoutIndexAndWithManifestOk() throws Exception
+    {
+        Manifest man = createManifest();
+
+        createJar(man, false /* includeIndex */);
+
+        assertJarContents(man);
+    }
+
+    /**
+     * Tests that we can copy a {@link JarInputStream} even if it does not contains a manifest file. 
+     */
+    public void testCopyJarWithoutIndexAndWithoutManifestOk() throws Exception
+    {
+        Manifest man = null;
+
+        createJar(man, false /* includeIndex */);
+
+        assertJarContents(man);
+    }
+
+    protected void setUp() throws IOException
+    {
+        m_tempDir = createTempDir();
+        m_jarFile = new File(m_tempDir, "input.jar");
+    }
+
+    protected void tearDown()
+    {
+        Utils.delete(m_tempDir, true);
+    }
+
+    private void appendFiles(JarOutputStream jos, int count) throws IOException
+    {
+        int size = 1024;
+
+        for (int i = 0, j = 1; i < count; i++, j++)
+        {
+            JarEntry entry = new JarEntry("sub/" + j);
+            jos.putNextEntry(entry);
+            for (int k = 0; k < size; k++)
+            {
+                jos.write('0' + j);
+            }
+            jos.closeEntry();
+        }
+    }
+
+    private void assertJarContents(Manifest man) throws IOException
+    {
+        File indexFile = new File(m_tempDir, "index.txt");
+
+        FileInputStream fis = new FileInputStream(m_jarFile);
+        JarInputStream jis = new ContentCopyingJarInputStream(fis, indexFile, m_tempDir);
+
+        try
+        {
+            JarEntry entry;
+            while ((entry = jis.getNextJarEntry()) != null)
+            {
+                File f = new File(m_tempDir, entry.getName());
+
+                // Without reading the actual contents, the copy should already exist...
+                assertTrue(entry.getName() + " does not exist?!", f.exists());
+                
+                int size = (INDEX_NAME.equals(entry.getName()) ? 33 : 1024);
+
+                byte[] input = new byte[size];
+                int read = jis.read(input);
+
+                assertEquals("Not all bytes were read: " + entry.getName(), size, read);
+
+                // Contents will only be completely written after closing the JAR entry itself...
+                jis.closeEntry();
+
+                verifyContents(f, input);
+            }
+
+            assertEquals("Manifest not as expected", man, jis.getManifest());
+        }
+        finally
+        {
+            jis.close();
+        }
+    }
+    
+    private void createEmptyJar() throws IOException {
+        FileOutputStream fos = new FileOutputStream(m_jarFile);
+        JarOutputStream jos = new JarOutputStream(fos);
+        jos.close();
+    }
+
+    private void createJar(Manifest man, boolean includeIndex) throws IOException
+    {
+        FileOutputStream fos = new FileOutputStream(m_jarFile);
+        JarOutputStream jos;
+
+        if (man == null || includeIndex)
+        {
+            jos = new JarOutputStream(fos);
+        }
+        else
+        {
+            jos = new JarOutputStream(fos, man);
+        }
+
+        if (includeIndex)
+        {
+            // Write the INDEX.LIST file as first entry...
+            jos.putNextEntry(new ZipEntry(INDEX_NAME));
+            jos.write(("JarIndex-Version: 1.0\n\n" + m_jarFile.getName() + "\n").getBytes());
+            jos.closeEntry();
+
+            if (man != null)
+            {
+                jos.putNextEntry(new ZipEntry(MANIFEST_NAME));
+                man.write(jos);
+                jos.closeEntry();
+            }
+        }
+
+        try
+        {
+            appendFiles(jos, 5);
+        }
+        finally
+        {
+            jos.close();
+        }
+    }
+
+    private Manifest createManifest()
+    {
+        Manifest mf = new Manifest();
+        mf.getMainAttributes().putValue("Manifest-Version", "1.0");
+        mf.getMainAttributes().putValue("Bundle-ManifestVersion", "2");
+        mf.getMainAttributes().putValue("Bundle-Version", "1.0.0");
+        mf.getMainAttributes().putValue("Bundle-SymbolicName", "com.foo.bar");
+        return mf;
+    }
+
+    private File createTempDir() throws IOException
+    {
+        File tmpFile = File.createTempFile("ccjis_test", null);
+        tmpFile.delete();
+        tmpFile.mkdir();
+        return tmpFile;
+    }
+
+    private void verifyContents(File file, byte[] expected) throws IOException
+    {
+        FileInputStream fis = new FileInputStream(file);
+        GZIPInputStream gis = new GZIPInputStream(fis);
+        try
+        {
+            byte[] b = new byte[expected.length];
+
+            int read = gis.read(b);
+            assertEquals(b.length, read);
+
+            for (int i = 0; i < expected.length; i++)
+            {
+                assertEquals(expected[i], b[i]);
+            }
+        }
+        finally
+        {
+            gis.close();
+            fis.close();
+        }
+    }
+}

Propchange: felix/trunk/deploymentadmin/deploymentadmin/src/test/java/org/apache/felix/deploymentadmin/ContentCopyingJarInputStreamTest.java
------------------------------------------------------------------------------
    svn:eol-style = native