Oipo commented on a change in pull request #261:
URL: https://github.com/apache/celix/pull/261#discussion_r446171761
##########
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:
I agree that recursive mutexes are code smells. The issue here is that
multiple locks are done all over the place -- making it a veritable nightmare
for ensuring thread safety.
The line you point at is one such an example where it *seems* like it is
safe to put outside the lock -- it isn't. If you follow the function calls, you
end up at
https://github.com/apache/celix/blob/5375a2604281fa85e432fff370fda40f78c7dff0/bundles/pubsub/pubsub_topology_manager/src/pubsub_topology_manager.c#L888
which shows that the endpoint of the entry is being modified in the callback.
The option then is to pass some temporary memory, call the psa function,
re-iterate over the pstm entries to find the entry again...but what if the
entry is gone at that point? The psa can easily be told to clean up its
resources, but how do you communicate to the pstm that the callback went wrong?
I guess the pstm would *also* have to re-iterate the entries to find it and if
it concludes it is missing, do some cleanup/abort setting up this sender. But
you would really rather want to recover from this state. Do you retry? Do you
tell the bundle that is requesting a sender to be setup that it is incapable of
doing so? How?
This is a whole can of worms that shows the bigger issue in celix (not just
pubsub): it pretends to be able to handle asynchronicity in various places, but
it really doesn't. It's build for sequential execution.
I don't blame people, don't get me wrong: multithreaded programming is much,
much harder. What we need to properly fix this is to embody the asynchronicity
in a concept. Something akin to the actor model, with each actor having an
error mailbox would help, self-healing mechanisms would obviously be better.
Another solution would be an event-loop with error callbacks, which remove the
need to have nested locks as well.
----------------------------------------------------------------
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]
