[Asterisk-code-review] stasis: No need to keep a stasis type ref in a stasis msg or... (asterisk[master])

[George Joseph]

George Joseph has submitted this change and it was merged. ( https://gerrit.asterisk.org/10192 )

Change subject: stasis: No need to keep a stasis type ref in a stasis msg or cache object.
......................................................................

stasis: No need to keep a stasis type ref in a stasis msg or cache object.

Stasis message types are global ao2 objects and we make stasis messages
and cache entries hold references to them.  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 and cache entries no longer hold a ref to the
message type.

Change-Id: Ibaa28efa8d8ad3836f0c65957192424c7f561707
---
M main/stasis_cache.c
M main/stasis_message.c
2 files changed, 18 insertions(+), 4 deletions(-)

Approvals:
  Joshua Colp: Looks good to me, but someone else must approve
  Kevin Harwell: Looks good to me, approved
  George Joseph: Approved for Submit



diff --git a/main/stasis_cache.c b/main/stasis_cache.c
index 9907c6c..dfb154d 100644
--- a/main/stasis_cache.c
+++ b/main/stasis_cache.c
@@ -156,7 +156,6 @@
 	struct stasis_cache_entry *entry = obj;
 	size_t idx;
 
-	ao2_cleanup(entry->key.type);
 	entry->key.type = NULL;
 	ast_free((char *) entry->key.id);
 	entry->key.id = NULL;
@@ -204,7 +203,16 @@
 		ao2_cleanup(entry);
 		return NULL;
 	}
-	entry->key.type = ao2_bump(type);
+	/*
+	 * Normal ao2 ref counting rules says we should increment the message
+	 * type ref here and decrement it in cache_entry_dtor().  However, the
+	 * stasis message snapshot is cached here, will always have the same type
+	 * as the cache entry, and can legitimately cause the type ref count to
+	 * hit the excessive ref count assertion.  Since the cache entry will
+	 * always have a snapshot we can get away with not holding a ref here.
+	 */
+	ast_assert(type == stasis_message_type(snapshot));
+	entry->key.type = type;
 	cache_entry_compute_hash(&entry->key);
 
 	is_remote = ast_eid_cmp(&ast_eid_default, stasis_message_eid(snapshot)) ? 1 : 0;
diff --git a/main/stasis_message.c b/main/stasis_message.c
index 49d6c05..19f4a92 100644
--- a/main/stasis_message.c
+++ b/main/stasis_message.c
@@ -110,7 +110,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/10192
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ibaa28efa8d8ad3836f0c65957192424c7f561707
Gerrit-Change-Number: 10192
Gerrit-PatchSet: 2
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Sean Bright <sean.bright at gmail.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180920/fa9d4fd5/attachment.html>