You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@felix.apache.org by ja...@apache.org on 2014/04/15 17:32:44 UTC
svn commit: r1587613 - in /felix/trunk/deploymentadmin/deploymentadmin/src:
main/java/org/apache/felix/deploymentadmin/
test/java/org/apache/felix/deploymentadmin/
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