Module: sip-router Branch: master Commit: fc4f2216f867b00a6685abdf51b8165572f24f69 URL: http://git.sip-router.org/cgi-bin/gitweb.cgi/sip-router/?a=commit;h=fc4f221… Author: Carlos Ruiz Diaz &lt;carlos.ruizdiaz(a)gmail.com&gt; Committer: Carlos Ruiz Diaz &lt;carlos.ruizdiaz(a)gmail.com&gt; Date: Thu Oct 24 11:05:17 2013 -0300 ims_charging: fixed deadlock when interim CCA timeout occurs --- modules/ims_charging/dialog.c | 2 + modules/ims_charging/ims_ro.c | 12 +++++++- modules/ims_charging/ro_session_hash.h | 4 +- modules/ims_charging/ro_timer.c | 47 ++++++++++++++++++++----------- 4 files changed, 45 insertions(+), 20 deletions(-) diff --git a/modules/ims_charging/dialog.c b/modules/ims_charging/dialog.c index 3e0861e..1b2453b 100644 --- a/modules/ims_charging/dialog.c +++ b/modules/ims_charging/dialog.c @@ -39,7 +39,9 @@ void dlg_reply(struct dlg_cell *dlg, int type, struct dlg_cb_params *_params) { LM_ERR("Ro Session object is NULL...... aborting\n"); return; } + ro_session_entry = &(ro_session_table->entries[session->h_entry]); + ro_session_lock(ro_session_table, ro_session_entry); if (session->active) { diff --git a/modules/ims_charging/ims_ro.c b/modules/ims_charging/ims_ro.c index 724c463..72a6fe5 100644 --- a/modules/ims_charging/ims_ro.c +++ b/modules/ims_charging/ims_ro.c @@ -642,6 +642,16 @@ error: cdpb.AAASessionsUnlock(auth->hash); cdpb.AAADropCCAccSession(auth); } + + shm_free(i_req); + // + // since callback function will be never called because of the error, we need to release the lock on the session + // to it can be reused later. + // + struct ro_session_entry *ro_session_entry = &(ro_session_table->entries[ro_session->h_entry]); + unref_ro_session_unsafe(ro_session, 1, ro_session_entry);//unref from the initial timer that fired this event. + ro_session_unlock(ro_session_table, ro_session_entry); + return; } @@ -1029,7 +1039,7 @@ static void resume_on_initial_ccr(int is_timeout, void *param, AAAMessage *cca, if (is_timeout) { update_stat(ccr_timeouts, 1); LM_ERR("Transaction timeout - did not get CCA\n"); - error_code = RO_RETURN_ERROR; + error_code = RO_RETURN_ERROR; goto error0; } diff --git a/modules/ims_charging/ro_session_hash.h b/modules/ims_charging/ro_session_hash.h index d57ef28..105bd37 100644 --- a/modules/ims_charging/ro_session_hash.h +++ b/modules/ims_charging/ro_session_hash.h @@ -78,7 +78,7 @@ extern struct ro_session_table *ro_session_table; * \param _entry locked entry */ #define ro_session_lock(_table, _entry) \ - lock_set_get( (_table)->locks, (_entry)->lock_idx); + { LM_DBG("LOCKING %d", (_entry)->lock_idx); lock_set_get( (_table)->locks, (_entry)->lock_idx); LM_DBG("LOCKED %d", (_entry)->lock_idx);} /*! @@ -87,7 +87,7 @@ extern struct ro_session_table *ro_session_table; * \param _entry locked entry */ #define ro_session_unlock(_table, _entry) \ - lock_set_release( (_table)->locks, (_entry)->lock_idx); + { LM_DBG("UNLOCKING %d", (_entry)->lock_idx); lock_set_release( (_table)->locks, (_entry)->lock_idx); LM_DBG("UNLOCKED %d", (_entry)->lock_idx); } /*! * \brief Reference an ro_session without locking diff --git a/modules/ims_charging/ro_timer.c b/modules/ims_charging/ro_timer.c index 1340c3b..797b696 100644 --- a/modules/ims_charging/ro_timer.c +++ b/modules/ims_charging/ro_timer.c @@ -256,19 +256,22 @@ void resume_ro_session_ontimeout(struct interim_ccr *i_req) { time_t now = time(0); time_t used_secs; struct ro_session_entry *ro_session_entry = NULL; + int call_terminated = 0; if (!i_req) { LM_ERR("This is so wrong: i_req is NULL\n"); return; } + ro_session_entry = &(ro_session_table->entries[i_req->ro_session->h_entry]); + LM_DBG("credit=%d credit_valid_for=%d", i_req->new_credit, i_req->credit_valid_for); used_secs = now - i_req->ro_session->last_event_timestamp; /* check to make sure diameter server is giving us sane values */ if (i_req->new_credit > i_req->credit_valid_for) { - LM_WARN("That's weird, Diameter server gave us credit with a lower validity period :D. Setting reserved time to validity perioud instead \n"); + LM_WARN("That's weird, Diameter server gave us credit with a lower validity period :D. Setting reserved time to validity period instead \n"); i_req->new_credit = i_req->credit_valid_for; } @@ -321,8 +324,21 @@ void resume_ro_session_ontimeout(struct interim_ccr *i_req) { i_req->ro_session->event_type = no_more_credit; int whatsleft = i_req->ro_session->reserved_secs - used_secs; if (whatsleft <= 0) { - LM_WARN("Immediately killing call due to no more credit\n"); + // TODO we need to handle this situation more precisely. + // in case CCR times out, we get a call shutdown but the error message assumes it was due to a lack of credit. + // + LM_WARN("Immediately killing call due to no more credit *OR* no CCA received (timeout) after reservation request\n"); + + // + // we need to unlock the session or else we might get a deadlock on dlg_terminated() dialog callback. + // Do not unref the session because it will be made inside the dlg_terminated() function. + // + + //unref_ro_session_unsafe(i_req->ro_session, 1, ro_session_entry); + ro_session_unlock(ro_session_table, ro_session_entry); + dlgb.lookup_terminate_dlg(i_req->ro_session->dlg_h_entry, i_req->ro_session->dlg_h_id, NULL ); + call_terminated = 1; } else { LM_DBG("No more credit for user - letting call run out of money in [%i] seconds", whatsleft); @@ -337,10 +353,13 @@ void resume_ro_session_ontimeout(struct interim_ccr *i_req) { } } - ro_session_entry = &(ro_session_table->entries[i_req->ro_session->h_entry]); - - unref_ro_session_unsafe(i_req->ro_session, 1, ro_session_entry);//unref from the initial timer that fired this event. - ro_session_unlock(ro_session_table, ro_session_entry); + // + // if call was forcefully terminated, the lock was released before dlgb.lookup_terminate_dlg() function call. + // + if (!call_terminated) { + unref_ro_session_unsafe(i_req->ro_session, 1, ro_session_entry);//unref from the initial timer that fired this event. + ro_session_unlock(ro_session_table, ro_session_entry); + } shm_free(i_req); LM_DBG("Exiting async ccr interim nicely"); @@ -350,26 +369,21 @@ void resume_ro_session_ontimeout(struct interim_ccr *i_req) { * If we cant we need to put a new timer to kill the call at the appropriate time */ void ro_session_ontimeout(struct ro_tl *tl) { - time_t now, used_secs, call_time; + time_t now, used_secs, call_time; LM_DBG("We have a fired timer [p=%p] and tl=[%i].\n", tl, tl->timeout); /* find the session id for this timer*/ struct ro_session_entry *ro_session_entry = NULL; - - struct ro_session* ro_session; - ro_session = ((struct ro_session*) ((char *) (tl) - - (unsigned long) (&((struct ro_session*) 0)->ro_tl))); + struct ro_session* ro_session = ((struct ro_session*) ((char *) (tl) - (unsigned long) (&((struct ro_session*) 0)->ro_tl))); if (!ro_session) { LM_ERR("Can't find a session. This is bad"); return; } -// LM_ALERT("LOCKING... "); ro_session_entry = &(ro_session_table->entries[ro_session->h_entry]); ro_session_lock(ro_session_table, ro_session_entry); -// LM_ALERT("LOCKED!"); LM_DBG("event-type=%d", ro_session->event_type); @@ -428,7 +442,6 @@ void ro_session_ontimeout(struct ro_tl *tl) { ref_ro_session_unsafe(ro_session, 1); } LM_ERR("Immediately killing call due to unknown error\n"); - dlgb.lookup_terminate_dlg(ro_session->dlg_h_entry, ro_session->dlg_h_id, NULL ); } break; @@ -439,15 +452,15 @@ void ro_session_ontimeout(struct ro_tl *tl) { LM_INFO("Call/session must be ended - no more funds.\n"); else if (ro_session->event_type == unknown_error) LM_ERR("last event caused an error. We will now tear down this session.\n"); - - dlgb.lookup_terminate_dlg(ro_session->dlg_h_entry, ro_session->dlg_h_id, NULL ); } + update_stat(killed_calls, 1); - unref_ro_session_unsafe(ro_session, 1, ro_session_entry); //unref from the initial timer that fired this event. + //unref_ro_session_unsafe(ro_session, 1, ro_session_entry); //unref from the initial timer that fired this event. ro_session_unlock(ro_session_table, ro_session_entry); + dlgb.lookup_terminate_dlg(ro_session->dlg_h_entry, ro_session->dlg_h_id, NULL); return; }