Call BundleContext.ungetService() when done with an event handler. Furthermore, the bundle now blocks in BundleActivator.stop() until all pending events are delivered.
git-svn-id: https://svn.apache.org/repos/asf/incubator/felix/trunk@398723 13f79535-47bb-0310-9956-ffa450edef68
diff --git a/org.apache.felix.eventadmin/src/main/java/org/apache/felix/eventadmin/impl/Activator.java b/org.apache.felix.eventadmin/src/main/java/org/apache/felix/eventadmin/impl/Activator.java
index d0e8cdb..5e74432 100644
--- a/org.apache.felix.eventadmin/src/main/java/org/apache/felix/eventadmin/impl/Activator.java
+++ b/org.apache.felix.eventadmin/src/main/java/org/apache/felix/eventadmin/impl/Activator.java
@@ -36,6 +36,7 @@
import org.apache.felix.eventadmin.impl.security.SecureEventAdminFactory;
import org.apache.felix.eventadmin.impl.security.TopicPermissions;
import org.apache.felix.eventadmin.impl.tasks.AsyncDeliverTasks;
+import org.apache.felix.eventadmin.impl.tasks.BlockTask;
import org.apache.felix.eventadmin.impl.tasks.DeliverTasks;
import org.apache.felix.eventadmin.impl.tasks.DispatchTask;
import org.apache.felix.eventadmin.impl.tasks.SyncDeliverTasks;
@@ -96,21 +97,6 @@
*
* @author <a href="mailto:felix-dev@incubator.apache.org">Felix Project Team</a>
*/
-// TODO: This is a tricky one: what do we do in case we are stopped but still have
-// already posted or send events that are not yet delivered? At the moment we will
-// throw an IllegalStateException on new events after we are stopped but
-// will deliver the pending events. Now the question is do we block in the stop
-// method until we don't have pending events anymore or (as we currently do) return
-// immediately and deliver the events ignoring the fact that we are stopped? What is
-// the best practice? Alternatively, we could just free all blocked threads and
-// discard any pending events (Note that this would need some refactoring). From my
-// point of view the current approach has the advantage that the semantics are
-// preserved: First, the immediate return allows other bundles to be notified that we
-// are stopped (and hence they deserve an IllegalStateException if they call us
-// regardless) second, successful posted or send events will be delivered, and third
-// we don't block a shutdown due to only using daemon threads internally. However,
-// it might take some time until we settle down which is somewhat cumbersome given
-// that we already are marked as stopped?
// TODO: Security is in place but untested due to not being implemented by the
// framework. However, it needs to be revisited once security is implemented.
// Two places are affected by this namely, security/* and handler/*
@@ -246,9 +232,16 @@
// Finally, adapt the outside events to our kind of events as per spec
adaptEvents(context, m_admin);
}
-
+
/**
- * Called upon stopping the bundle.
+ * Called upon stopping the bundle. This will block until all pending events are
+ * delivered. An IllegalStateException will be thrown on new events starting with
+ * the begin of this method. However, it might take some time until we settle
+ * down which is somewhat cumbersome given that the spec asks for return in
+ * a timely manner. Note that calling the stop method in one of the event
+ * delivery threads will cause the handler to be timed-out. Furthermore, calling
+ * stop in one of the event delivery threads with time-out disabled will lead to
+ * a deadlock.
*
* @param context The bundle context passed by the framework
*
@@ -260,9 +253,16 @@
m_pool.close();
- m_asyncQueue.close();
+ // This tasks will be unblocked once the queues are empty
+ final BlockTask asyncShutdownBlock = new BlockTask();
- m_syncQueue.close();
+ final BlockTask syncShutdownBlock = new BlockTask();
+
+ // Now close the queues. Note that already added tasks will be delivered
+ // The given shutdownTask will be executed once the queue is empty
+ m_asyncQueue.close(asyncShutdownBlock);
+
+ m_syncQueue.close(syncShutdownBlock);
m_admin = null;
@@ -271,6 +271,13 @@
m_asyncQueue = null;
m_syncQueue = null;
+
+ // Wait till the queues are empty (i.e., all pending events are delivered)
+ // Warning: if this is one of the event delivery threads this will lead to
+ // a deadlock in case that time-outs are disabled.
+ asyncShutdownBlock.block();
+
+ syncShutdownBlock.block();
}
/*
diff --git a/org.apache.felix.eventadmin/src/main/java/org/apache/felix/eventadmin/impl/dispatch/TaskHandler.java b/org.apache.felix.eventadmin/src/main/java/org/apache/felix/eventadmin/impl/dispatch/TaskHandler.java
index b3f3a04..a472e31 100644
--- a/org.apache.felix.eventadmin/src/main/java/org/apache/felix/eventadmin/impl/dispatch/TaskHandler.java
+++ b/org.apache.felix.eventadmin/src/main/java/org/apache/felix/eventadmin/impl/dispatch/TaskHandler.java
@@ -103,16 +103,21 @@
}
/**
- * Close the queue.
+ * Close the queue. The given shutdown task will be executed once the queue is
+ * empty.
+ *
+ * @param shutdownTask The task to execute once the queue is empty
*
* @see org.apache.felix.eventadmin.impl.dispatch.TaskQueue#close()
*/
- public void close()
+ public void close(final HandlerTask shutdownTask)
{
synchronized(m_queue)
{
m_closed = true;
+ m_queue.add(shutdownTask);
+
m_queue.notifyAll();
}
}
diff --git a/org.apache.felix.eventadmin/src/main/java/org/apache/felix/eventadmin/impl/dispatch/TaskQueue.java b/org.apache.felix.eventadmin/src/main/java/org/apache/felix/eventadmin/impl/dispatch/TaskQueue.java
index c75a692..25a0951 100644
--- a/org.apache.felix.eventadmin/src/main/java/org/apache/felix/eventadmin/impl/dispatch/TaskQueue.java
+++ b/org.apache.felix.eventadmin/src/main/java/org/apache/felix/eventadmin/impl/dispatch/TaskQueue.java
@@ -49,7 +49,9 @@
public void push(HandlerTask[] tasks);
/**
- * Close the queue.
+ * Close the queue. The given callback will be executed once the queue is empty.
+ *
+ * @param shutdownTask The task to execute once the queue is empty
*/
- public void close();
+ public void close(final HandlerTask shutdownTask);
}
diff --git a/org.apache.felix.eventadmin/src/main/java/org/apache/felix/eventadmin/impl/handler/BlacklistingHandlerTasks.java b/org.apache.felix.eventadmin/src/main/java/org/apache/felix/eventadmin/impl/handler/BlacklistingHandlerTasks.java
index 9f50b3a..38c6828 100644
--- a/org.apache.felix.eventadmin/src/main/java/org/apache/felix/eventadmin/impl/handler/BlacklistingHandlerTasks.java
+++ b/org.apache.felix.eventadmin/src/main/java/org/apache/felix/eventadmin/impl/handler/BlacklistingHandlerTasks.java
@@ -190,6 +190,23 @@
return (EventHandler) ((null != result) ? result : m_nullEventHandler);
}
+
+ /**
+ * Unget the service reference for the given event handler unless it is the
+ * NullEventHandler. This is a private method and only public due to
+ * its usage in a friend class.
+ *
+ * @param handler The event handler service to unget
+ * @param handlerRef The service reference to unget
+ */
+ public void ungetEventHandler(final EventHandler handler,
+ final ServiceReference handlerRef)
+ {
+ if(m_nullEventHandler != handler)
+ {
+ m_context.ungetService(handlerRef);
+ }
+ }
/*
* This is a null object that is supposed to do nothing. This is used once an
diff --git a/org.apache.felix.eventadmin/src/main/java/org/apache/felix/eventadmin/impl/tasks/HandlerTaskImpl.java b/org.apache.felix.eventadmin/src/main/java/org/apache/felix/eventadmin/impl/tasks/HandlerTaskImpl.java
index a263762..90cf25c 100644
--- a/org.apache.felix.eventadmin/src/main/java/org/apache/felix/eventadmin/impl/tasks/HandlerTaskImpl.java
+++ b/org.apache.felix.eventadmin/src/main/java/org/apache/felix/eventadmin/impl/tasks/HandlerTaskImpl.java
@@ -78,6 +78,8 @@
+ m_eventHandlerRef + " | Bundle("
+ m_eventHandlerRef.getBundle() + ")]", e);
}
+
+ m_handlerTasks.ungetEventHandler(handler, m_eventHandlerRef);
}
/**
diff --git a/org.apache.felix.eventadmin/src/main/java/org/apache/felix/eventadmin/impl/util/LogWrapper.java b/org.apache.felix.eventadmin/src/main/java/org/apache/felix/eventadmin/impl/util/LogWrapper.java
index e1cc5e7..6985d8e 100644
--- a/org.apache.felix.eventadmin/src/main/java/org/apache/felix/eventadmin/impl/util/LogWrapper.java
+++ b/org.apache.felix.eventadmin/src/main/java/org/apache/felix/eventadmin/impl/util/LogWrapper.java
@@ -201,13 +201,16 @@
// class as well.
for (Iterator iter = m_loggerRefs.iterator(); iter.hasNext();)
{
+ final ServiceReference next = (ServiceReference) iter.next();
+
org.osgi.service.log.LogService logger =
- (org.osgi.service.log.LogService) m_context.getService(
- (ServiceReference) iter.next());
+ (org.osgi.service.log.LogService) m_context.getService(next);
if (null != logger)
{
logger.log(level, logMsg);
+
+ m_context.ungetService(next);
}
else
{
@@ -245,13 +248,16 @@
// class as well.
for (Iterator iter = m_loggerRefs.iterator(); iter.hasNext();)
{
+ final ServiceReference next = (ServiceReference) iter.next();
+
org.osgi.service.log.LogService logger =
- (org.osgi.service.log.LogService) m_context.getService(
- (ServiceReference) iter.next());
+ (org.osgi.service.log.LogService) m_context.getService(next);
if (null != logger)
{
logger.log(level, logMsg, ex);
+
+ m_context.ungetService(next);
}
else
{
@@ -289,13 +295,16 @@
// class as well.
for (Iterator iter = m_loggerRefs.iterator(); iter.hasNext();)
{
+ final ServiceReference next = (ServiceReference) iter.next();
+
org.osgi.service.log.LogService logger =
- (org.osgi.service.log.LogService) m_context.getService(
- (ServiceReference) iter.next());
+ (org.osgi.service.log.LogService) m_context.getService(next);
if (null != logger)
{
logger.log(sr, level, logMsg);
+
+ m_context.ungetService(next);
}
else
{
@@ -335,13 +344,16 @@
// class as well.
for (Iterator iter = m_loggerRefs.iterator(); iter.hasNext();)
{
+ final ServiceReference next = (ServiceReference) iter.next();
+
org.osgi.service.log.LogService logger =
- (org.osgi.service.log.LogService) m_context.getService(
- (ServiceReference) iter.next());
+ (org.osgi.service.log.LogService) m_context.getService(next);
if (null != logger)
{
logger.log(sr, level, logMsg, ex);
+
+ m_context.ungetService(next);
}
else
{