related to https://github.com/kamailio/kamailio/commit/be46742ebe162a2c48ee5ce27e09497b...
the below patch solves it, but i'm not sure of the implications why not let the expire routine do its job instead of checking expiration when adding a value ? also moved the `ht_handle_expired_record` after the record is removed
``` diff --git a/src/modules/htable/ht_api.c b/src/modules/htable/ht_api.c index 1bbd079..92a9421 100644 --- a/src/modules/htable/ht_api.c +++ b/src/modules/htable/ht_api.c @@ -674,29 +674,29 @@ && strncmp(name->s, it->name.s, name->len)==0) { /* found */ - if(now>0 && it->expire!=0 && it->expire<now) { - /* entry has expired */ - ht_handle_expired_record(ht, it); - - if(ht->flags==PV_VAL_INT) { - /* initval is integer, use it to create a fresh entry */ - it->flags &= ~AVP_VAL_STR; - it->value.n = ht->initval.n; - /* increment will be done below */ - } else { - /* delete expired entry */ - if(it->prev==NULL) - ht->entries[idx].first = it->next; - else - it->prev->next = it->next; - if(it->next) - it->next->prev = it->prev; - ht->entries[idx].esize--; - if(mode) ht_slot_unlock(ht, idx); - ht_cell_free(it); - return NULL; - } - } +// if(now>0 && it->expire!=0 && it->expire<now) { +// /* entry has expired */ +// ht_handle_expired_record(ht, it); +// +// if(ht->flags==PV_VAL_INT) { +// /* initval is integer, use it to create a fresh entry */ +// it->flags &= ~AVP_VAL_STR; +// it->value.n = ht->initval.n; +// /* increment will be done below */ +// } else { +// /* delete expired entry */ +// if(it->prev==NULL) +// ht->entries[idx].first = it->next; +// else +// it->prev->next = it->next; +// if(it->next) +// it->next->prev = it->prev; +// ht->entries[idx].esize--; +// if(mode) ht_slot_unlock(ht, idx); +// ht_cell_free(it); +// return NULL; +// } +// } /* update value */ if(it->flags&AVP_VAL_STR) { @@ -803,21 +803,21 @@ && strncmp(name->s, it->name.s, name->len)==0) { /* found */ - if(ht->htexpire>0 && it->expire!=0 && it->expire<time(NULL)) { - /* entry has expired, delete it and return NULL */ - ht_handle_expired_record(ht, it); - - if(it->prev==NULL) - ht->entries[idx].first = it->next; - else - it->prev->next = it->next; - if(it->next) - it->next->prev = it->prev; - ht->entries[idx].esize--; - ht_slot_unlock(ht, idx); - ht_cell_free(it); - return NULL; - } +// if(ht->htexpire>0 && it->expire!=0 && it->expire<time(NULL)) { +// /* entry has expired, delete it and return NULL */ +// ht_handle_expired_record(ht, it); +// +// if(it->prev==NULL) +// ht->entries[idx].first = it->next; +// else +// it->prev->next = it->next; +// if(it->next) +// it->next->prev = it->prev; +// ht->entries[idx].esize--; +// ht_slot_unlock(ht, idx); +// ht_cell_free(it); +// return NULL; +// } if(old!=NULL) { if(old->msize>=it->msize) @@ -1056,7 +1056,6 @@ if(it->expire!=0 && it->expire<now) { /* expired */ - ht_handle_expired_record(ht, it); if(it->prev==NULL) ht->entries[i].first = it->next; else @@ -1064,6 +1063,7 @@ if(it->next) it->next->prev = it->prev; ht->entries[i].esize--; + ht_handle_expired_record(ht, it); ht_cell_free(it); } it = it0; ```
core dump attached [core-dump.zip](https://github.com/kamailio/kamailio/files/1084801/core-dump.zip)
Do not do patches that comment a lot of code because it is hard to detect if all code was commented or some were removed/added/changed. Just remove the code that you think is broken or eventually add it in between `#if 0 ... #endif`.
sorry about that, patch updated
``` diff --git a/src/modules/htable/ht_api.c b/src/modules/htable/ht_api.c index 1bbd079..b5872d0 100644 --- a/src/modules/htable/ht_api.c +++ b/src/modules/htable/ht_api.c @@ -674,6 +674,7 @@ && strncmp(name->s, it->name.s, name->len)==0) { /* found */ +#if 0 if(now>0 && it->expire!=0 && it->expire<now) { /* entry has expired */ ht_handle_expired_record(ht, it); @@ -697,6 +698,7 @@ return NULL; } } +#endif /* update value */ if(it->flags&AVP_VAL_STR) { @@ -803,6 +805,7 @@ && strncmp(name->s, it->name.s, name->len)==0) { /* found */ +#if 0 if(ht->htexpire>0 && it->expire!=0 && it->expire<time(NULL)) { /* entry has expired, delete it and return NULL */ ht_handle_expired_record(ht, it); @@ -818,6 +821,7 @@ ht_cell_free(it); return NULL; } +#endif if(old!=NULL) { if(old->msize>=it->msize) @@ -1056,7 +1060,6 @@ if(it->expire!=0 && it->expire<now) { /* expired */ - ht_handle_expired_record(ht, it); if(it->prev==NULL) ht->entries[i].first = it->next; else @@ -1064,6 +1067,7 @@ if(it->next) it->next->prev = it->prev; ht->entries[i].esize--; + ht_handle_expired_record(ht, it); ht_cell_free(it); } it = it0; ```
Do you have an event_route for expired htable items in your configuration file?
I do not see at a very quick check how the removed code is causing an issue, unless there are executions with mode==0 of ht_cell_value_add(), so the lock is not acquired. As a matter of fact, that mode stuff should be removed, because the htable locks are re-entrant now.
The check for expired items needs to be done at that moment, otherwise one can increment/decrement an expired item, because the auto-expire routine is done on a timer based, on an interval basis.
yes, https://github.com/2600hz/kazoo-configs-kamailio/blob/master/kamailio/nodes-...
OK, I will dig in more once I get some time. Again, I could not find a reason on a quick check, but no much time right now, have to go for a while. Maybe, if you have some time, you can try to remove the mode and do always locking there.
Digging further I found that the handling of expired records on get/add operations was resulting in accessing the item again, after it was freed, in the case the item was accessed from event route. In your case it was exposed by:
* https://github.com/2600hz/kazoo-configs-kamailio/blob/master/kamailio/nodes-...
If the expired item was like `media=>xyz::zone`, next line is accessing it, by extracting xyz from $shtrecord() and then taking it again via $sht().
So I removed the delete operation and execution of event route for expired records during get/add operations, but slightly different than your patch, to be sure it starts with a fresh value for expired value in add operation.
Not doing the delete on add/get is making things consistent with set operations. Check for expired is still done, but no event route execution and delete. The delete and event route execution will be done by auto-expire in all cases (no longer mixed with get/add, so coherent with set behaviour, as add op is practically a set +/- a value).
I if you can give it a try and no issues found, feel free to backport -- if you don't get the time, I will do it sometime next week, once I am more confident it doesn't break something else.
Closed #1152.
@miconda your commit fixed it.
Thanks for testing, I just backported to 5.0.