From c99a9b228ba2aeaabb0f1832cb860c6e5080c989 Mon Sep 17 00:00:00 2001
From: Richard Mudgett <rmudgett@digium.com>
Date: Fri, 14 Sep 2018 15:51:41 -0500
Subject: [PATCH] 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
---
main/stasis_cache.c | 12 ++++++++++--
main/stasis_message.c | 10 ++++++++--
2 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/main/stasis_cache.c b/main/stasis_cache.c
index 9907c6c226..dfb154d412 100644
--- a/main/stasis_cache.c
+++ b/main/stasis_cache.c
@@ -156,7 +156,6 @@ static void cache_entry_dtor(void *obj)
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 @@ static struct stasis_cache_entry *cache_entry_create(struct stasis_message_type
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 49d6c05052..19f4a928fd 100644
--- a/main/stasis_message.c
+++ b/main/stasis_message.c
@@ -110,7 +110,6 @@ struct stasis_message {
static void stasis_message_dtor(void *obj)
{
struct stasis_message *message = obj;
- ao2_cleanup(message->type);
ao2_cleanup(message->data);
}
@@ -129,7 +128,14 @@ struct stasis_message *stasis_message_create_full(struct stasis_message_type *ty
}
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;
--
GitLab