You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@felix.apache.org by pd...@apache.org on 2017/01/02 08:20:08 UTC
svn commit: r1776903 - in /felix/trunk/dependencymanager:
org.apache.felix.dependencymanager.itest/src/org/apache/felix/dm/itest/api/
org.apache.felix.dependencymanager/src/org/apache/felix/dm/
org.apache.felix.dependencymanager/src/org/apache/felix/dm...
Author: pderop
Date: Mon Jan 2 08:20:08 2017
New Revision: 1776903
URL: http://svn.apache.org/viewvc?rev=1776903&view=rev
Log:
FELIX-5471: Removed the timeout guard blocking code in ComponentImpl.schedule method because it may produces a deadlock (see FELIX5471_CyclicDependencyTest.java).
Added:
felix/trunk/dependencymanager/org.apache.felix.dependencymanager.itest/src/org/apache/felix/dm/itest/api/FELIX5471_CyclicDependencyTest.java
Modified:
felix/trunk/dependencymanager/org.apache.felix.dependencymanager/src/org/apache/felix/dm/DependencyManager.java
felix/trunk/dependencymanager/org.apache.felix.dependencymanager/src/org/apache/felix/dm/impl/ComponentImpl.java
felix/trunk/dependencymanager/org.apache.felix.dependencymanager/src/org/apache/felix/dm/impl/InvocationUtil.java
Added: felix/trunk/dependencymanager/org.apache.felix.dependencymanager.itest/src/org/apache/felix/dm/itest/api/FELIX5471_CyclicDependencyTest.java
URL: http://svn.apache.org/viewvc/felix/trunk/dependencymanager/org.apache.felix.dependencymanager.itest/src/org/apache/felix/dm/itest/api/FELIX5471_CyclicDependencyTest.java?rev=1776903&view=auto
==============================================================================
--- felix/trunk/dependencymanager/org.apache.felix.dependencymanager.itest/src/org/apache/felix/dm/itest/api/FELIX5471_CyclicDependencyTest.java (added)
+++ felix/trunk/dependencymanager/org.apache.felix.dependencymanager.itest/src/org/apache/felix/dm/itest/api/FELIX5471_CyclicDependencyTest.java Mon Jan 2 08:20:08 2017
@@ -0,0 +1,108 @@
+/*
+ * 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.dm.itest.api;
+
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+
+import org.apache.felix.dm.Component;
+import org.apache.felix.dm.ComponentState;
+import org.apache.felix.dm.ComponentStateListener;
+import org.apache.felix.dm.DependencyManager;
+import org.apache.felix.dm.itest.util.Ensure;
+import org.apache.felix.dm.itest.util.TestBase;
+
+/**
+ * Verifies if a concurrent deactivation of two components depending on each other does not produce a deadlock.
+ */
+public class FELIX5471_CyclicDependencyTest extends TestBase {
+
+ volatile Ensure m_ensure;
+
+ public void cyclicDependencyTest() throws InterruptedException {
+ DependencyManager m = getDM();
+ ExecutorService tpool = Executors.newFixedThreadPool(2);
+ try {
+ for (int count = 0; count < 1000; count++) {
+ m_ensure = new Ensure();
+
+ Component a = m.createComponent()
+ .setImplementation(new A())
+ .setInterface(A.class.getName(), null)
+ .add(m.createServiceDependency().setService(B.class).setRequired(true).setCallbacks("add", "remove"));
+
+ Component b = m.createComponent()
+ .setImplementation(new B())
+ .setInterface(B.class.getName(), null)
+ .add(m.createServiceDependency().setService(A.class).setRequired(false).setCallbacks("add", "remove"));
+
+ m.add(a);
+ m.add(b);
+
+ ComponentStateListener l = (c, s) -> {
+ if (s == ComponentState.INACTIVE) {
+ m_ensure.step();
+ }
+ };
+ a.add(l);
+ b.add(l);
+
+ m_ensure.waitForStep(4, 50000); // A started, B started
+
+ tpool.execute(() -> m.remove(a));
+ tpool.execute(() -> m.remove(b));
+
+ m_ensure.waitForStep(10, 50000); // A unbound from B, stopped and inactive, B unbound from A, stopped and inactive
+ }
+ } finally {
+ tpool.shutdown();
+ }
+ }
+
+ public class Base {
+ void start() {
+ m_ensure.step();
+ }
+
+ void stop() {
+ m_ensure.step();
+ }
+
+ }
+
+ public class A extends Base {
+ public void add(B b) {
+ m_ensure.step();
+ }
+
+ public void remove(B b) {
+ m_ensure.step();
+ }
+ }
+
+ public class B extends Base {
+ public void add(A a) {
+ m_ensure.step();
+ }
+
+ public void remove(A a) {
+ m_ensure.step();
+ }
+ }
+}
Modified: felix/trunk/dependencymanager/org.apache.felix.dependencymanager/src/org/apache/felix/dm/DependencyManager.java
URL: http://svn.apache.org/viewvc/felix/trunk/dependencymanager/org.apache.felix.dependencymanager/src/org/apache/felix/dm/DependencyManager.java?rev=1776903&r1=1776902&r2=1776903&view=diff
==============================================================================
--- felix/trunk/dependencymanager/org.apache.felix.dependencymanager/src/org/apache/felix/dm/DependencyManager.java (original)
+++ felix/trunk/dependencymanager/org.apache.felix.dependencymanager/src/org/apache/felix/dm/DependencyManager.java Mon Jan 2 08:20:08 2017
@@ -70,19 +70,6 @@ public class DependencyManager {
public static final String SERVICEREGISTRY_CACHE_INDICES = "org.apache.felix.dependencymanager.filterindex";
public static final String METHOD_CACHE_SIZE = "org.apache.felix.dependencymanager.methodcache";
- /**
- * Max time we can wait for execution of some tasks which must be handled synchronously through the internal component
- * executor (which can be a threadpool).
- * Typically, this timeout is used when a component is stopped or if a dependency is being unbound from a given
- * component.
- */
- public static final String SCHEDULE_TIMEOUT = "org.apache.felix.dependencymanager.scheduletimeout";
-
- /**
- * Default value for the SCHEDULE_TIMEOUT parameter, in millis.
- */
- public static volatile long SCHEDULE_TIMEOUT_VAL = 30000;
-
private final BundleContext m_context;
private final Logger m_logger;
private final ConcurrentHashMap<Component, Component> m_components = new ConcurrentHashMap<>();
@@ -115,14 +102,6 @@ public class DependencyManager {
}
}
}
-
- String scheduleTimeout = bundleContext.getProperty(SCHEDULE_TIMEOUT);
- if (scheduleTimeout != null) {
- try {
- SCHEDULE_TIMEOUT_VAL = Long.valueOf(scheduleTimeout);
- } catch (NumberFormatException e) {
- }
- }
}
}
catch (BundleException e) {
Modified: felix/trunk/dependencymanager/org.apache.felix.dependencymanager/src/org/apache/felix/dm/impl/ComponentImpl.java
URL: http://svn.apache.org/viewvc/felix/trunk/dependencymanager/org.apache.felix.dependencymanager/src/org/apache/felix/dm/impl/ComponentImpl.java?rev=1776903&r1=1776902&r2=1776903&view=diff
==============================================================================
--- felix/trunk/dependencymanager/org.apache.felix.dependencymanager/src/org/apache/felix/dm/impl/ComponentImpl.java (original)
+++ felix/trunk/dependencymanager/org.apache.felix.dependencymanager/src/org/apache/felix/dm/impl/ComponentImpl.java Mon Jan 2 08:20:08 2017
@@ -43,11 +43,7 @@ import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentSkipListSet;
import java.util.concurrent.CopyOnWriteArrayList;
-import java.util.concurrent.ExecutionException;
import java.util.concurrent.Executor;
-import java.util.concurrent.FutureTask;
-import java.util.concurrent.TimeUnit;
-import java.util.concurrent.TimeoutException;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicLong;
@@ -425,19 +421,10 @@ public class ComponentImpl implements Co
@Override
public void stop() {
if (m_active.compareAndSet(true, false)) {
- Runnable task = () -> {
- m_isStarted = false;
+ schedule(true /* try execute synchronously if using a tpool */, () -> {
+ m_isStarted = false;
handleChange();
- };
-
- Executor exec = getExecutor();
- if (exec instanceof DispatchExecutor) {
- // Now, we have to schedule our stopTask in our component executor. If the executor is a parallel
- // dispatcher, then try to invoke our stop task synchronously (it does not make sense to try to stop a component asynchronously).
- ((DispatchExecutor) exec).execute(task, false);
- } else {
- exec.execute(task);
- }
+ });
}
}
@@ -463,33 +450,36 @@ public class ComponentImpl implements Co
public void handleEvent(final DependencyContext dc, final EventType type, final Event... event) {
// since this method can be invoked by anyone from any thread, we need to
// pass on the event to a runnable that we execute using the component's
- // executor. There is one corner case: if this is a REMOVE event, we must try to stay synchronous
- // because if the remove event corresponds to a service being unregistered, then we must try to stop
- // our component depending on the lost service before the lost service is actually stopped.
- boolean synchronously = (type == EventType.REMOVED);
+ // executor. There is one corner case: if this is a REMOVE event, and if we are using a threadpool,
+ // then make a best effort to try invoking the component unbind callback synchronously (to make
+ // sure the unbound service is not stopped at the time we call unbind on our component
+ // which depends on the removing service).
+ // This is just a best effort, and the removed event will be handled asynchronosly if our
+ // queue is currently being run by another thread, or by the threadpool.
- schedule(synchronously, () -> {
- try {
- switch (type) {
- case ADDED:
- handleAdded(dc, event[0]);
- break;
- case CHANGED:
- handleChanged(dc, event[0]);
- break;
- case REMOVED:
- handleRemoved(dc, event[0]);
- break;
- case SWAPPED:
- handleSwapped(dc, event[0], event[1]);
- break;
- }
- } finally {
- // Clear cache of component callbacks invocation, except if we are currently called from handleChange().
- // (See FELIX-4913).
- clearInvokeCallbackCache();
- }
- });
+ boolean synchronously = (type == EventType.REMOVED);
+ schedule(synchronously, () -> {
+ try {
+ switch (type) {
+ case ADDED:
+ handleAdded(dc, event[0]);
+ break;
+ case CHANGED:
+ handleChanged(dc, event[0]);
+ break;
+ case REMOVED:
+ handleRemoved(dc, event[0]);
+ break;
+ case SWAPPED:
+ handleSwapped(dc, event[0], event[1]);
+ break;
+ }
+ } finally {
+ // Clear cache of component callbacks invocation, except if we are currently called from handleChange().
+ // (See FELIX-4913).
+ clearInvokeCallbackCache();
+ }
+ });
}
@Override
@@ -1686,23 +1676,25 @@ public class ComponentImpl implements Co
}
}
- private void schedule(boolean synchronously, Runnable task) {
- if (synchronously) {
- FutureTask<Void> future = new FutureTask<Void>(task, null);
- Executor exec = getExecutor();
+ /**
+ * Executes a task using our queue. The task is executed synchronously in case the queue is
+ * not being run by another thread, or by the threadpool.
+ *
+ * @param trySynchronous try to execute the task synchronously (best effort).
+ * @param task the task to execute.
+ */
+ private void schedule(boolean trySynchronous, Runnable task) {
+ Executor exec = getExecutor();
+ if (trySynchronous) {
if (exec instanceof DispatchExecutor) {
- // try to invoke the future from the current thread, not using the threadpool.
- ((DispatchExecutor) exec).execute(future, false);
- } else {
- exec.execute(future);
+ // Try to execute the task from the current thread if the threadpool is not currently running our queue.
+ ((DispatchExecutor) exec).execute(task, false);
+ return;
}
- try {
- future.get(DependencyManager.SCHEDULE_TIMEOUT_VAL, TimeUnit.MILLISECONDS);
- } catch (InterruptedException | ExecutionException | TimeoutException e) {
- m_logger.warn("task could not be scheduled timely in component %s (exception: %s)", this, e.toString());
- }
- } else {
- getExecutor().execute(task);
- }
- }
+ }
+ // If we are using a serial queue (no threadpool), then the queue executes the task synchronously
+ // if no other master thread is running the queue.
+ // If the are using a threadpool, then the task is always executed asynchronously, in the threadpool.
+ exec.execute(task);
+ }
}
Modified: felix/trunk/dependencymanager/org.apache.felix.dependencymanager/src/org/apache/felix/dm/impl/InvocationUtil.java
URL: http://svn.apache.org/viewvc/felix/trunk/dependencymanager/org.apache.felix.dependencymanager/src/org/apache/felix/dm/impl/InvocationUtil.java?rev=1776903&r1=1776902&r2=1776903&view=diff
==============================================================================
--- felix/trunk/dependencymanager/org.apache.felix.dependencymanager/src/org/apache/felix/dm/impl/InvocationUtil.java (original)
+++ felix/trunk/dependencymanager/org.apache.felix.dependencymanager/src/org/apache/felix/dm/impl/InvocationUtil.java Mon Jan 2 08:20:08 2017
@@ -234,7 +234,7 @@ public class InvocationUtil {
queue.execute(ft);
try {
- Exception err = ft.get(DependencyManager.SCHEDULE_TIMEOUT_VAL, TimeUnit.MILLISECONDS);
+ Exception err = ft.get();
if (err != null) {
throw err;
}