[sr-dev] git:master: modules_k/pua: Fixed race hazard on pua table
Peter Dunkley
peter.dunkley at crocodile-rcs.com
Tue Mar 13 11:09:09 CET 2012
Hi Anca,
I have huge levels of back-end subscribes going on here so I don't want
to be wasting time handling retransmissions for this very frequent
condition.
When I first saw this race condition I thought moving to TCP on the
back-end would help. It did help a bit, but the hazard still occurred
(though less frequently) and in a slightly different way.
This temporary dialog stuff was a pain to do, but in the end it was the
only way to get things working without retransmissions.
Peter
On Tue, 2012-03-13 at 12:00 +0200, Anca Vamanu wrote:
> Hi Peter,
>
> Thanks for explaining, I understand now.
> Just one more curiosity if you allow me :) . Suppose the case when
> Notify comes and doesn't find any dialog. What if no reply is sent in
> this case? It will be retransmitted and even the first retransmission
> has big chances of coming after the 200OK and finding the dialog in
> database. Have you thought at this solution?
>
> Regards,
> Anca
>
>
> On 03/13/2012 11:31 AM, Peter Dunkley wrote:
>
> > Hi,
> >
> > This is what the PUA module used to do. Unfortunately, there is a
> > race hazard (which happens very frequently in certain scenarios)
> > where the NOTIFY that follows a SUBSCRIBE can be received and
> > processed faster than the 200 OK to the SUBSCRIBE. The temporary
> > dialog stuff is in there so that this NOTIFY can be matched and
> > processed.
> >
> > So, while putting the PUA behaviour back to this will fix the latest
> > race hazard I found it will re-introduce a much more likely one.
> >
> > Regards,
> >
> > Peter
> >
> > On Tue, 2012-03-13 at 10:55 +0200, Anca Vamanu wrote:
> >
> >
> > > Hi Peter,
> > >
> > >
> > > Wouldn't it had been easier if you waited until 200OK to do the insert?
> > > Kamailio is does not store transactions persistently, so there is no
> > > point to store the subscribe dialog information until it is not
> > > established. The reason is that if kamailio is restarted in the mean
> > > time before receiving the 200OK, it won't match the 200OK to the
> > > transaction and won't complete the dialog anyways.
> > > This is actually a solution I adopted in the b2bua modules which had the
> > > same race problems.
> > >
> > > Regards,
> > > Anca
> > >
> > >
> > >
> > > On 03/13/2012 12:01 AM, Peter Dunkley wrote:
> > > > Module: sip-router
> > > > Branch: master
> > > > Commit: 287ee15ffa985cb6d07f192f1d1cbfadb31c0fd8
> > > > URL: http://git.sip-router.org/cgi-bin/gitweb.cgi/sip-router/?a=commit;h=287ee15ffa985cb6d07f192f1d1cbfadb31c0fd8
> > > >
> > > > Author: Peter Dunkley<peter.dunkley at crocodile-rcs.com>
> > > > Committer: Peter Dunkley<peter.dunkley at crocodile-rcs.com>
> > > > Date: Mon Mar 12 21:52:39 2012 +0000
> > > >
> > > > modules_k/pua: Fixed race hazard on pua table
> > > >
> > > > - During testing a race hazard where the 2XX to a back-end SUBSCRIBE can be
> > > > received and processed (and the DB UPDATE to convert a temporary dialog to a
> > > > full dialog) before the DB INSERT to create a temporary dialog is run. There
> > > > is an incredibly small window for this, but it was happening consistently on
> > > > one system.
> > > > - The easiest way to fix this is to use the replace() DB API to convert the
> > > > dialog and live with the initial INSERT failing (this does not actually
> > > > return an error from the SRDB1 interface so the rest of the code continues
> > > > OK). Unfortunately, the replace() API is not available for some databases
> > > > (for example, PostgreSQL).
> > > > - I have updated the code to use replace() when it is available and to do an
> > > > update() then check affected_rows() (and if 0 then do an insert()) when
> > > > replace() is not available.
> > > > - The update() and then insert() process makes the window for the race much
> > > > smaller, but doesn't get rid of it completely. However, with PostgreSQL a
> > > > DB rule can be used to fix it completely:
> > > > - PostgreSQL DB rule:
> > > > CREATE RULE "pua_insert_race1" AS ON INSERT TO "pua"
> > > > WHERE EXISTS(
> > > > SELECT 1 FROM pua WHERE call_id=NEW.call_id AND from_tag=NEW.from_tag
> > > > AND pres_id=NEW.pres_id AND to_tag=''
> > > > ) DO INSTEAD (
> > > > UPDATE pua
> > > > SET expires=NEW.expires,
> > > > desired_expires=NEW.desired_expires,
> > > > flag=NEW.flag,
> > > > etag=NEW.etag,
> > > > tuple_id=NEW.tuple_id,
> > > > to_tag=NEW.to_tag,
> > > > cseq=NEW.cseq,
> > > > record_route=NEW.record_route,
> > > > contact=NEW.contact,
> > > > remote_contact=NEW.remote_contact,
> > > > version=NEW.version,
> > > > extra_headers=NEW.extra_headers
> > > > WHERE call_id=NEW.call_id AND from_tag=NEW.from_tag
> > > > AND pres_id=NEW.pres_id
> > > > );
> > > > - You can also add another PostgreSQL rule to make the failing INSERT
> > > > (described above) do so quietly. This does not affect the function of the
> > > > code, but it will make the logs quieter (which is nice):
> > > > CREATE RULE "pua_insert_race2" AS ON INSERT TO "pua"
> > > > WHERE EXISTS(
> > > > SELECT 1 FROM pua WHERE call_id=NEW.call_id AND from_tag=NEW.from_tag
> > > > AND to_tag<>''
> > > > ) DO INSTEAD NOTHING;
> > > >
> > > > ---
> > > >
> > > > modules_k/pua/pua_db.c | 187 +++++++++++++++++++++++++---------------
> > > > modules_k/pua/send_subscribe.c | 54 ------------
> > > > 2 files changed, 118 insertions(+), 123 deletions(-)
> > > >
> > > > diff --git a/modules_k/pua/pua_db.c b/modules_k/pua/pua_db.c
> > > > index b83e3ed..b652cee 100644
> > > > --- a/modules_k/pua/pua_db.c
> > > > +++ b/modules_k/pua/pua_db.c
> > > > @@ -883,9 +883,9 @@ int get_record_id_puadb(ua_pres_t *pres, str **rec_id )
> > > > /******************************************************************************/
> > > > int convert_temporary_dialog_puadb(ua_pres_t *pres)
> > > > {
> > > > - db_key_t query_cols[4], data_cols[10];
> > > > - db_val_t query_vals[4], data_vals[10];
> > > > - int n_query_cols = 0, n_data_cols = 0;
> > > > + db_key_t query_cols[18];
> > > > + db_val_t query_vals[18];
> > > > + int n_query_cols = 0;
> > > >
> > > > if (pres==NULL)
> > > > {
> > > > @@ -920,82 +920,131 @@ int convert_temporary_dialog_puadb(ua_pres_t *pres)
> > > > n_query_cols++;
> > > >
> > > > /* The columns I need to fill in to convert a temporary dialog to a dialog */
> > > > - data_cols[n_data_cols] =&str_expires_col;
> > > > - data_vals[n_data_cols].type = DB1_INT;
> > > > - data_vals[n_data_cols].nul = 0;
> > > > - data_vals[n_data_cols].val.int_val = pres->expires;
> > > > - n_data_cols++;
> > > > -
> > > > - data_cols[n_data_cols] =&str_desired_expires_col;
> > > > - data_vals[n_data_cols].type = DB1_INT;
> > > > - data_vals[n_data_cols].nul = 0;
> > > > - data_vals[n_data_cols].val.int_val = pres->desired_expires;
> > > > - n_data_cols++;
> > > > -
> > > > - data_cols[n_data_cols] =&str_flag_col;
> > > > - data_vals[n_data_cols].type = DB1_INT;
> > > > - data_vals[n_data_cols].nul = 0;
> > > > - data_vals[n_data_cols].val.int_val = pres->flag;
> > > > - n_data_cols++;
> > > > -
> > > > - data_cols[n_data_cols] =&str_to_tag_col;
> > > > - data_vals[n_data_cols].type = DB1_STR;
> > > > - data_vals[n_data_cols].nul = 0;
> > > > - data_vals[n_data_cols].val.str_val = pres->to_tag;
> > > > - n_data_cols++;
> > > > -
> > > > - data_cols[n_data_cols] =&str_cseq_col;
> > > > - data_vals[n_data_cols].type = DB1_INT;
> > > > - data_vals[n_data_cols].nul = 0;
> > > > - data_vals[n_data_cols].val.int_val = pres->cseq;
> > > > - n_data_cols++;
> > > > -
> > > > - data_cols[n_data_cols] =&str_record_route_col;
> > > > - data_vals[n_data_cols].type = DB1_STR;
> > > > - data_vals[n_data_cols].nul = 0;
> > > > - data_vals[n_data_cols].val.str_val = pres->record_route;
> > > > - n_data_cols++;
> > > > -
> > > > - data_cols[n_data_cols] =&str_contact_col;
> > > > - data_vals[n_data_cols].type = DB1_STR;
> > > > - data_vals[n_data_cols].nul = 0;
> > > > - data_vals[n_data_cols].val.str_val = pres->contact;
> > > > - n_data_cols++;
> > > > -
> > > > - data_cols[n_data_cols] =&str_remote_contact_col;
> > > > - data_vals[n_data_cols].type = DB1_STR;
> > > > - data_vals[n_data_cols].nul = 0;
> > > > - data_vals[n_data_cols].val.str_val = pres->remote_contact;
> > > > - n_data_cols++;
> > > > -
> > > > - data_cols[n_data_cols] =&str_version_col;
> > > > - data_vals[n_data_cols].type = DB1_INT;
> > > > - data_vals[n_data_cols].nul = 0;
> > > > - data_vals[n_data_cols].val.int_val = pres->version;
> > > > - n_data_cols++;
> > > > -
> > > > - data_cols[n_data_cols] =&str_extra_headers_col;
> > > > - data_vals[n_data_cols].type = DB1_STR;
> > > > - data_vals[n_data_cols].nul = 0;
> > > > + query_cols[n_query_cols] =&str_expires_col;
> > > > + query_vals[n_query_cols].type = DB1_INT;
> > > > + query_vals[n_query_cols].nul = 0;
> > > > + query_vals[n_query_cols].val.int_val = pres->expires;
> > > > + n_query_cols++;
> > > > +
> > > > + query_cols[n_query_cols] =&str_desired_expires_col;
> > > > + query_vals[n_query_cols].type = DB1_INT;
> > > > + query_vals[n_query_cols].nul = 0;
> > > > + query_vals[n_query_cols].val.int_val = pres->desired_expires;
> > > > + n_query_cols++;
> > > > +
> > > > + query_cols[n_query_cols] =&str_flag_col;
> > > > + query_vals[n_query_cols].type = DB1_INT;
> > > > + query_vals[n_query_cols].nul = 0;
> > > > + query_vals[n_query_cols].val.int_val = pres->flag;
> > > > + n_query_cols++;
> > > > +
> > > > + query_cols[n_query_cols] =&str_to_tag_col;
> > > > + query_vals[n_query_cols].type = DB1_STR;
> > > > + query_vals[n_query_cols].nul = 0;
> > > > + query_vals[n_query_cols].val.str_val = pres->to_tag;
> > > > + n_query_cols++;
> > > > +
> > > > + query_cols[n_query_cols] =&str_cseq_col;
> > > > + query_vals[n_query_cols].type = DB1_INT;
> > > > + query_vals[n_query_cols].nul = 0;
> > > > + query_vals[n_query_cols].val.int_val = pres->cseq;
> > > > + n_query_cols++;
> > > > +
> > > > + query_cols[n_query_cols] =&str_record_route_col;
> > > > + query_vals[n_query_cols].type = DB1_STR;
> > > > + query_vals[n_query_cols].nul = 0;
> > > > + query_vals[n_query_cols].val.str_val = pres->record_route;
> > > > + n_query_cols++;
> > > > +
> > > > + query_cols[n_query_cols] =&str_contact_col;
> > > > + query_vals[n_query_cols].type = DB1_STR;
> > > > + query_vals[n_query_cols].nul = 0;
> > > > + query_vals[n_query_cols].val.str_val = pres->contact;
> > > > + n_query_cols++;
> > > > +
> > > > + query_cols[n_query_cols] =&str_remote_contact_col;
> > > > + query_vals[n_query_cols].type = DB1_STR;
> > > > + query_vals[n_query_cols].nul = 0;
> > > > + query_vals[n_query_cols].val.str_val = pres->remote_contact;
> > > > + n_query_cols++;
> > > > +
> > > > + query_cols[n_query_cols] =&str_version_col;
> > > > + query_vals[n_query_cols].type = DB1_INT;
> > > > + query_vals[n_query_cols].nul = 0;
> > > > + query_vals[n_query_cols].val.int_val = pres->version;
> > > > + n_query_cols++;
> > > > +
> > > > + query_cols[n_query_cols] =&str_extra_headers_col;
> > > > + query_vals[n_query_cols].type = DB1_STR;
> > > > + query_vals[n_query_cols].nul = 0;
> > > > if (pres->extra_headers)
> > > > {
> > > > - data_vals[n_data_cols].val.str_val.s = pres->extra_headers->s;
> > > > - data_vals[n_data_cols].val.str_val.len = pres->extra_headers->len;
> > > > + query_vals[n_query_cols].val.str_val.s = pres->extra_headers->s;
> > > > + query_vals[n_query_cols].val.str_val.len = pres->extra_headers->len;
> > > > }
> > > > else
> > > > {
> > > > - data_vals[n_data_cols].val.str_val.s = "";
> > > > - data_vals[n_data_cols].val.str_val.len = 0;
> > > > + query_vals[n_query_cols].val.str_val.s = "";
> > > > + query_vals[n_query_cols].val.str_val.len = 0;
> > > > }
> > > > - n_data_cols++;
> > > > + n_query_cols++;
> > > >
> > > > - if (pua_dbf.update(pua_db, query_cols, 0, query_vals,
> > > > - data_cols, data_vals, n_query_cols, n_data_cols)< 0)
> > > > + query_cols[n_query_cols] =&str_event_col;
> > > > + query_vals[n_query_cols].type = DB1_INT;
> > > > + query_vals[n_query_cols].nul = 0;
> > > > + query_vals[n_query_cols].val.int_val = pres->event;
> > > > + n_query_cols++;
> > > > +
> > > > + query_cols[n_query_cols] =&str_watcher_uri_col;
> > > > + query_vals[n_query_cols].type = DB1_STR;
> > > > + query_vals[n_query_cols].nul = 0;
> > > > + query_vals[n_query_cols].val.str_val.s = pres->watcher_uri->s;
> > > > + query_vals[n_query_cols].val.str_val.len = pres->watcher_uri->len;
> > > > + n_query_cols++;
> > > > +
> > > > + query_cols[n_query_cols] =&str_etag_col;
> > > > + query_vals[n_query_cols].type = DB1_STR;
> > > > + query_vals[n_query_cols].nul = 0;
> > > > + query_vals[n_query_cols].val.str_val.s = 0;
> > > > + query_vals[n_query_cols].val.str_val.len = 0;
> > > > + n_query_cols++;
> > > > +
> > > > + query_cols[n_query_cols] =&str_tuple_id_col;
> > > > + query_vals[n_query_cols].type = DB1_STR;
> > > > + query_vals[n_query_cols].nul = 0;
> > > > + query_vals[n_query_cols].val.str_val.s = 0;
> > > > + query_vals[n_query_cols].val.str_val.len = 0;
> > > > + n_query_cols++;
> > > > +
> > > > + if (pua_dbf.replace != NULL)
> > > > {
> > > > - LM_ERR("Failed update db\n");
> > > > - return -1;
> > > > + if (pua_dbf.replace(pua_db, query_cols, query_vals, n_query_cols)< 0)
> > > > + {
> > > > + LM_ERR("Failed replace db\n");
> > > > + return -1;
> > > > + }
> > > > + }
> > > > + else
> > > > + {
> > > > + if (pua_dbf.update(pua_db, query_cols, 0, query_vals,
> > > > + query_cols + 4, query_vals + 4, 4, n_query_cols - 4)< 0)
> > > > + {
> > > > + LM_ERR("Failed update db\n");
> > > > + return -1;
> > > > + }
> > > > +
> > > > + LM_DBG("affected_rows: %d\n", pua_dbf.affected_rows(pua_db));
> > > > + if (pua_dbf.affected_rows(pua_db) == 0)
> > > > + {
> > > > + if (pua_dbf.insert(pua_db, query_cols, query_vals, n_query_cols)< 0)
> > > > + {
> > > > + LM_ERR("Failed insert db\n");
> > > > + return -1;
> > > > + }
> > > > + }
> > > > }
> > > >
> > > > +
> > > > shm_free(pres->remote_contact.s);
> > > > shm_free(pres);
> > > >
> > > > diff --git a/modules_k/pua/send_subscribe.c b/modules_k/pua/send_subscribe.c
> > > > index 15a1a02..b135ac3 100644
> > > > --- a/modules_k/pua/send_subscribe.c
> > > > +++ b/modules_k/pua/send_subscribe.c
> > > > @@ -1050,18 +1050,9 @@ insert:
> > > >
> > > > if(subs->flag& UPDATE_TYPE)
> > > > {
> > > > - /*
> > > > - LM_ERR("request for a subscription"
> > > > - " with update type and no record found\n");
> > > > - ret= -1;
> > > > - goto done;
> > > > - commented this because of the strange type parameter in usrloc callback functions
> > > > - */
> > > > -
> > > > LM_DBG("request for a subscription with update type"
> > > > " and no record found\n");
> > > > subs->flag= INSERT_TYPE;
> > > > -
> > > > }
> > > > hentity= subscribe_cbparam(subs, REQ_OTHER);
> > > > if(hentity== NULL)
> > > > @@ -1168,50 +1159,6 @@ insert:
> > > > }
> > > > else
> > > > {
> > > > - /*
> > > > - if(presentity->desired_expires== 0)
> > > > - {
> > > > -
> > > > - if(subs->expires< 0)
> > > > - {
> > > > - LM_DBG("Found previous request for unlimited subscribe-"
> > > > - " do not send subscribe\n");
> > > > -
> > > > - if (subs->event& PWINFO_EVENT)
> > > > - {
> > > > - presentity->watcher_count++;
> > > > - }
> > > > - lock_release(&HashT->p_records[hash_code].lock);
> > > > - goto done;
> > > > -
> > > > - }
> > > > -
> > > > -
> > > > - if(subs->event& PWINFO_EVENT)
> > > > - {
> > > > - if(subs->expires== 0)
> > > > - {
> > > > - presentity->watcher_count--;
> > > > - if( presentity->watcher_count> 0)
> > > > - {
> > > > - lock_release(&HashT->p_records[hash_code].lock);
> > > > - goto done;
> > > > - }
> > > > - }
> > > > - else
> > > > - {
> > > > - presentity->watcher_count++;
> > > > - if(presentity->watcher_count> 1)
> > > > - {
> > > > - lock_release(&HashT->p_records[hash_code].lock);
> > > > - goto done;
> > > > - }
> > > > - }
> > > > - }
> > > > -
> > > > - }
> > > > - */
> > > > -
> > > > if (subs->internal_update_flag == INTERNAL_UPDATE_TRUE)
> > > > {
> > > > LM_INFO("attempting to re-SUBSCRIBE on internal (rls_update_subs()) update - skipping\n");
> > > > @@ -1265,7 +1212,6 @@ insert:
> > > > if (dbmode!=PUA_DB_ONLY)
> > > > lock_release(&HashT->p_records[hash_code].lock);
> > > >
> > > > - // hentity->flag= flag;
> > > > LM_DBG("event parameter: %d\n", hentity->event);
> > > >
> > > > set_uac_req(&uac_r,&met, str_hdr, 0, td, TMCB_LOCAL_COMPLETED,
> > > >
> > > >
> > > > _______________________________________________
> > > > sr-dev mailing list
> > > > sr-dev at lists.sip-router.org
> > > > http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
> > > >
> > >
> >
> >
> > --
> > Peter Dunkley
> > Technical Director
> > Crocodile RCS Ltd
>
>
--
Peter Dunkley
Technical Director
Crocodile RCS Ltd
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.sip-router.org/pipermail/sr-dev/attachments/20120313/a1d801ba/attachment-0001.htm>
More information about the sr-dev
mailing list