[Asterisk-code-review] Core/Stasis: Fix "FRACK!, Failed assertion bad magic number" happens ... (asterisk[16])

[Joshua Colp]

Joshua Colp has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/15762 )

Change subject: Core/Stasis: Fix "FRACK!, Failed assertion bad magic number" happens when unsubscribe an application from an event source
......................................................................


Patch Set 2:

> Patch Set 2:
> 
> You are not carefully reading the clarification of the changes I have made. Please read the above explanations again.
> 
> Note that I did not come up with this approach to managing objects, I only adhere to this rule.
> 
> The subscription logic allows you to subscribe not only to one but also to several applications to the same subscription. The only difference is that there are two ways to store objects for later search and retrieval:
> 1. Vector for technology subscriptions (tech_subscriptions)
> 2. Special container for resource subscriptions (endpoint_subscriptions)

Normally yes you would want applications to also have a reference to the subscription so subscription can automatically be destroyed, but in this implementation applications do not actually have a reference to the subscription so it doesn't actually help and doesn't provide automatic lifetime tracking. Subscription lifetime still has to be manually managed through the subscription subscribe/unsubscribe implementation because the subscription is stored within tech_subscriptions or endpoint_subscriptions.

This is why the intended reference counting model does not do reference counting on the subscription based on the applications. The intention appears to have been "as long as there is one application then the subscription has a minimum reference count of 1".

Your change alters the reference counting model to make it follow the number of applications, but I'm not sure that's necessary based on my past comment regarding moving "ao2_ref(sub, -1);" to be in the tech_subscriptions block.

With that change:

In messaging_app_subscribe_endpoint get_or_create_subscription will get or create a subscription and store it in either endpoint_subscriptions or tech_subscriptions - this function will then return and in total there are 2 references at that point. The messaging_app_subscribe_endpoint function releases the reference passed to it from get_or_create_subscription at the end because of RAII_VAR - leaving the reference count at 1.

In messaging_app_unsubscribe_endpoint if the "ao2_ref(sub, -1);" is moved then only the case where there are no more applications will the reference held by endpoint_subscriptions or tech_subscriptions be released. This is because in the case where there are applications remaining the reference returned by get_subscription is balanced by the RAII_VAR in messaging_app_unsubscribe_endpoint, and the reference held by endpoint_subscriptions or tech_subscriptions is untouched.


-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/15762
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: Ia91b15f8e5ea68f850c66889a6325d9575901729
Gerrit-Change-Number: 15762
Gerrit-PatchSet: 2
Gerrit-Owner: Evgenios Muratidis <jone1984 at hotmail.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Comment-Date: Mon, 24 May 2021 13:20:00 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20210524/66d22289/attachment.html>