13 Mar
2012
13 Mar
'12
2:01 a.m.
Module: sip-router
Branch: master
Commit: 287ee15ffa985cb6d07f192f1d1cbfadb31c0fd8
URL:
http://git.sip-router.org/cgi-bin/gitweb.cgi/sip-router/?a=commit;h=287ee15…
Author: Peter Dunkley <peter.dunkley(a)crocodile-rcs.com>
Committer: Peter Dunkley <peter.dunkley(a)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,
13 Mar
13 Mar
12:55 p.m.
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=287ee15…
Author: Peter Dunkley<peter.dunkley(a)crocodile-rcs.com>
Committer: Peter Dunkley<peter.dunkley(a)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(a)lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
1:31 p.m.
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:
--
Peter Dunkley
Technical Director
Crocodile RCS Ltd
Module: sip-router
Branch: master
Commit: 287ee15ffa985cb6d07f192f1d1cbfadb31c0fd8
URL:
http://git.sip-router.org/cgi-bin/gitweb.cgi/sip-router/?a=commit;h=287ee15…
Author: Peter Dunkley<peter.dunkley(a)crocodile-rcs.com>
Committer: Peter Dunkley<peter.dunkley(a)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(a)lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
2 p.m.
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:
--
Peter Dunkley
Technical Director
Crocodile RCS Ltd
Module: sip-router
Branch: master
Commit: 287ee15ffa985cb6d07f192f1d1cbfadb31c0fd8
URL:http://git.sip-router.org/cgi-bin/gitweb.cgi/sip-router/?a=commit;h=287…
Author: Peter Dunkley<peter.dunkley(a)crocodile-rcs.com
<mailto:peter.dunkley@crocodile-rcs.com>>
Committer: Peter Dunkley<peter.dunkley(a)crocodile-rcs.com
<mailto:peter.dunkley@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(a)lists.sip-router.org <mailto:sr-dev@lists.sip-router.org>
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
2:09 p.m.
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:
--
Peter Dunkley
Technical Director
Crocodile RCS Ltd
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:
--
Peter Dunkley
Technical Director
Crocodile RCS Ltd Module: sip-router
Branch: master
Commit: 287ee15ffa985cb6d07f192f1d1cbfadb31c0fd8
URL:
http://git.sip-router.org/cgi-bin/gitweb.cgi/sip-router/?a=commit;h=287ee15…
Author: Peter Dunkley<peter.dunkley(a)crocodile-rcs.com>
Committer: Peter Dunkley<peter.dunkley(a)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(a)lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
3:42 p.m.
Hello,
On 3/13/12 11:09 AM, Peter Dunkley wrote:
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.
also have in mind there is no retransmission for sip over tcp, tls or
sctp -- a connection directly to the presence server may ensure the
messages are received in order they were sent, but if in between is a
proxy that does transport gatewaying from client to presence server
(e.g., udp from client to proxy, then tcp to PS), the order cannot be
ensured. The SIP path can be a mixture of segments, some with
retransmissions, some without.
Cheers,
Daniel
Peter
On Tue, 2012-03-13 at 12:00 +0200, Anca Vamanu wrote:
--
Daniel-Constantin Mierla
Kamailio Advanced Training, April 23-26, 2012, Berlin, Germany
http://www.asipto.com/index.php/kamailio-advanced-training/
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:
--
Peter Dunkley
Technical Director
Crocodile RCS Ltd
_______________________________________________
sr-dev mailing list
sr-dev(a)lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev 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=287…
>
> Author: Peter Dunkley<peter.dunkley(a)crocodile-rcs.com
<mailto:peter.dunkley@crocodile-rcs.com>>
> Committer: Peter Dunkley<peter.dunkley(a)crocodile-rcs.com
<mailto:peter.dunkley@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(a)lists.sip-router.org <mailto:sr-dev@lists.sip-router.org>
> http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
>
--
Peter Dunkley
Technical Director
Crocodile RCS Ltd
4582
days inactive
4583
days old
5 comments
3 participants
participants (3)
-
Anca Vamanu -
Daniel-Constantin Mierla -
Peter Dunkley