[GitHub] [celix] Oipo commented on a change in pull request #261: pubsub topology manager deadlock fix

[GitBox <gi...@apache.org>]

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:
users@infra.apache.org