[sr-dev] git:master: modules_k/presence: Fixed DB insert race hazard on the watchers table

[Peter Dunkley]

Hi Anca,

This is the watchers table, not the active_watchers table.  It's the
cache table for presence authorisation (based on a parse of pres-rules),
so it has nothing to do with dialogs.

Regards,

Peter

On Thu, 2012-03-29 at 18:30 +0300, Anca Vamanu wrote:

> Hi Peter,
> 
> You do not create transactions for Subscribe requests before calling 
> handle_subscribe() function to catch retransmissions?
> I wonder how can you get to the situation of another process inserting a 
> record (with exactly the same callid, to tag & from tag) between the 
> query and the insert.
> 
> Regards,
> Anca
> 
> On 03/29/2012 06:13 PM, Peter Dunkley wrote:
> > Module: sip-router
> > Branch: master
> > Commit: 5a89af6ea8b83ecc781d3f169023fde8388a2da6
> > URL:    http://git.sip-router.org/cgi-bin/gitweb.cgi/sip-router/?a=commit;h=5a89af6ea8b83ecc781d3f169023fde8388a2da6
> >
> > Author: Peter Dunkley<peter.dunkley at crocodile-rcs.com>
> > Committer: Peter Dunkley<peter.dunkley at crocodile-rcs.com>
> > Date:   Thu Mar 29 16:02:13 2012 +0100
> >
> > modules_k/presence: Fixed DB insert race hazard on the watchers table
> >
> > - The time between the query on the watchers table (which determines
> >    there is no matching entry) and the insert is substantial.  During
> >    a soak I observed inserts failing because rows had been inserted in
> >    this time window.
> > - The fix is to use replace (where available) instead of insert.
> > - Also fixed a small whitespace issue I noticed, and added an extra
> >    use_table call (as I think there was one missing).
> >
> > ---
> >
> >   modules_k/presence/presence.c  |    2 +-
> >   modules_k/presence/subscribe.c |   32 ++++++++++++++++++++++++++++----
> >   2 files changed, 29 insertions(+), 5 deletions(-)
> >
> > diff --git a/modules_k/presence/presence.c b/modules_k/presence/presence.c
> > index dd415f7..f6f9152 100644
> > --- a/modules_k/presence/presence.c
> > +++ b/modules_k/presence/presence.c
> > @@ -833,7 +833,7 @@ int update_watchers_status(str pres_uri, pres_ev_t* ev, str* rules_doc)
> >   	}ws_t;
> >   	ws_t* ws_list= NULL;
> >
> > -    LM_DBG("start\n");
> > +	LM_DBG("start\n");
> >
> >   	if(ev->content_type.s== NULL)
> >   	{
> > diff --git a/modules_k/presence/subscribe.c b/modules_k/presence/subscribe.c
> > index 0c914c7..0a9db83 100644
> > --- a/modules_k/presence/subscribe.c
> > +++ b/modules_k/presence/subscribe.c
> > @@ -315,6 +315,12 @@ int insert_subs_db(subs_t* s, int type)
> >   	query_vals[reason_col].val.str_val= s->reason;
> >   	query_vals[socket_info_col].val.str_val= s->sockinfo_str;
> >
> > +	if (pa_dbf.use_table(pa_db,&active_watchers_table)<  0)
> > +	{
> > +		LM_ERR("in use table sql operation\n");	
> > +		return -1;
> > +	}
> > +
> >   	LM_DBG("inserting subscription in active_watchers table\n");
> >   	if(pa_dbf.insert(pa_db, query_cols, query_vals, n_query_cols)<  0)
> >   	{
> > @@ -2310,10 +2316,28 @@ int insert_db_subs_auth(subs_t* subs)
> >   		return -1;
> >   	}
> >
> > -	if(pa_dbf.insert(pa_db, db_keys, db_vals, n_query_cols )<  0)
> > -	{	
> > -		LM_ERR("in sql insert\n");
> > -		return -1;
> > +	if (pa_dbf.replace != NULL)
> > +	{
> > +		if(pa_dbf.replace(pa_db, db_keys, db_vals, n_query_cols,
> > +					2, 0)<  0)
> > +		{
> > +			LM_ERR("in sql replace\n");
> > +			return -1;
> > +		}
> > +	}
> > +	else
> > +	{
> > +		/* If you use insert() instead of replace() be prepared for some
> > + 		   DB error messages.  There is a lot of time between the
> > +		   query() that indicated there was no matching entry in the DB
> > +		   and this insert(), so on a multi-user system it is entirely
> > +		   possible (even likely) that a record will be added after the
> > +		   query() but before this insert(). */
> > +		if(pa_dbf.insert(pa_db, db_keys, db_vals, n_query_cols )<  0)
> > +		{	
> > +			LM_ERR("in sql insert\n");
> > +			return -1;
> > +		}
> >   	}
> >
> >   	return 0;
> >
> >
> > _______________________________________________
> > sr-dev mailing list
> > sr-dev at lists.sip-router.org
> > http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
> >
> 
> 
> _______________________________________________
> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.sip-router.org/pipermail/sr-dev/attachments/20120329/1ce116f3/attachment-0001.htm>