FELIX-4409/4410:

- improve logging in case a bundle update fails by including the BSN
  and version(s);
- made the update commands a little more resilient againts unchecked
  exceptions to ensure that an (un)installation of a DP always yields
  a DeploymentException in case of failures;
- added JUnit test to test the behaviour of DA when an invalid DP is
  given (e.g., containing two bundles with the same BSN).



git-svn-id: https://svn.apache.org/repos/asf/felix/trunk@1563873 13f79535-47bb-0310-9956-ffa450edef68
diff --git a/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/AbstractAction.java b/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/AbstractAction.java
new file mode 100644
index 0000000..c9298d9
--- /dev/null
+++ b/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/AbstractAction.java
@@ -0,0 +1,46 @@
+/*
+ * 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.spi;
+
+/**
+ * Base implementation for commit/rollback actions with proper error handling.
+ * 
+ * @author <a href="mailto:dev@felix.apache.org">Felix Project Team</a>
+ */
+abstract class AbstractAction implements Runnable {
+    /**
+     * Runs this action by calling {@link #doRun()} and in case of failure,
+     * {@link #onFailure(Exception)}.
+     */
+    public final void run() {
+        try {
+            doRun();
+        }
+        catch (Exception e) {
+            onFailure(e);
+        }
+    }
+
+    protected abstract void doRun() throws Exception;
+
+    protected void onFailure(Exception e) {
+        // Nop
+    }
+}
diff --git a/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/Command.java b/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/Command.java
index 08bb2e0..312d66c 100644
--- a/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/Command.java
+++ b/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/Command.java
@@ -18,6 +18,8 @@
  */
 package org.apache.felix.deploymentadmin.spi;
 
+import java.io.Closeable;
+import java.io.IOException;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.ListIterator;
@@ -27,51 +29,89 @@
 import org.osgi.service.deploymentadmin.DeploymentException;
 
 /**
- * Commands describe a group of tasks to be executed within the execution a deployment session.
- * A command that has already executed can be rolled back and a command that is currently in progress
- * can be signaled to stop it's activities by canceling it.
+ * Commands describe a group of tasks to be executed within the execution a
+ * deployment session. A command that has already executed can be rolled back
+ * and a command that is currently in progress can be signaled to stop it's
+ * activities by canceling it.
  */
 public abstract class Command {
 
     private final List m_rollback = new ArrayList();
     private final List m_commit = new ArrayList();
+
     private volatile boolean m_cancelled;
 
     /**
-     * Executes the command, the specified <code>DeploymentSession</code> can be used to obtain various
-     * information about the deployment session which the command is part of.
-     *
-     * @param session The deployment session this command is part of.
-     * @throws DeploymentException Thrown if the command could not successfully execute.
-     */
-    public abstract void execute(DeploymentSessionImpl session) throws DeploymentException;
-
-    /**
-     * Rolls back all actions that were added through the <code>addRollback(Runnable r)</code> method (in reverse
-     * adding order). It is not guaranteed that the state of everything related to the command will be as if the
-     * command was never executed, a best effort should be made though.
+     * Executes the command, the specified <code>DeploymentSession</code> can be
+     * used to obtain various information about the deployment session which the
+     * command is part of.
      * 
-     * @param session the deployment session to rollback, should be used for logging purposes.
+     * @param session The deployment session this command is part of.
+     * @throws DeploymentException Thrown if the command could not successfully
+     *         execute.
      */
-    protected void rollback(DeploymentSessionImpl session) {
-        for (ListIterator i = m_rollback.listIterator(m_rollback.size()); i.hasPrevious();) {
-            Runnable runnable = (Runnable) i.previous();
-            runnable.run();
+    public final void execute(DeploymentSessionImpl session) throws DeploymentException {
+        try {
+            doExecute(session);
         }
-        cleanUp();
+        catch (Exception e) {
+            if (e instanceof DeploymentException) {
+                throw (DeploymentException) e;
+            }
+            throw new DeploymentException(DeploymentException.CODE_OTHER_ERROR, "Command failed!", e);
+        }
     }
 
     /**
-     * Commits all changes the command may have defined when it was executed by calling the <code>execute()</code> method.
+     * Executes the command, the specified <code>DeploymentSession</code> can be
+     * used to obtain various information about the deployment session which the
+     * command is part of.
      * 
-     * @param session the deployment session to commit, should be used for logging purposes.
+     * @param session The deployment session this command is part of.
+     * @throws Exception in case of this command could not terminate
+     *         successfully.
      */
-    protected void commit(DeploymentSessionImpl session) {
-        for (ListIterator i = m_commit.listIterator(); i.hasNext();) {
-            Runnable runnable = (Runnable) i.next();
-            runnable.run();
+    protected abstract void doExecute(DeploymentSessionImpl session) throws Exception;
+
+    /**
+     * Rolls back all actions that were added through the
+     * <code>addRollback(Runnable r)</code> method (in reverse adding order). It
+     * is not guaranteed that the state of everything related to the command
+     * will be as if the command was never executed, a best effort should be
+     * made though.
+     * 
+     * @param session the deployment session to rollback, should be used for
+     *        logging purposes.
+     */
+    protected void rollback(DeploymentSessionImpl session) {
+        try {
+            for (ListIterator i = m_rollback.listIterator(m_rollback.size()); i.hasPrevious();) {
+                Runnable runnable = (Runnable) i.previous();
+                runnable.run();
+            }
         }
-        cleanUp();
+        finally {
+            cleanUp();
+        }
+    }
+
+    /**
+     * Commits all changes the command may have defined when it was executed by
+     * calling the <code>execute()</code> method.
+     * 
+     * @param session the deployment session to commit, should be used for
+     *        logging purposes.
+     */
+    protected final void commit(DeploymentSessionImpl session) {
+        try {
+            for (ListIterator i = m_commit.listIterator(); i.hasNext();) {
+                Runnable runnable = (Runnable) i.next();
+                runnable.run();
+            }
+        }
+        finally {
+            cleanUp();
+        }
     }
 
     private void cleanUp() {
@@ -81,9 +121,11 @@
     }
 
     /**
-     * Determines if the command was canceled. This method should be used regularly by implementing classes to determine if
-     * their command was canceled, if so they should return as soon as possible from their operations.
-     *
+     * Determines if the command was canceled. This method should be used
+     * regularly by implementing classes to determine if their command was
+     * canceled, if so they should return as soon as possible from their
+     * operations.
+     * 
      * @return true if the command was canceled, false otherwise.
      */
     protected boolean isCancelled() {
@@ -92,26 +134,28 @@
 
     /**
      * Adds an action to be executed in case of a roll back.
-     *
+     * 
      * @param runnable The runnable to be executed in case of a roll back.
      */
-    protected void addRollback(Runnable runnable) {
+    protected void addRollback(AbstractAction runnable) {
         m_rollback.add(runnable);
     }
 
     /**
      * Adds an action to be executed in case of a commit
-     *
+     * 
      * @param runnable The runnable to be executes in case of a commit.
      */
-    protected void addCommit(Runnable runnable) {
+    protected void addCommit(AbstractAction runnable) {
         m_commit.add(runnable);
     }
-    
+
     /**
-     * Sets the command to being cancelled, this does not have an immediate effect. Commands that are executing should
-     * check regularly if they were cancelled and if so they should make an effort to stop their operations as soon as possible
-     * followed by throwing an <code>DeploymentException.CODE_CANCELLED</code> exception.
+     * Sets the command to being cancelled, this does not have an immediate
+     * effect. Commands that are executing should check regularly if they were
+     * cancelled and if so they should make an effort to stop their operations
+     * as soon as possible followed by throwing an
+     * <code>DeploymentException.CODE_CANCELLED</code> exception.
      */
     public void cancel() {
         m_cancelled = true;
@@ -119,11 +163,24 @@
 
     /**
      * Determines whether a given bundle is actually a fragment bundle.
+     * 
      * @param bundle the bundle to test, cannot be <code>null</code>.
-     * @return <code>true</code> if the given bundle is actually a fragment bundle, <code>false</code> otherwise.
+     * @return <code>true</code> if the given bundle is actually a fragment
+     *         bundle, <code>false</code> otherwise.
      */
     static final boolean isFragmentBundle(Bundle bundle) {
         Object fragmentHost = bundle.getHeaders().get(Constants.FRAGMENT_HOST);
         return fragmentHost != null;
     }
+    
+    static final void closeSilently(Closeable resource) {
+        if (resource != null) {
+            try {
+                resource.close();
+            }
+            catch (IOException e) {
+                // Not much we can do...
+            }
+        }
+    }
 }
diff --git a/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/CommitResourceCommand.java b/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/CommitResourceCommand.java
index e86edba..94de905 100644
--- a/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/CommitResourceCommand.java
+++ b/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/CommitResourceCommand.java
@@ -29,15 +29,16 @@
 import org.osgi.service.log.LogService;
 
 /**
- * Command that commits all the resource processors that were added to the command.
+ * Command that commits all the resource processors that were added to the
+ * command.
  */
 public class CommitResourceCommand extends Command {
 
     private final List m_processors = new ArrayList();
 
-    public void execute(DeploymentSessionImpl session) throws DeploymentException {
+    protected void doExecute(DeploymentSessionImpl session) throws Exception {
         for (ListIterator i = m_processors.listIterator(m_processors.size()); i.hasPrevious();) {
-    		ResourceProcessor processor = (ResourceProcessor) i.previous();
+            ResourceProcessor processor = (ResourceProcessor) i.previous();
             try {
                 processor.prepare();
             }
@@ -76,17 +77,20 @@
             catch (Exception e) {
                 // We cannot throw an exception, see OSGi spec.
                 session.getLog().log(LogService.LOG_ERROR, "Rollback of resource processor '" + processor + "' failed", e);
-            } finally {
+            }
+            finally {
                 i.remove();
             }
         }
     }
 
     /**
-     * Add a resource processor, all resource processors that are added will be committed when the command is executed.
-     *
+     * Add a resource processor, all resource processors that are added will be
+     * committed when the command is executed.
+     * 
      * @param processor The resource processor to add.
-     * @return true if the resource processor was added, false if it was already added.
+     * @return true if the resource processor was added, false if it was already
+     *         added.
      */
     public boolean addResourceProcessor(ResourceProcessor processor) {
         for (Iterator i = m_processors.iterator(); i.hasNext();) {
diff --git a/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/DeploymentSessionImpl.java b/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/DeploymentSessionImpl.java
index f546370..6c89186 100644
--- a/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/DeploymentSessionImpl.java
+++ b/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/DeploymentSessionImpl.java
@@ -43,6 +43,7 @@
     private final AbstractDeploymentPackage m_source;
     private final List m_commands;
     private final DeploymentAdminImpl m_admin;
+
     private volatile Command m_currentCommand = null;
     private volatile boolean m_cancelled;
 
@@ -54,10 +55,13 @@
     }
 
     /**
-     * Calling this method will cause the commands specified for this session to be executed. the commands will be rolled back if the session is
-     * canceled or if an exception is caused by one of the commands.
-     *
-     * @throws DeploymentException If the session was canceled (<code>DeploymentException.CODE_CANCELLED</code>) or if one of the commands caused an exception (<code>DeploymentException.*</code>)
+     * Calling this method will cause the commands specified for this session to
+     * be executed. the commands will be rolled back if the session is canceled
+     * or if an exception is caused by one of the commands.
+     * 
+     * @throws DeploymentException If the session was canceled (
+     *         <code>DeploymentException.CODE_CANCELLED</code>) or if one of the
+     *         commands caused an exception (<code>DeploymentException.*</code>)
      */
     public void call(boolean ignoreExceptions) throws DeploymentException {
         List executedCommands = new ArrayList();
@@ -74,6 +78,8 @@
             }
             catch (DeploymentException de) {
                 if (!ignoreExceptions) {
+                    // XXX catch exception and verify whether it is possible to
+                    // have exceptions during a rollback
                     rollback(executedCommands);
                     throw de;
                 }
@@ -94,8 +100,9 @@
 
     /**
      * Cancels the session if it is in progress.
-     *
-     * @return true if a session was in progress and now canceled, false otherwise.
+     * 
+     * @return true if a session was in progress and now canceled, false
+     *         otherwise.
      */
     public boolean cancel() {
         m_cancelled = true;
@@ -110,12 +117,12 @@
     }
 
     /**
-     * Retrieve the base directory of the persistent storage area according to 
+     * Retrieve the base directory of the persistent storage area according to
      * OSGi Core R4 6.1.6.10 for the given <code>BundleContext</code>.
      * 
      * @param bundle of which the storage area will be returned
      * @return a <code>File</code> that represents the base directory of the
-     *     persistent storage area for the bundle
+     *         persistent storage area for the bundle
      */
     public File getDataFile(Bundle bundle) {
         File result = null;
@@ -123,9 +130,9 @@
         BundleContext context = bundle.getBundleContext();
         if (context != null) {
             result = context.getDataFile("");
-        }
-        else {
-            // TODO this method should not return null or throw an exception; we need to resolve this...
+        } else {
+            // TODO this method should not return null or throw an exception; we
+            // need to resolve this...
             throw new IllegalStateException("Could not retrieve valid bundle context from bundle " + bundle.getSymbolicName());
         }
 
@@ -145,7 +152,7 @@
 
     /**
      * Returns the bundle context of the bundle this class is part of.
-     *
+     * 
      * @return The <code>BundleContext</code>.
      */
     public BundleContext getBundleContext() {
@@ -154,7 +161,7 @@
 
     /**
      * Returns the currently present log service.
-     *
+     * 
      * @return The <code>LogService</code>.
      */
     public LogService getLog() {
@@ -163,7 +170,7 @@
 
     /**
      * Returns the currently present package admin.
-     *
+     * 
      * @return The <code>PackageAdmin</code>
      */
     public PackageAdmin getPackageAdmin() {
@@ -171,8 +178,9 @@
     }
 
     /**
-     * Returns the target deployment package as an <code>AbstractDeploymentPackage</code>.
-     *
+     * Returns the target deployment package as an
+     * <code>AbstractDeploymentPackage</code>.
+     * 
      * @return The target deployment package of the session.
      */
     public AbstractDeploymentPackage getTargetAbstractDeploymentPackage() {
@@ -180,8 +188,9 @@
     }
 
     /**
-     * Returns the source deployment package as an <code>AbstractDeploymentPackage</code>.
-     *
+     * Returns the source deployment package as an
+     * <code>AbstractDeploymentPackage</code>.
+     * 
      * @return The source deployment packge of the session.
      */
     public AbstractDeploymentPackage getSourceAbstractDeploymentPackage() {
diff --git a/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/DropAllBundlesCommand.java b/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/DropAllBundlesCommand.java
index 6a59e33..bfe8a89 100644
--- a/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/DropAllBundlesCommand.java
+++ b/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/DropAllBundlesCommand.java
@@ -18,14 +18,11 @@
  */
 package org.apache.felix.deploymentadmin.spi;
 
-import java.io.IOException;
 import java.io.InputStream;
 
 import org.apache.felix.deploymentadmin.AbstractDeploymentPackage;
 import org.apache.felix.deploymentadmin.BundleInfoImpl;
 import org.osgi.framework.Bundle;
-import org.osgi.framework.BundleException;
-import org.osgi.service.deploymentadmin.DeploymentException;
 import org.osgi.service.log.LogService;
 
 /**
@@ -33,7 +30,7 @@
  */
 public class DropAllBundlesCommand extends Command {
 
-    public void execute(DeploymentSessionImpl session) throws DeploymentException {
+    protected void doExecute(DeploymentSessionImpl session) throws Exception {
         AbstractDeploymentPackage target = session.getTargetAbstractDeploymentPackage();
         LogService log = session.getLog();
 
@@ -49,14 +46,13 @@
                     addRollback(new InstallBundleRunnable(bundle, target, log));
                 }
             }
-            catch (BundleException be) {
+            catch (Exception be) {
                 log.log(LogService.LOG_WARNING, "Bundle '" + symbolicName + "' could not be uninstalled", be);
             }
         }
     }
 
-    private static class InstallBundleRunnable implements Runnable {
-
+    private static class InstallBundleRunnable extends AbstractAction {
         private final AbstractDeploymentPackage m_target;
         private final Bundle m_bundle;
         private final LogService m_log;
@@ -67,27 +63,23 @@
             m_log = log;
         }
 
-        public void run() {
+        protected void doRun() throws Exception {
             InputStream is = null;
             try {
                 is = m_target.getBundleStream(m_bundle.getSymbolicName());
                 if (is != null) {
                     m_bundle.update(is);
+                } else {
+                    throw new RuntimeException("Unable to get inputstream for bundle '" + m_bundle.getSymbolicName() + "'");
                 }
-                throw new Exception("Unable to get Inputstream for bundle " + m_bundle.getSymbolicName());
-            }
-            catch (Exception e) {
-                m_log.log(LogService.LOG_WARNING,
-                    "Could not rollback uninstallation of bundle '" + m_bundle.getSymbolicName() + "'", e);
             }
             finally {
-                if (is != null) {
-                    try {
-                        is.close();
-                    }
-                    catch (IOException e) {}
-                }
+                closeSilently(is);
             }
         }
+
+        protected void onFailure(Exception e) {
+            m_log.log(LogService.LOG_WARNING, "Could not rollback uninstallation of bundle '" + m_bundle.getSymbolicName() + "'", e);
+        }
     }
 }
diff --git a/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/DropAllResourcesCommand.java b/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/DropAllResourcesCommand.java
index 9b4a051..b918953 100644
--- a/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/DropAllResourcesCommand.java
+++ b/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/DropAllResourcesCommand.java
@@ -27,30 +27,30 @@
 import org.osgi.framework.ServiceReference;
 import org.osgi.service.deploymentadmin.DeploymentException;
 import org.osgi.service.deploymentadmin.spi.ResourceProcessor;
-import org.osgi.service.deploymentadmin.spi.ResourceProcessorException;
 import org.osgi.service.log.LogService;
 
 /**
  * Command that drops resources.
  */
 public class DropAllResourcesCommand extends Command {
-
     private final CommitResourceCommand m_commitCommand;
 
     /**
-     * Creates an instance of this command. The commit command is used to make sure
-     * the resource processors used to drop all resources will be committed at a later stage in the process.
-     *
-     * @param commitCommand The commit command that will be executed at a later stage in the process.
+     * Creates an instance of this command. The commit command is used to make
+     * sure the resource processors used to drop all resources will be committed
+     * at a later stage in the process.
+     * 
+     * @param commitCommand The commit command that will be executed at a later
+     *        stage in the process.
      */
     public DropAllResourcesCommand(CommitResourceCommand commitCommand) {
         m_commitCommand = commitCommand;
     }
 
-    public void execute(DeploymentSessionImpl session) throws DeploymentException {
+    protected void doExecute(DeploymentSessionImpl session) throws Exception {
         // Allow proper rollback in case the drop fails...
         addRollback(new RollbackCommitAction(session));
-        
+
         AbstractDeploymentPackage target = session.getTargetAbstractDeploymentPackage();
         BundleContext context = session.getBundleContext();
         LogService log = session.getLog();
@@ -61,7 +61,7 @@
         ResourceInfoImpl[] orderedTargetResources = target.getOrderedResourceInfos();
         for (int i = orderedTargetResources.length - 1; i >= 0; i--) {
             ResourceInfoImpl resourceInfo = orderedTargetResources[i];
-            
+
             String rpName = resourceInfo.getResourceProcessor();
             String path = resourceInfo.getPath();
 
@@ -78,34 +78,34 @@
                 log.log(LogService.LOG_ERROR, "Failed to find resource processor for '" + rpName + "'!");
                 throw new DeploymentException(DeploymentException.CODE_PROCESSOR_NOT_FOUND, "Failed to find resource processor '" + rpName + "'!");
             }
-            
+
             ResourceProcessor resourceProcessor = (ResourceProcessor) context.getService(ref);
             if (resourceProcessor == null) {
                 log.log(LogService.LOG_ERROR, "Failed to find resource processor for '" + rpName + "'!");
                 throw new DeploymentException(DeploymentException.CODE_PROCESSOR_NOT_FOUND, "Failed to find resource processor '" + rpName + "'!");
             }
-            
+
             try {
                 if (m_commitCommand.addResourceProcessor(resourceProcessor)) {
                     resourceProcessor.begin(session);
                 }
                 resourceProcessor.dropAllResources();
             }
-            catch (ResourceProcessorException e) {
+            catch (Exception e) {
                 log.log(LogService.LOG_ERROR, "Failed to drop all resources for resource processor '" + rpName + "'!", e);
                 throw new DeploymentException(DeploymentException.CODE_OTHER_ERROR, "Failed to drop all resources for resource processor '" + rpName + "'!", e);
             }
         }
     }
-    
-    private class RollbackCommitAction implements Runnable {
-        private final DeploymentSessionImpl m_session; 
-        
+
+    private class RollbackCommitAction extends AbstractAction {
+        private final DeploymentSessionImpl m_session;
+
         public RollbackCommitAction(DeploymentSessionImpl session) {
             m_session = session;
         }
-        
-        public void run() {
+
+        protected void doRun() {
             m_commitCommand.rollback(m_session);
         }
     }
diff --git a/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/DropBundleCommand.java b/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/DropBundleCommand.java
index 3f2ddd3..ee018d0 100644
--- a/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/DropBundleCommand.java
+++ b/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/DropBundleCommand.java
@@ -18,14 +18,11 @@
  */
 package org.apache.felix.deploymentadmin.spi;
 
-import java.io.IOException;
 import java.io.InputStream;
 
 import org.apache.felix.deploymentadmin.AbstractDeploymentPackage;
 import org.apache.felix.deploymentadmin.BundleInfoImpl;
 import org.osgi.framework.Bundle;
-import org.osgi.framework.BundleException;
-import org.osgi.service.deploymentadmin.DeploymentException;
 import org.osgi.service.log.LogService;
 
 /**
@@ -33,7 +30,7 @@
  */
 public class DropBundleCommand extends Command {
 
-    public void execute(DeploymentSessionImpl session) throws DeploymentException {
+    protected void doExecute(DeploymentSessionImpl session) throws Exception {
         AbstractDeploymentPackage target = session.getTargetAbstractDeploymentPackage();
         AbstractDeploymentPackage source = session.getSourceAbstractDeploymentPackage();
         LogService log = session.getLog();
@@ -41,23 +38,23 @@
         BundleInfoImpl[] orderedTargetBundles = target.getOrderedBundleInfos();
         for (int i = orderedTargetBundles.length - 1; i >= 0; i--) {
             BundleInfoImpl bundleInfo = orderedTargetBundles[i];
-            if (!bundleInfo.isCustomizer() && source.getBundleInfoByName(bundleInfo.getSymbolicName()) == null) {
+            String symbolicName = bundleInfo.getSymbolicName();
+
+            if (!bundleInfo.isCustomizer() && source.getBundleInfoByName(symbolicName) == null) {
                 // stale bundle, save a copy for rolling back and uninstall it
-                String symbolicName = bundleInfo.getSymbolicName();
                 try {
                     Bundle bundle = target.getBundle(symbolicName);
                     bundle.uninstall();
                     addRollback(new InstallBundleRunnable(bundle, target, log));
                 }
-                catch (BundleException be) {
+                catch (Exception be) {
                     log.log(LogService.LOG_WARNING, "Bundle '" + symbolicName + "' could not be uninstalled", be);
                 }
             }
         }
     }
 
-    private static class InstallBundleRunnable implements Runnable {
-
+    private static class InstallBundleRunnable extends AbstractAction {
         private final AbstractDeploymentPackage m_target;
         private final Bundle m_bundle;
         private final LogService m_log;
@@ -68,27 +65,23 @@
             m_log = log;
         }
 
-        public void run() {
+        protected void doRun() throws Exception {
             InputStream is = null;
             try {
                 is = m_target.getBundleStream(m_bundle.getSymbolicName());
                 if (is != null) {
                     m_bundle.update(is);
+                } else {
+                    throw new RuntimeException("Unable to get Inputstream for bundle '" + m_bundle.getSymbolicName() + "'");
                 }
-                throw new Exception("Unable to get Inputstream for bundle " + m_bundle.getSymbolicName());
-            }
-            catch (Exception e) {
-                m_log.log(LogService.LOG_WARNING,
-                    "Could not rollback uninstallation of bundle '" + m_bundle.getSymbolicName() + "'", e);
             }
             finally {
-                if (is != null) {
-                    try {
-                        is.close();
-                    }
-                    catch (IOException e) {}
-                }
+                closeSilently(is);
             }
         }
+
+        protected void onFailure(Exception e) {
+            m_log.log(LogService.LOG_WARNING, "Could not rollback uninstallation of bundle '" + m_bundle.getSymbolicName() + "'", e);
+        }
     }
 }
diff --git a/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/DropResourceCommand.java b/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/DropResourceCommand.java
index ae63cb8..c33a228 100644
--- a/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/DropResourceCommand.java
+++ b/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/DropResourceCommand.java
@@ -23,7 +23,6 @@
 import org.osgi.framework.BundleContext;
 import org.osgi.framework.ServiceReference;
 import org.osgi.service.deploymentadmin.spi.ResourceProcessor;
-import org.osgi.service.deploymentadmin.spi.ResourceProcessorException;
 import org.osgi.service.log.LogService;
 
 /**
@@ -34,19 +33,21 @@
     private final CommitResourceCommand m_commitCommand;
 
     /**
-     * Creates an instance of this command. The commit command is used to make sure
-     * the resource processors used to drop resources will be committed at a later stage in the process.
-     *
-     * @param commitCommand The commit command that will be executed at a later stage in the process.
+     * Creates an instance of this command. The commit command is used to make
+     * sure the resource processors used to drop resources will be committed at
+     * a later stage in the process.
+     * 
+     * @param commitCommand The commit command that will be executed at a later
+     *        stage in the process.
      */
     public DropResourceCommand(CommitResourceCommand commitCommand) {
         m_commitCommand = commitCommand;
     }
 
-    public void execute(DeploymentSessionImpl session) {
+    protected void doExecute(DeploymentSessionImpl session) throws Exception {
         // Allow proper rollback in case the drop fails...
         addRollback(new RollbackCommitAction(session));
-        
+
         AbstractDeploymentPackage target = session.getTargetAbstractDeploymentPackage();
         AbstractDeploymentPackage source = session.getSourceAbstractDeploymentPackage();
         BundleContext context = session.getBundleContext();
@@ -63,11 +64,11 @@
                     if (resourceProcessor != null) {
                         try {
                             if (m_commitCommand.addResourceProcessor(resourceProcessor)) {
-                            	resourceProcessor.begin(session);
+                                resourceProcessor.begin(session);
                             }
                             resourceProcessor.dropped(path);
                         }
-                        catch (ResourceProcessorException e) {
+                        catch (Exception e) {
                             log.log(LogService.LOG_WARNING, "Not allowed to drop resource '" + path + "'", e);
                         }
                     }
@@ -75,15 +76,15 @@
             }
         }
     }
-    
-    private class RollbackCommitAction implements Runnable {
-        private final DeploymentSessionImpl m_session; 
-        
+
+    private class RollbackCommitAction extends AbstractAction {
+        private final DeploymentSessionImpl m_session;
+
         public RollbackCommitAction(DeploymentSessionImpl session) {
             m_session = session;
         }
-        
-        public void run() {
+
+        protected void doRun() {
             m_commitCommand.rollback(m_session);
         }
     }
diff --git a/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/GetStorageAreaCommand.java b/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/GetStorageAreaCommand.java
index f7fa8a6..bc0093b 100644
--- a/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/GetStorageAreaCommand.java
+++ b/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/GetStorageAreaCommand.java
@@ -22,7 +22,6 @@
 import java.util.HashMap;
 import java.util.Map;
 
-
 import org.osgi.framework.Bundle;
 import org.osgi.service.deploymentadmin.BundleInfo;
 import org.osgi.service.deploymentadmin.DeploymentException;
@@ -30,14 +29,14 @@
 import org.osgi.service.log.LogService;
 
 /**
- * Command that determines the storage area's of all bundles in the source deployment
- * package of a deployment session.
+ * Command that determines the storage area's of all bundles in the source
+ * deployment package of a deployment session.
  */
 public class GetStorageAreaCommand extends Command {
 
     private final Map m_storageAreas = new HashMap();
 
-    public void execute(DeploymentSessionImpl session) throws DeploymentException {
+    protected void doExecute(DeploymentSessionImpl session) throws Exception {
         DeploymentPackage target = session.getTargetDeploymentPackage();
         BundleInfo[] infos = target.getBundleInfos();
         for (int i = 0; i < infos.length; i++) {
@@ -51,20 +50,21 @@
                     m_storageAreas.put(bundle.getSymbolicName(), root);
                 }
                 catch (IllegalStateException ise) {
-                    session.getLog().log(LogService.LOG_WARNING, "Could not get reference to storage area of bundle '" + bundle.getSymbolicName() +"'");
+                    session.getLog().log(LogService.LOG_WARNING, "Could not get reference to storage area of bundle '" + bundle.getSymbolicName() + "'");
                 }
             }
         }
     }
 
     /**
-     * Determines the storage area's of all bundles in the source deployment package of
-     * a deployment session.
-     *
-     * @return <code>Map</code> with <code>File</code> object references to the storage area's, they bundle symbolic name is used as a key in the <code>Map</code>.
+     * Determines the storage area's of all bundles in the source deployment
+     * package of a deployment session.
+     * 
+     * @return <code>Map</code> with <code>File</code> object references to the
+     *         storage area's, they bundle symbolic name is used as a key in the
+     *         <code>Map</code>.
      */
     public Map getStorageAreas() {
         return m_storageAreas;
     }
-
 }
diff --git a/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/ProcessResourceCommand.java b/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/ProcessResourceCommand.java
index 29d85fb..c4f4edf 100644
--- a/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/ProcessResourceCommand.java
+++ b/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/ProcessResourceCommand.java
@@ -32,29 +32,30 @@
 import org.osgi.service.deploymentadmin.spi.ResourceProcessorException;
 
 /**
- * Command that processes all the processed resources in the source deployment package
- * of a deployment session by finding their Resource Processors and having those process
- * the resources.
- * System property <code>org.apache.felix.deploymentadmin.allowforeigncustomizers</code> allows
- * you to skip the source handling of resource processors, allowing the use of processors already on
- * the system. Defaults to <code>false</code>.
+ * Command that processes all the processed resources in the source deployment
+ * package of a deployment session by finding their Resource Processors and
+ * having those process the resources. System property
+ * <code>org.apache.felix.deploymentadmin.allowforeigncustomizers</code> allows
+ * you to skip the source handling of resource processors, allowing the use of
+ * processors already on the system. Defaults to <code>false</code>.
  */
 public class ProcessResourceCommand extends Command {
 
     private final CommitResourceCommand m_commitCommand;
 
     /**
-     * Creates an instance of this command, the <code>CommitCommand</code> is used
-     * to ensure that all used <code>ResourceProcessor</code>s will be committed at a later
-     * stage in the deployment session.
-     *
-     * @param commitCommand The <code>CommitCommand</code> that will commit all resource processors used in this command.
+     * Creates an instance of this command, the <code>CommitCommand</code> is
+     * used to ensure that all used <code>ResourceProcessor</code>s will be
+     * committed at a later stage in the deployment session.
+     * 
+     * @param commitCommand The <code>CommitCommand</code> that will commit all
+     *        resource processors used in this command.
      */
     public ProcessResourceCommand(CommitResourceCommand commitCommand) {
         m_commitCommand = commitCommand;
     }
 
-    public void execute(DeploymentSessionImpl session) throws DeploymentException {
+    protected void doExecute(DeploymentSessionImpl session) throws Exception {
         // Allow proper rollback in case the drop fails...
         addRollback(new RollbackCommitAction(session));
 
@@ -73,17 +74,17 @@
         try {
             String allowForeignCustomerizers = System.getProperty("org.apache.felix.deploymentadmin.allowforeigncustomizers", "false");
 
-        	while (!expectedResources.isEmpty()) {
-            	AbstractInfo jarEntry = source.getNextEntry();
-            	if (jarEntry == null) {
-                	throw new DeploymentException(DeploymentException.CODE_OTHER_ERROR, "Expected more resources in the stream: " + expectedResources.keySet());
-            	}
-            	
-            	String name = jarEntry.getPath();
+            while (!expectedResources.isEmpty()) {
+                AbstractInfo jarEntry = source.getNextEntry();
+                if (jarEntry == null) {
+                    throw new DeploymentException(DeploymentException.CODE_OTHER_ERROR, "Expected more resources in the stream: " + expectedResources.keySet());
+                }
+
+                String name = jarEntry.getPath();
 
                 ResourceInfoImpl resourceInfo = (ResourceInfoImpl) expectedResources.remove(name);
                 if (resourceInfo == null) {
-                	throw new DeploymentException(DeploymentException.CODE_OTHER_ERROR, "Resource '" + name + "' is not described in the manifest.");
+                    throw new DeploymentException(DeploymentException.CODE_OTHER_ERROR, "Resource '" + name + "' is not described in the manifest.");
                 }
 
                 ServiceReference ref = source.getResourceProcessor(name);
@@ -94,28 +95,24 @@
                         if (resourceProcessor != null) {
                             try {
                                 if (m_commitCommand.addResourceProcessor(resourceProcessor)) {
-                                	resourceProcessor.begin(session);
+                                    resourceProcessor.begin(session);
                                 }
                                 resourceProcessor.process(name, source.getCurrentEntryStream());
                             }
                             catch (ResourceProcessorException rpe) {
                                 if (rpe.getCode() == ResourceProcessorException.CODE_RESOURCE_SHARING_VIOLATION) {
                                     throw new DeploymentException(DeploymentException.CODE_RESOURCE_SHARING_VIOLATION, "Sharing violation while processing resource '" + name + "'", rpe);
-                                }
-                                else {
+                                } else {
                                     throw new DeploymentException(DeploymentException.CODE_OTHER_ERROR, "Error while processing resource '" + name + "'", rpe);
                                 }
                             }
-                        }
-                        else {
+                        } else {
                             throw new DeploymentException(DeploymentException.CODE_PROCESSOR_NOT_FOUND, "No resource processor for resource: '" + name + "'");
                         }
-                    }
-                    else {
+                    } else {
                         throw new DeploymentException(DeploymentException.CODE_FOREIGN_CUSTOMIZER, "Resource processor for resource '" + name + "' belongs to foreign deployment package");
                     }
-                }
-                else {
+                } else {
                     throw new DeploymentException(DeploymentException.CODE_PROCESSOR_NOT_FOUND, "No resource processor for resource: '" + name + "'");
                 }
             }
@@ -124,15 +121,15 @@
             throw new DeploymentException(DeploymentException.CODE_OTHER_ERROR, "Problem while reading stream", e);
         }
     }
-    
-    private class RollbackCommitAction implements Runnable {
-        private final DeploymentSessionImpl m_session; 
-        
+
+    private class RollbackCommitAction extends AbstractAction {
+        private final DeploymentSessionImpl m_session;
+
         public RollbackCommitAction(DeploymentSessionImpl session) {
             m_session = session;
         }
-        
-        public void run() {
+
+        protected void doRun() {
             m_commitCommand.rollback(m_session);
         }
     }
diff --git a/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/SnapshotCommand.java b/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/SnapshotCommand.java
index 2fde4ed..1b6f589 100644
--- a/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/SnapshotCommand.java
+++ b/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/SnapshotCommand.java
@@ -44,7 +44,7 @@
         m_getStorageAreaCommand = getStorageAreaCommand;
     }
 
-    public void execute(DeploymentSessionImpl session) throws DeploymentException {
+    protected void doExecute(DeploymentSessionImpl session) throws Exception {
         AbstractDeploymentPackage target = session.getTargetAbstractDeploymentPackage();
         BundleContext context = session.getBundleContext();
 
@@ -54,9 +54,11 @@
             if (isCancelled()) {
                 throw new DeploymentException(DeploymentException.CODE_CANCELLED);
             }
-            Bundle bundle = target.getBundle(infos[i].getSymbolicName());
+
+            String symbolicName = infos[i].getSymbolicName();
+            Bundle bundle = target.getBundle(symbolicName);
             if (bundle != null) {
-                File root = (File) storageAreas.get(bundle.getSymbolicName());
+                File root = (File) storageAreas.get(symbolicName);
                 if (root != null) {
                     File snapshot = context.getDataFile("snapshots");
                     snapshot.mkdirs();
@@ -67,11 +69,12 @@
                         addRollback(new RestoreSnapshotRunnable(session, snapshot, root));
                         addCommit(new DeleteSnapshotRunnable(session, snapshot));
                     }
-                    catch (IOException e) {
+                    catch (Exception e) {
+                        session.getLog().log(LogService.LOG_WARNING, "Could not access storage area of bundle '" + symbolicName + "'!", e);
                         snapshot.delete();
                     }
                 } else {
-                    session.getLog().log(LogService.LOG_WARNING, "Could not retrieve storage area of bundle '" + bundle.getSymbolicName() + "', skipping it.");
+                    session.getLog().log(LogService.LOG_WARNING, "Could not retrieve storage area of bundle '" + symbolicName + "', skipping it.");
                 }
             }
         }
@@ -87,14 +90,7 @@
             }
         }
         finally {
-            if (output != null) {
-                try {
-                    output.close();
-                }
-                catch (Exception ex) {
-                    // Not much we can do
-                }
-            }
+            closeSilently(output);
         }
     }
 
@@ -106,8 +102,7 @@
             for (int i = 0; i < childs.length; i++) {
                 storeRecursive(childs[i], new File(path, childs[i].getName()), output);
             }
-        }
-        else {
+        } else {
             InputStream input = null;
             try {
                 input = new FileInputStream(current);
@@ -118,20 +113,14 @@
                 output.closeEntry();
             }
             finally {
-                try {
-                    if (input != null) {
-                        input.close();
-                    }
-                }
-                catch (Exception ex) {
-                    // Not much we can do
-                }
+                closeSilently(input);
             }
         }
     }
 
-    private static class DeleteSnapshotRunnable implements Runnable {
+    private static class DeleteSnapshotRunnable extends AbstractAction {
         private final DeploymentSessionImpl m_session;
+
         private final File m_snapshot;
 
         private DeleteSnapshotRunnable(DeploymentSessionImpl session, File snapshot) {
@@ -139,16 +128,18 @@
             m_snapshot = snapshot;
         }
 
-        public void run() {
+        protected void doRun() {
             if (!m_snapshot.delete()) {
                 m_session.getLog().log(LogService.LOG_WARNING, "Failed to delete snapshot in " + m_snapshot + "!");
             }
         }
     }
 
-    private static class RestoreSnapshotRunnable implements Runnable {
+    private static class RestoreSnapshotRunnable extends AbstractAction {
         private final DeploymentSessionImpl m_session;
+
         private final File m_snapshot;
+
         private final File m_root;
 
         private RestoreSnapshotRunnable(DeploymentSessionImpl session, File snapshot, File root) {
@@ -157,19 +148,20 @@
             m_root = root;
         }
 
-        public void run() {
+        protected void doRun() throws Exception {
             try {
                 delete(m_root, false);
                 unpack(m_snapshot, m_root);
             }
-            catch (Exception ex) {
-                m_session.getLog().log(LogService.LOG_WARNING, "Failed to restore snapshot!", ex);
-            }
             finally {
                 m_snapshot.delete();
             }
         }
 
+        protected void onFailure(Exception e) {
+            m_session.getLog().log(LogService.LOG_WARNING, "Failed to restore snapshot!", e);
+        }
+
         private void delete(File root, boolean deleteRoot) {
             if (root.isDirectory()) {
                 File[] childs = root.listFiles();
@@ -189,8 +181,7 @@
                 for (ZipEntry entry = input.getNextEntry(); entry != null; entry = input.getNextEntry()) {
                     if (entry.isDirectory()) {
                         (new File(target, entry.getName())).mkdirs();
-                    }
-                    else {
+                    } else {
                         OutputStream output = null;
                         try {
                             output = new FileOutputStream(target);
@@ -200,29 +191,15 @@
                             }
                         }
                         finally {
-                            if (output != null) {
-                                try {
-                                    output.close();
-                                }
-                                catch (Exception ex) {
-                                    // Not much we can do
-                                }
-                            }
+                            closeSilently(output);
                         }
                     }
                     input.closeEntry();
                 }
             }
             finally {
-                if (input != null) {
-                    try {
-                        input.close();
-                    }
-                    catch (Exception ex) {
-                        // Not much we can do
-                    }
-                }
+                closeSilently(input);
             }
         }
-   }
+    }
 }
\ No newline at end of file
diff --git a/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/StartBundleCommand.java b/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/StartBundleCommand.java
index 3df2169..ea9d4ae 100644
--- a/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/StartBundleCommand.java
+++ b/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/StartBundleCommand.java
@@ -21,19 +21,19 @@
 import org.apache.felix.deploymentadmin.AbstractDeploymentPackage;
 import org.apache.felix.deploymentadmin.BundleInfoImpl;
 import org.osgi.framework.Bundle;
-import org.osgi.framework.BundleException;
 import org.osgi.framework.FrameworkEvent;
 import org.osgi.framework.FrameworkListener;
 import org.osgi.service.log.LogService;
 import org.osgi.service.packageadmin.PackageAdmin;
 
 /**
- * Command that starts all bundles described in the source deployment package of a deployment session.
+ * Command that starts all bundles described in the source deployment package of
+ * a deployment session.
  */
 public class StartBundleCommand extends Command {
     private final RefreshPackagesMonitor m_refreshMonitor = new RefreshPackagesMonitor();
 
-    public void execute(DeploymentSessionImpl session) {
+    protected void doExecute(DeploymentSessionImpl session) throws Exception {
         AbstractDeploymentPackage source = session.getSourceAbstractDeploymentPackage();
         PackageAdmin packageAdmin = session.getPackageAdmin();
         RefreshPackagesListener listener = new RefreshPackagesListener();
@@ -49,29 +49,31 @@
         for (int i = 0; i < bundleInfos.length; i++) {
             BundleInfoImpl bundleInfoImpl = bundleInfos[i];
             if (!bundleInfoImpl.isCustomizer()) {
-                Bundle bundle = source.getBundle(bundleInfoImpl.getSymbolicName());
+                String symbolicName = bundleInfoImpl.getSymbolicName();
+
+                Bundle bundle = source.getBundle(symbolicName);
                 if (bundle != null) {
                     if (isFragmentBundle(bundle)) {
-                        log.log(LogService.LOG_INFO, "Skipping fragment bundle '" + bundle.getSymbolicName() + "'");
+                        log.log(LogService.LOG_INFO, "Skipping fragment bundle '" + symbolicName + "'");
                     } else {
                         try {
                             bundle.start();
                         }
-                        catch (BundleException be) {
-                            log.log(LogService.LOG_WARNING, "Could not start bundle '" + bundle.getSymbolicName() + "'", be);
+                        catch (Exception be) {
+                            log.log(LogService.LOG_WARNING, "Could not start bundle '" + symbolicName + "'", be);
                         }
                     }
-                }
-                else {
-                	log.log(LogService.LOG_WARNING, "Could not start bundle '" + bundleInfoImpl.getSymbolicName() + "' because it is not defined in the framework");
+                } else {
+                    log.log(LogService.LOG_WARNING, "Could not start bundle '" + symbolicName + "' because it is not present in the framework");
                 }
             }
         }
     }
 
     /**
-     * RefreshPackagesListener is only listing to FrameworkEvents of the type PACKAGES_REFRESHED. It will
-     * notify any object waiting the completion of a refreshpackages() call.
+     * RefreshPackagesListener is only listing to FrameworkEvents of the type
+     * PACKAGES_REFRESHED. It will notify any object waiting the completion of a
+     * refreshpackages() call.
      */
     private class RefreshPackagesListener implements FrameworkListener {
         public void frameworkEvent(FrameworkEvent event) {
@@ -82,7 +84,8 @@
     }
 
     /**
-     * Use this monitor when its desired to wait for the completion of the asynchronous PackageAdmin.refreshPackages() call.
+     * Use this monitor when its desired to wait for the completion of the
+     * asynchronous PackageAdmin.refreshPackages() call.
      */
     private static class RefreshPackagesMonitor {
         private static final int REFRESH_TIMEOUT = 10000;
@@ -90,11 +93,12 @@
         private volatile boolean m_alreadyNotified = false;
 
         /**
-         * Waits for the completion of the PackageAdmin.refreshPackages() call. Because
-         * its not sure whether all OSGi framework implementations implement this method as
-         * specified we have build in a timeout. So if a event about the completion of the
-         * refreshpackages() is never received, we continue after the timeout whether the refresh
-         * was done or not.
+         * Waits for the completion of the PackageAdmin.refreshPackages() call.
+         * Because its not sure whether all OSGi framework implementations
+         * implement this method as specified we have build in a timeout. So if
+         * a event about the completion of the refreshpackages() is never
+         * received, we continue after the timeout whether the refresh was done
+         * or not.
          */
         public synchronized void waitForRefresh() {
             if (!m_alreadyNotified) {
@@ -106,16 +110,16 @@
                 finally {
                     m_alreadyNotified = false;
                 }
-            }
-            else {
-                // just reset the misted notification variable, this Monitor object might be reused.
+            } else {
+                // just reset the misted notification variable, this Monitor
+                // object might be reused.
                 m_alreadyNotified = false;
             }
         }
 
         /**
-         * After a PACKAGES_REFRESHED event notify all the parties interested in the completion of
-         * the PackageAdmin.refreshPackages() call.
+         * After a PACKAGES_REFRESHED event notify all the parties interested in
+         * the completion of the PackageAdmin.refreshPackages() call.
          */
         public synchronized void proceed() {
             m_alreadyNotified = true;
diff --git a/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/StartCustomizerCommand.java b/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/StartCustomizerCommand.java
index 9675663..cf1e9ea 100644
--- a/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/StartCustomizerCommand.java
+++ b/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/StartCustomizerCommand.java
@@ -25,18 +25,18 @@
 import org.apache.felix.deploymentadmin.AbstractDeploymentPackage;
 import org.apache.felix.deploymentadmin.BundleInfoImpl;
 import org.osgi.framework.Bundle;
-import org.osgi.framework.BundleException;
 import org.osgi.service.deploymentadmin.DeploymentException;
 import org.osgi.service.log.LogService;
 
 /**
- * Command that starts all customizer bundles defined in the source deployment packages of a deployment
- * session. In addition all customizer bundles of the target deployment package that are not present in the source
- * deployment package are started as well.
+ * Command that starts all customizer bundles defined in the source deployment
+ * packages of a deployment session. In addition all customizer bundles of the
+ * target deployment package that are not present in the source deployment
+ * package are started as well.
  */
 public class StartCustomizerCommand extends Command {
 
-    public void execute(DeploymentSessionImpl session) throws DeploymentException {
+    protected void doExecute(DeploymentSessionImpl session) throws Exception {
         AbstractDeploymentPackage target = session.getTargetAbstractDeploymentPackage();
         AbstractDeploymentPackage source = session.getSourceAbstractDeploymentPackage();
 
@@ -69,15 +69,16 @@
             try {
                 bundle.start();
             }
-            catch (BundleException be) {
+            catch (Exception be) {
                 throw new DeploymentException(DeploymentException.CODE_OTHER_ERROR, "Could not start customizer bundle '" + bundle.getSymbolicName() + "'", be);
             }
             addRollback(new StopCustomizerRunnable(session, bundle));
         }
     }
 
-    private static class StopCustomizerRunnable implements Runnable {
+    private static class StopCustomizerRunnable extends AbstractAction {
         private final DeploymentSessionImpl m_session;
+
         private final Bundle m_bundle;
 
         public StopCustomizerRunnable(DeploymentSessionImpl session, Bundle bundle) {
@@ -85,13 +86,12 @@
             m_bundle = bundle;
         }
 
-        public void run() {
-            try {
-                m_bundle.stop();
-            }
-            catch (BundleException e) {
-                m_session.getLog().log(LogService.LOG_WARNING, "Failed to stop bundle '" + m_bundle.getSymbolicName() + "'", e);
-            }
+        protected void doRun() throws Exception {
+            m_bundle.stop();
+        }
+
+        protected void onFailure(Exception e) {
+            m_session.getLog().log(LogService.LOG_WARNING, "Failed to stop bundle '" + m_bundle.getSymbolicName() + "'", e);
         }
     }
 }
diff --git a/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/StopBundleCommand.java b/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/StopBundleCommand.java
index b458842..fa75d90 100644
--- a/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/StopBundleCommand.java
+++ b/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/StopBundleCommand.java
@@ -21,27 +21,30 @@
 import org.apache.felix.deploymentadmin.AbstractDeploymentPackage;
 import org.apache.felix.deploymentadmin.BundleInfoImpl;
 import org.osgi.framework.Bundle;
-import org.osgi.framework.BundleException;
 import org.osgi.service.deploymentadmin.BundleInfo;
 import org.osgi.service.deploymentadmin.DeploymentException;
 import org.osgi.service.log.LogService;
 
 /**
- * Command that stops all bundles described in the target deployment package of a deployment session.
+ * Command that stops all bundles described in the target deployment package of
+ * a deployment session.
  * 
- * By spec every single bundle of the target package should be stopped, even if this is not strictly necessary 
- * because of bundles being unaffected by an update. To be able to skip the stopping of unaffected bundles the 
- * following system property can be defined: <code>org.apache.felix.deploymentadmin.stopunaffectedbundle</code>.
- * If this property has value <code>false</code> (case insensitive) then unaffected bundles will not be stopped, 
- * in all other cases the bundles will be stopped according to the OSGi specification.
+ * By spec every single bundle of the target package should be stopped, even if
+ * this is not strictly necessary because of bundles being unaffected by an
+ * update. To be able to skip the stopping of unaffected bundles the following
+ * system property can be defined:
+ * <code>org.apache.felix.deploymentadmin.stopunaffectedbundle</code>. If this
+ * property has value <code>false</code> (case insensitive) then unaffected
+ * bundles will not be stopped, in all other cases the bundles will be stopped
+ * according to the OSGi specification.
  */
 public class StopBundleCommand extends Command {
 
-    public void execute(DeploymentSessionImpl session) throws DeploymentException {
+    protected void doExecute(DeploymentSessionImpl session) throws Exception {
         String stopUnaffectedBundle = System.getProperty("org.apache.felix.deploymentadmin.stopunaffectedbundle", "true");
 
         LogService log = session.getLog();
-        
+
         AbstractDeploymentPackage target = session.getTargetAbstractDeploymentPackage();
         BundleInfo[] bundleInfos = target.getOrderedBundleInfos();
         for (int i = 0; i < bundleInfos.length; i++) {
@@ -49,25 +52,24 @@
                 throw new DeploymentException(DeploymentException.CODE_CANCELLED);
             }
             String symbolicName = bundleInfos[i].getSymbolicName();
-			Bundle bundle = target.getBundle(symbolicName);
+            Bundle bundle = target.getBundle(symbolicName);
             if (bundle != null) {
-        		if ("false".equalsIgnoreCase(stopUnaffectedBundle) && omitBundleStop(session, symbolicName)) {
-        			continue;
-        		}
-        		if (isFragmentBundle(bundle)) {
-                    log.log(LogService.LOG_INFO, "Skipping fragment bundle '" + bundle.getSymbolicName() + "'");
-        		} else {
-        		    addRollback(new StartBundleRunnable(session, bundle));
-        		    try {
-        		        bundle.stop();
-        		    }
-        		    catch (BundleException e) {
-        		        log.log(LogService.LOG_WARNING, "Could not stop bundle '" + bundle.getSymbolicName() + "'", e);
-        		    }
-        		}
-            }
-            else {
-            	log.log(LogService.LOG_WARNING, "Could not stop bundle '" + symbolicName + "' because it was not defined int he framework");
+                if ("false".equalsIgnoreCase(stopUnaffectedBundle) && omitBundleStop(session, symbolicName)) {
+                    continue;
+                }
+                if (isFragmentBundle(bundle)) {
+                    log.log(LogService.LOG_INFO, "Skipping fragment bundle '" + symbolicName + "'");
+                } else {
+                    addRollback(new StartBundleRunnable(session, bundle));
+                    try {
+                        bundle.stop();
+                    }
+                    catch (Exception e) {
+                        log.log(LogService.LOG_WARNING, "Could not stop bundle '" + symbolicName + "'", e);
+                    }
+                }
+            } else {
+                log.log(LogService.LOG_WARNING, "Could not stop bundle '" + symbolicName + "' because it was not present in the framework");
             }
         }
     }
@@ -78,24 +80,27 @@
      * @param session The current deployment session.
      * @param symbolicName The symbolic name of the bundle to inspect.
      * 
-     * @return Returns <code>true</code> if <code>Constants.DEPLOYMENTPACKAGE_MISSING</code> is true for the specified bundle in the
-     * source deployment package or if the version of the bundle is the same in both source and target deployment package. Returns 
-     * <code>false</code> otherwise.
+     * @return Returns <code>true</code> if
+     *         <code>Constants.DEPLOYMENTPACKAGE_MISSING</code> is true for the
+     *         specified bundle in the source deployment package or if the
+     *         version of the bundle is the same in both source and target
+     *         deployment package. Returns <code>false</code> otherwise.
      */
     private boolean omitBundleStop(DeploymentSessionImpl session, String symbolicName) {
-    	boolean result = false;
-		BundleInfoImpl sourceBundleInfo = session.getSourceAbstractDeploymentPackage().getBundleInfoByName(symbolicName);
-		BundleInfoImpl targetBundleInfo = session.getTargetAbstractDeploymentPackage().getBundleInfoByName(symbolicName);
-		boolean fixPackageMissing = sourceBundleInfo != null  && sourceBundleInfo.isMissing();
-		boolean sameVersion = (targetBundleInfo != null && sourceBundleInfo != null && targetBundleInfo.getVersion().equals(sourceBundleInfo.getVersion()));
-		if (fixPackageMissing || sameVersion) {
-			result = true;
-		}
-		return result;
-	}
+        boolean result = false;
+        BundleInfoImpl sourceBundleInfo = session.getSourceAbstractDeploymentPackage().getBundleInfoByName(symbolicName);
+        BundleInfoImpl targetBundleInfo = session.getTargetAbstractDeploymentPackage().getBundleInfoByName(symbolicName);
+        boolean fixPackageMissing = sourceBundleInfo != null && sourceBundleInfo.isMissing();
+        boolean sameVersion = (targetBundleInfo != null && sourceBundleInfo != null && targetBundleInfo.getVersion().equals(sourceBundleInfo.getVersion()));
+        if (fixPackageMissing || sameVersion) {
+            result = true;
+        }
+        return result;
+    }
 
-	private static class StartBundleRunnable implements Runnable {
+    private static class StartBundleRunnable extends AbstractAction {
         private final DeploymentSessionImpl m_session;
+
         private final Bundle m_bundle;
 
         public StartBundleRunnable(DeploymentSessionImpl session, Bundle bundle) {
@@ -103,14 +108,12 @@
             m_bundle = bundle;
         }
 
-        public void run() {
-            try {
-                m_bundle.start();
-            }
-            catch (BundleException e) {
-                m_session.getLog().log(LogService.LOG_WARNING, "Failed to start bundle '" + m_bundle.getSymbolicName() + "'", e);
-            }
+        protected void doRun() throws Exception {
+            m_bundle.start();
+        }
+
+        protected void onFailure(Exception e) {
+            m_session.getLog().log(LogService.LOG_WARNING, "Failed to start bundle '" + m_bundle.getSymbolicName() + "'", e);
         }
     }
 }
-
diff --git a/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/UpdateCommand.java b/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/UpdateCommand.java
index a5a08a5..2fca3bc 100644
--- a/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/UpdateCommand.java
+++ b/deploymentadmin/deploymentadmin/src/main/java/org/apache/felix/deploymentadmin/spi/UpdateCommand.java
@@ -29,19 +29,19 @@
 import org.apache.felix.deploymentadmin.Constants;
 import org.osgi.framework.Bundle;
 import org.osgi.framework.BundleContext;
-import org.osgi.framework.BundleException;
 import org.osgi.framework.Version;
 import org.osgi.service.deploymentadmin.DeploymentException;
 import org.osgi.service.log.LogService;
 
 /**
- * Command that installs all bundles described in the source deployment package of a deployment
- * session. If a bundle was already defined in the target deployment package of the same session
- * it is updated, otherwise the bundle is simply installed.
+ * Command that installs all bundles described in the source deployment package
+ * of a deployment session. If a bundle was already defined in the target
+ * deployment package of the same session it is updated, otherwise the bundle is
+ * simply installed.
  */
 public class UpdateCommand extends Command {
 
-    public void execute(DeploymentSessionImpl session) throws DeploymentException {
+    protected void doExecute(DeploymentSessionImpl session) throws Exception {
         AbstractDeploymentPackage source = session.getSourceAbstractDeploymentPackage();
         AbstractDeploymentPackage targetPackage = session.getTargetAbstractDeploymentPackage();
         BundleContext context = session.getBundleContext();
@@ -58,44 +58,49 @@
 
         try {
             while (!expectedBundles.isEmpty()) {
-            	AbstractInfo entry = source.getNextEntry();
-            	if (entry == null) {
-                	throw new DeploymentException(DeploymentException.CODE_OTHER_ERROR, "Expected more bundles in the stream: " + expectedBundles.keySet());
-            	}
-            	
-            	String name = entry.getPath();
+                AbstractInfo entry = source.getNextEntry();
+                if (entry == null) {
+                    throw new DeploymentException(DeploymentException.CODE_OTHER_ERROR, "Expected more bundles in the stream: " + expectedBundles.keySet());
+                }
+
+                String name = entry.getPath();
                 BundleInfoImpl bundleInfo = (BundleInfoImpl) expectedBundles.remove(name);
                 if (bundleInfo == null) {
                     throw new DeploymentException(DeploymentException.CODE_OTHER_ERROR, "Resource '" + name + "' is not described in the manifest.");
                 }
 
-                Bundle bundle = targetPackage.getBundle(bundleInfo.getSymbolicName());
+                String bsn = bundleInfo.getSymbolicName();
+                Version sourceVersion = bundleInfo.getVersion();
+
+                Bundle bundle = targetPackage.getBundle(bsn);
                 try {
                     if (bundle == null) {
                         // new bundle, install it
-                        bundle = context.installBundle(Constants.BUNDLE_LOCATION_PREFIX + bundleInfo.getSymbolicName(), new BundleInputStream(source.getCurrentEntryStream()));
+                        bundle = context.installBundle(Constants.BUNDLE_LOCATION_PREFIX + bsn, new BundleInputStream(source.getCurrentEntryStream()));
                         addRollback(new UninstallBundleRunnable(bundle, log));
                     } else {
                         // existing bundle, update it
-                        Version sourceVersion = bundleInfo.getVersion();
-                        Version targetVersion = Version.parseVersion((String) bundle.getHeaders().get(org.osgi.framework.Constants.BUNDLE_VERSION));
-                        if (!sourceVersion.equals(targetVersion)) {
+                        Version currentVersion = getVersion(bundle);
+                        if (!sourceVersion.equals(currentVersion)) {
                             bundle.update(new BundleInputStream(source.getCurrentEntryStream()));
                             addRollback(new UpdateBundleRunnable(bundle, targetPackage, log));
                         }
                     }
                 }
-                catch (BundleException be) {
+                catch (Exception be) {
                     if (isCancelled()) {
                         return;
                     }
-                    throw new DeploymentException(DeploymentException.CODE_OTHER_ERROR, "Could not install new bundle '" + name + "'", be);
+                    throw new DeploymentException(DeploymentException.CODE_OTHER_ERROR, "Could not install new bundle '" + name + "' (" + bsn + ")", be);
                 }
-                if (!bundle.getSymbolicName().equals(bundleInfo.getSymbolicName())) {
-                    throw new DeploymentException(DeploymentException.CODE_BUNDLE_NAME_ERROR, "Installed/updated bundle symbolicname do not match what was installed/updated");
+
+                if (!bundle.getSymbolicName().equals(bsn)) {
+                    throw new DeploymentException(DeploymentException.CODE_BUNDLE_NAME_ERROR, "Installed/updated bundle symbolicname (" + bundle.getSymbolicName() + ") do not match what was installed/updated: " + bsn);
                 }
-                if (!Version.parseVersion((String) bundle.getHeaders().get(org.osgi.framework.Constants.BUNDLE_VERSION)).equals(bundleInfo.getVersion())) {
-                    throw new DeploymentException(DeploymentException.CODE_OTHER_ERROR, "Installed/updated bundle version do not match what was installed/updated");
+
+                Version targetVersion = getVersion(bundle);
+                if (!sourceVersion.equals(targetVersion)) {
+                    throw new DeploymentException(DeploymentException.CODE_OTHER_ERROR, "Installed/updated bundle version (" + targetVersion + ") do not match what was installed/updated: " + sourceVersion + ", offending bundle = " + bsn);
                 }
             }
         }
@@ -104,7 +109,11 @@
         }
     }
 
-    private static class UninstallBundleRunnable implements Runnable {
+    private Version getVersion(Bundle bundle) {
+        return Version.parseVersion((String) bundle.getHeaders().get(org.osgi.framework.Constants.BUNDLE_VERSION));
+    }
+
+    private static class UninstallBundleRunnable extends AbstractAction {
         private final Bundle m_bundle;
         private final LogService m_log;
 
@@ -113,17 +122,16 @@
             m_log = log;
         }
 
-        public void run() {
-            try {
-                m_bundle.uninstall();
-            }
-            catch (BundleException e) {
-                m_log.log(LogService.LOG_WARNING, "Could not rollback update of bundle '" + m_bundle.getSymbolicName() + "'", e);
-            }
+        protected void doRun() throws Exception {
+            m_bundle.uninstall();
+        }
+
+        protected void onFailure(Exception e) {
+            m_log.log(LogService.LOG_WARNING, "Could not rollback update of bundle '" + m_bundle.getSymbolicName() + "'", e);
         }
     }
 
-    private static class UpdateBundleRunnable implements Runnable {
+    private static class UpdateBundleRunnable extends AbstractAction {
         private final AbstractDeploymentPackage m_targetPackage;
         private final Bundle m_bundle;
         private final LogService m_log;
@@ -134,29 +142,24 @@
             m_log = log;
         }
 
-        public void run() {
+        protected void doRun() throws Exception {
             InputStream is = null;
             try {
                 is = m_targetPackage.getBundleStream(m_bundle.getSymbolicName());
                 if (is != null) {
                     m_bundle.update(is);
+                } else {
+                    throw new RuntimeException("Unable to get inputstream for bundle " + m_bundle.getSymbolicName());
                 }
-                throw new Exception("Unable to get Inputstream for bundle " + m_bundle.getSymbolicName());
-            }
-            catch (Exception e) {
-                m_log.log(LogService.LOG_WARNING, "Could not rollback update of bundle '" + m_bundle.getSymbolicName() + "'", e);
             }
             finally {
-                if (is != null) {
-                    try {
-                        is.close();
-                    }
-                    catch (IOException e) {
-                        e.printStackTrace();
-                    }
-                }
+                closeSilently(is);
             }
         }
+
+        protected void onFailure(Exception e) {
+            m_log.log(LogService.LOG_WARNING, "Could not rollback update of bundle '" + m_bundle.getSymbolicName() + "'", e);
+        }
     }
 
     private final class BundleInputStream extends InputStream {
diff --git a/deploymentadmin/itest/pom.xml b/deploymentadmin/itest/pom.xml
index 7b89b57..05e42d9 100644
--- a/deploymentadmin/itest/pom.xml
+++ b/deploymentadmin/itest/pom.xml
@@ -52,7 +52,7 @@
 		<dependency>
 			<groupId>org.apache.felix</groupId>
 			<artifactId>org.apache.felix.deploymentadmin</artifactId>
-			<version>0.9.5-SNAPSHOT</version>
+			<version>0.9.6-SNAPSHOT</version>
 			<scope>test</scope>
 		</dependency>
 		<dependency>
diff --git a/deploymentadmin/itest/src/test/java/org/apache/felix/deploymentadmin/itest/InstallDeploymentPackageTest.java b/deploymentadmin/itest/src/test/java/org/apache/felix/deploymentadmin/itest/InstallDeploymentPackageTest.java
index ed28e24..3570358 100644
--- a/deploymentadmin/itest/src/test/java/org/apache/felix/deploymentadmin/itest/InstallDeploymentPackageTest.java
+++ b/deploymentadmin/itest/src/test/java/org/apache/felix/deploymentadmin/itest/InstallDeploymentPackageTest.java
@@ -23,6 +23,7 @@
 import org.junit.runner.RunWith;
 import org.ops4j.pax.exam.junit.PaxExam;
 import org.osgi.framework.Bundle;
+import org.osgi.service.deploymentadmin.DeploymentException;
 import org.osgi.service.deploymentadmin.DeploymentPackage;
 
 /**
@@ -32,6 +33,32 @@
 public class InstallDeploymentPackageTest extends BaseIntegrationTest {
 
     /**
+     * FELIX-4409/4410 - test the installation of an invalid deployment package.
+     */
+    @Test
+    public void testInstallInvalidDeploymentPackageFail() throws Exception {
+        DeploymentPackageBuilder dpBuilder = createNewDeploymentPackageBuilder("1.0.0");
+        // incluse two different versions of the same bundle (with the same BSN), this is *not* allowed per the DA spec...
+        dpBuilder
+            .add(dpBuilder.createBundleResource().setUrl(getTestBundle("bundleapi1", "bundleapi", "1.0.0")))
+            .add(dpBuilder.createBundleResource().setUrl(getTestBundle("bundleapi2", "bundleapi", "2.0.0")));
+
+        try
+        {
+            m_deploymentAdmin.installDeploymentPackage(dpBuilder.generate());
+            fail("DeploymentException expected!");
+        }
+        catch (DeploymentException e)
+        {
+            // Ok; expected...
+        }
+        
+        // Verify that none of the bundles are installed...
+        assertBundleNotExists("testbundles.bundleapi", "1.0.0");
+        assertBundleNotExists("testbundles.bundleapi", "2.0.0");
+    }
+
+    /**
      * Tests that adding the dependency for a bundle in an update package causes the depending bundle to be resolved and started.
      */
     @Test
diff --git a/deploymentadmin/testbundles/bundle2/src/main/java/org/apache/felix/deploymentadmin/test/bundle2/impl/Activator.java b/deploymentadmin/testbundles/bundle2/src/main/java/org/apache/felix/deploymentadmin/test/bundle2/impl/Activator.java
index cc17983..8c8de7a 100644
--- a/deploymentadmin/testbundles/bundle2/src/main/java/org/apache/felix/deploymentadmin/test/bundle2/impl/Activator.java
+++ b/deploymentadmin/testbundles/bundle2/src/main/java/org/apache/felix/deploymentadmin/test/bundle2/impl/Activator.java
@@ -43,16 +43,14 @@
     }
 
     public Object addingService(ServiceReference reference) {
-        Object service = m_context.getService(reference);
-        System.out.println("Service added: " + service);
-        return service;
+        return null;
     }
 
     public void modifiedService(ServiceReference reference, Object service) {
-        System.out.println("Service modified: " + service);
+        // Nop
     }
 
     public void removedService(ServiceReference reference, Object service) {
-        System.out.println("Service removed: " + service);
+        // Nop
     }
 }