Bug 547830 - fix possible deadlock in EnhancedExecutor.await()

The count used in EnhancedExecutor is dangerous because it increments
before dispatching work to the executor.  If any exception happens while
dispatching the task then the count will not be decremented.  This
changes the code to use futures instead to see when the work is done.

Change-Id: Ie042d0c269cffa95dc61d917f0c1a873ba448294
Signed-off-by: Thomas Watson <tjwatson@us.ibm.com>

6 files changed, 45 insertions, 43 deletions

diff --git a/bundles/org.eclipse.osgi.tests/META-INF/MANIFEST.MF b/bundles/org.eclipse.osgi.tests/META-INF/MANIFEST.MF
index 58c45dd4e..2524696c8 100644
--- a/bundles/org.eclipse.osgi.tests/META-INF/MANIFEST.MF
+++ b/bundles/org.eclipse.osgi.tests/META-INF/MANIFEST.MF
@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: Core OSGi Tests
 Bundle-SymbolicName: org.eclipse.osgi.tests;singleton:=true
-Bundle-Version: 3.13.500.qualifier
+Bundle-Version: 3.13.600.qualifier
 Bundle-Vendor: Eclipse.org
 Bundle-Localization: plugin
 Require-Bundle: 
diff --git a/bundles/org.eclipse.osgi.tests/pom.xml b/bundles/org.eclipse.osgi.tests/pom.xml
index 20e87650f..8c07556e5 100644
--- a/bundles/org.eclipse.osgi.tests/pom.xml
+++ b/bundles/org.eclipse.osgi.tests/pom.xml
@@ -19,7 +19,7 @@
   </parent>
   <groupId>org.eclipse.osgi</groupId>
   <artifactId>org.eclipse.osgi.tests</artifactId>
-  <version>3.13.500-SNAPSHOT</version>
+  <version>3.13.600-SNAPSHOT</version>
   <packaging>eclipse-test-plugin</packaging>
 
   <properties>
diff --git a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/container/TestModuleContainer.java b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/container/TestModuleContainer.java
index a2900f742..3e83af0f1 100644
--- a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/container/TestModuleContainer.java
+++ b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/container/TestModuleContainer.java
@@ -1832,7 +1832,7 @@ public class TestModuleContainer extends AbstractTest {
 		Assert.assertEquals("n3 should resolve.", State.RESOLVED, uses_n3.getState());
 	}
 
-	// DISABLE see bug 498064 @Test
+	@Test
 	public void testUsesTimeout() throws BundleException {
 		// Always want to go to zero threads when idle
 		int coreThreads = 0;
diff --git a/bundles/org.eclipse.osgi/META-INF/MANIFEST.MF b/bundles/org.eclipse.osgi/META-INF/MANIFEST.MF
index 1f3e297a4..fc081bfdb 100644
--- a/bundles/org.eclipse.osgi/META-INF/MANIFEST.MF
+++ b/bundles/org.eclipse.osgi/META-INF/MANIFEST.MF
@@ -101,7 +101,7 @@ Bundle-Activator: org.eclipse.osgi.internal.framework.SystemBundleActivator
 Bundle-Description: %systemBundle
 Bundle-Copyright: %copyright
 Bundle-Vendor: %eclipse.org
-Bundle-Version: 3.14.0.qualifier
+Bundle-Version: 3.14.100.qualifier
 Bundle-Localization: systembundle
 Bundle-DocUrl: http://www.eclipse.org
 Eclipse-ExtensibleAPI: true
diff --git a/bundles/org.eclipse.osgi/felix/src/org/apache/felix/resolver/ResolverImpl.java b/bundles/org.eclipse.osgi/felix/src/org/apache/felix/resolver/ResolverImpl.java
index 43975c7bf..3ec67c4c5 100755
--- a/bundles/org.eclipse.osgi/felix/src/org/apache/felix/resolver/ResolverImpl.java
+++ b/bundles/org.eclipse.osgi/felix/src/org/apache/felix/resolver/ResolverImpl.java
@@ -2448,8 +2448,8 @@ public class ResolverImpl implements Resolver
     private static class EnhancedExecutor
     {
         private final Executor executor;
-        private final AtomicInteger count = new AtomicInteger();
-        private Throwable throwable;
+        private final Queue<Future<Void>> awaiting = new ConcurrentLinkedQueue<>();
+        private final AtomicReference<Throwable> throwable = new AtomicReference<>();
 
         public EnhancedExecutor(Executor executor)
         {
@@ -2458,8 +2458,7 @@ public class ResolverImpl implements Resolver
 
         public void execute(final Runnable runnable)
         {
-            count.incrementAndGet();
-            executor.execute(new Runnable()
+            FutureTask<Void> task = new FutureTask(new Runnable()
             {
                 public void run()
                 {
@@ -2469,59 +2468,62 @@ public class ResolverImpl implements Resolver
                     }
                     catch (Throwable t)
                     {
-                        synchronized (count)
-                        {
-                            if (throwable == null)
-                            {
-                                throwable = t;
-                            }
-                        }
-                    }
-                    finally
-                    {
-                        if (count.decrementAndGet() == 0)
-                        {
-                            synchronized (count)
-                            {
-                                count.notifyAll();
-                            }
-                        }
+                        throwable.compareAndSet(null, t);
                     }
                 }
-            });
+            }, (Void) null);
+            // must have a happens-first to add the task to awaiting
+            awaiting.add(task);
+            try
+            {
+                executor.execute(task);
+            }
+            catch (Throwable t)
+            {
+                // if the task did not get added successfully to the executor we must cancel
+                // the task so we don't await on it
+                task.cancel(false);
+                throwable.compareAndSet(null, t);
+            }
         }
 
         public void await()
         {
-            synchronized (count)
+            Future<Void> awaitTask;
+            while (throwable.get() == null && (awaitTask = awaiting.poll()) != null)
             {
-                if (count.get() > 0)
+                if (!awaitTask.isDone() && !awaitTask.isCancelled())
                 {
                     try
                     {
-                        count.wait();
+                        awaitTask.get();
+                    }
+                    catch (CancellationException e)
+                    {
+                        // ignore; will have throwable set
                     }
                     catch (InterruptedException e)
                     {
                         throw new IllegalStateException(e);
                     }
-                    if (throwable != null)
+                    catch (ExecutionException e)
                     {
-                        if (throwable instanceof RuntimeException)
-                        {
-                            throw (RuntimeException) throwable;
-                        }
-                        else if (throwable instanceof Error)
-                        {
-                            throw (Error) throwable;
-                        }
-                        else
-                        {
-                            throw new RuntimeException(throwable);
-                        }
+                        throw new RuntimeException(e.getCause());
                     }
                 }
             }
+            Throwable t = throwable.get();
+            if (t!= null) {
+                if (t instanceof Runnable)
+                {
+                    throw (RuntimeException) t;
+                }
+                else if (t instanceof Error)
+                {
+                    throw (Error) t;
+                }
+                throw new RuntimeException(t);
+            }
         }
     }
 
diff --git a/bundles/org.eclipse.osgi/pom.xml b/bundles/org.eclipse.osgi/pom.xml
index dd81d86ba..9ae03a65b 100644
--- a/bundles/org.eclipse.osgi/pom.xml
+++ b/bundles/org.eclipse.osgi/pom.xml
@@ -19,7 +19,7 @@
   </parent>
   <groupId>org.eclipse.osgi</groupId>
   <artifactId>org.eclipse.osgi</artifactId>
-  <version>3.14.0-SNAPSHOT</version>
+  <version>3.14.100-SNAPSHOT</version>
   <packaging>eclipse-plugin</packaging>
 
   <build>