Stability improvements for multiple clients #741
Conversation
Destroy new_subscriber also if available
There was a problem hiding this comment.
Just a note and not related to this bug fix but the instance member self.lock is not very descriptive about what is being locked. It would be better named as self.subscriptions_lock so it is more clear that this lock is needed every time the self.subscriptions and self.new_subscriptions members are accessed. Or put another way, the locking logic is kinda complex in this class and it would be nice if it was less error prone.
Anyway these changes look good to me.
|
CI for Rolling was updated to Ubuntu 22.04 and should work now |
|
@p0rys What do you think about changing |
|
This fixes #758 |
There was a problem hiding this comment.
This PR avoids the deadlock issue, but it causes some side effects such as existing clients receiving messages twice when a new client subscribes to the topic.
@p0rys do you have time address my comments? Otherwise I can do that quickly myself
| if self.new_subscriber: | ||
| self.node_handle.destroy_subscription(self.new_subscriber) |
There was a problem hiding this comment.
This should be done inside the with self.lock block as the assignment of self.new_subscriber is also protected by the lock
| # send msg to client | ||
| self.callback(msg) |
There was a problem hiding this comment.
This would effectively cause existing clients to receive messages twice when a new client connects.
I think it would be better to leave it how it was and instead use an RLock to avoid the potential deadlock.
|
Superseded by #803 |
Public API Changes
None
Description
This changes will improve the stability when multiple clients are connected simultaneously.
Fix 1 (07bd9b8): Changes "has_subscribers" to return the combined count of subscriptions and new subscriptions . This will avoid unregister of subscribers (see line 284-285) if one client disconnects while all others are still waiting for the initial callback (there are still in "new_subscriptions" and not in "subscriptions").
Fix 2 (227bfbe): Removes a possible deadlock if a client is unsubscribed before it receives the initial callback. In that case "self.new_subscriptions.values()" passed as "callbacks" would be empty and trigger a second "self.lock" (line 209) while still in the lock from line 232.