[Asterisk-code-review] stasis message.c: No need to keep a stasis type ref in a sta... (asterisk[13])

[Richard Mudgett]

Richard Mudgett has uploaded this change for review. ( https://gerrit.asterisk.org/10189


Change subject: stasis_message.c: No need to keep a stasis type ref in a stasis msg object.
......................................................................

stasis_message.c: No need to keep a stasis type ref in a stasis msg object.

Stasis message types are global ao2 objects and we make stasis messages
hold references to them.  Stasis messages could then be cached.  Since
there are currently situations where cache objects are never deleted, the
reference count on the types can exceed 100000 and generate a FRACK
assertion message.  The stasis message cache could conceivably also have
that many messages legitimately on large systems.

The only down side to not holding the message type ref in the stasis
message is it only makes a crash either at shutdown or when manually
unloading a busy module slightly more likely.  However, this is more
exposing a pre-existing stasis shutdown ordering issue than a problem with
not holding a message type ref in stasis messages.

* Made stasis messages no longer hold a ref to the message type.

Change-Id: Ibaa28efa8d8ad3836f0c65957192424c7f561707
---
M main/stasis_message.c
1 file changed, 8 insertions(+), 2 deletions(-)



  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/89/10189/1

diff --git a/main/stasis_message.c b/main/stasis_message.c
index 6274026..ba9953f 100644
--- a/main/stasis_message.c
+++ b/main/stasis_message.c
@@ -111,7 +111,6 @@
 static void stasis_message_dtor(void *obj)
 {
 	struct stasis_message *message = obj;
-	ao2_cleanup(message->type);
 	ao2_cleanup(message->data);
 }
 
@@ -129,7 +128,14 @@
 	}
 
 	message->timestamp = ast_tvnow();
-	ao2_ref(type, +1);
+	/*
+	 * XXX Normal ao2 ref counting rules says we should increment the message
+	 * type ref here and decrement it in stasis_message_dtor().  However, the
+	 * stasis message could be cached and legitimately cause the type ref count
+	 * to hit the excessive ref count assertion.  Since the message type
+	 * practically has to be a global object anyway, we can get away with not
+	 * holding a ref in the stasis message.
+	 */
 	message->type = type;
 	ao2_ref(data, +1);
 	message->data = data;

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

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ibaa28efa8d8ad3836f0c65957192424c7f561707
Gerrit-Change-Number: 10189
Gerrit-PatchSet: 1
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180918/6dcdbffe/attachment.html>