pnoltes commented on a change in pull request #261:
URL: https://github.com/apache/celix/pull/261#discussion_r446152795
##########
File path: bundles/pubsub/pubsub_topology_manager/src/pubsub_topology_manager.c
##########
@@ -69,7 +69,13 @@ celix_status_t
pubsub_topologyManager_create(celix_bundle_context_t *context, ce
status |= celixThreadMutex_create(&manager->discoveredEndpoints.mutex,
NULL);
status |=
celixThreadMutex_create(&manager->announceEndpointListeners.mutex, NULL);
status |= celixThreadMutex_create(&manager->topicReceivers.mutex, NULL);
- status |= celixThreadMutex_create(&manager->topicSenders.mutex, NULL);
+
+ celix_thread_mutexattr_t attr;
+ status |= celixThreadMutexAttr_create(&attr);
+ status |= celixThreadMutexAttr_settype(&attr,
CELIX_THREAD_MUTEX_RECURSIVE);
Review comment:
IMO the use of a recursive mutex indicates a bug. For more info why see
(e.g.) http://www.zaval.org/resources/library/butenhof1.html
If I am correct the issue is in calling the psa (specifically setup up a new
publisher) from an lock.
Note that the call at
https://github.com/apache/celix/blob/5375a2604281fa85e432fff370fda40f78c7dff0/bundles/pubsub/pubsub_topology_manager/src/pubsub_topology_manager.c#L862
can be done outside of a lock.
This is because (by design) the setup/teardown of topic senders/receivers is
thread confined. So no new enrties will be added or removed if an psa update is
called from the setupTopicSenders (or teardown)
So I think that instead of calling the psa (using the psa service id)
directly, you can collect the entries which need to call the psa in a tmp
celix_array_list_t and loop/call the psa outside of the lock.
In the loop you still need a short lock to update the entry->needsMatch,
based on the success of the psa call.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
