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

[Anca Vamanu]

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
>