RE: Re: [PATCH] fix bug #529 (sms-resend-* ignored for concatenated messages)
Michael Zervakis <[email protected]>
| Newsgroups | gmane.comp.mobile.kannel.devel |
|---|---|
| Message-ID | <[email protected]> |
Hi, I figured out that we can use the same mechanism as in Msg struct to uniquelly identify each SMSCconn. In the attached patch the smscconn struct was modified to include a uuid_t field and Msg struct to store the smscconn uuid_t if we get a temporary error. Smsc_rout2 was also modified to respect msg->msg_ConnId if set. SMSC-A -> splits (2 parts) -> 1 part sent OK -> 2 part get temp. error -> write the conn->uuid_t and put it into global queue for resend -> router reads msg->msg_ConnId and sends failed part via same connection (if still active) -----Original Message----- From: Alexander Malysh [mailto:[email protected]] On Behalf Of Alexander Malysh Sent: Sunday, September 12, 2010 1:06 PM To: Michael Zervakis Cc: [email protected]; [email protected] Subject: Re: [PATCH] fix bug #529 (sms-resend-* ignored for concatenated messages) Hi, as I already wrote, this patch is not enough. Please see ML wy... Thanks, Alexander Malysh Am 09.09.2010 um 19:04 schrieb Michael Zervakis: > Hi, > > This patch is a possible solution fog Bug #529. The Msg definition was modified so we can store the conn->id originally selected by router. > > SMSC-A -> splits (2 parts) -> 1 part sent OK -> 2 part get temp. error -> write the conn->id and put it into global queue for resend -> router reads msg->sms.msg_ConnId and sends failed part via same connection (if still active) > > > Regards, > > > From: [email protected] [mailto:[email protected]] On Behalf Of Konstantin Vayner > Sent: Thursday, December 17, 2009 12:28 PM > To: Alexander Malysh > Cc: [email protected] > Subject: Re: [PATCH] fix bug #529 (sms-resend-* ignored for concatenated messages) > > > why remembering smsc-id in sms.smsc_id is not enough? > how does smsbox remember routing when i submit a message with predefined smsc id from http (sendsms) ? > > On Thu, Dec 17, 2009 at 12:10 PM, Alexander Malysh <[email protected]> wrote: > > > Am 17.12.2009 um 10:43 schrieb Konstantin Vayner: > > > > so the best option would be to requeue the part via same smsc, right ? > > > yes, but it's not easy todo. You have to remember SMSC pointer not only SMSC-name/id and then teach all routing parts > > to respect it... > > > > cause requeueing all parts may also get extra messages to the handset despite it not being able to reconstruct (not to mention the extra money ;) ) > > On Thu, Dec 17, 2009 at 11:33 AM, Alexander Malysh <[email protected]> wrote: > > Hi, > > > unfortunately this will not work as expected (the rule is: _all_ parts if multipart message have to be send via the same SMSC)... > > > example: > > > SMSC-A -> splits (2 parts) -> 1 part sent OK -> 2 part get temp. error -> you put it into global queue for resend -> 2 part sent via SMSC-B -> handset rejects it > > > We have only two possibility here: > > 1) if temp error occurs put the _whole_ message into resend queue and resend then _all_ parts (very easy todo) > > 2) remember smsc which was used for first parts and resend it via the same smsc (complicated but save money :) ) > > > Thanks, > > Alexander Malysh > > > Am 16.12.2009 um 18:17 schrieb Konstantin Vayner: > > > > Bug report: http://redmine.kannel.org/issues/show/529 > > Quote from gw/bb_smscconn.c : > > static void handle_split(SMSCConn *conn, Msg *msg, long reason) > { > struct split_parts *split = msg->sms.split_parts; > > /* > * If temporarely failed, try again immediately but only if connection active. > * Because if connection is not active we will loop for ever here consuming 100% CPU > * time due to internal queue cleanup in smsc module that call bb_smscconn_failed. > */ > if (reason == SMSCCONN_FAILED_TEMPORARILY && smscconn_status(conn) == SMSCCONN_ACTIVE && > smscconn_send(conn, msg) == 0) { > /* destroy this message because it will be duplicated in smsc module */ > msg_destroy(msg); > return; > } > > (end quote) > > So, if an smsc is alive and throws temporary error every time you try to submit such a message, we enter endless loop of attempting to resend it.... > > > Suggested patch follows (also attached). > Sorry its not cvs diff - having firewall issues accessing pserver now so i ran diff vs snapshot generated yesterday > I will be able to produce a normal cvs diff tomorrow morning if it is needed > > > --- kannel-snapshot/gw/bb_smscconn.c 2009-11-15 16:12:28.000000000 +0200 > +++ gateway-cvs/gw/bb_smscconn.c 2009-12-16 19:47:32.000000000 +0200 > @@ -203,18 +203,6 @@ > struct split_parts *split = msg->sms.split_parts; > /* > - * If temporarely failed, try again immediately but only if connection active. > - * Because if connection is not active we will loop for ever here consuming 100% CPU > - * time due to internal queue cleanup in smsc module that call bb_smscconn_failed. > - */ > - if (reason == SMSCCONN_FAILED_TEMPORARILY && smscconn_status(conn) == SMSCCONN_ACTIVE && > - smscconn_send(conn, msg) == 0) { > - /* destroy this message because it will be duplicated in smsc module */ > - msg_destroy(msg); > - return; > - } > - - /* > * if the reason is not a success and status is still success > * then set status of a split to the reason. > * Note: reason 'malformed','discarded' or 'rejected' has higher priority! > @@ -303,7 +291,7 @@ > void bb_smscconn_send_failed(SMSCConn *conn, Msg *sms, int reason, Octstr *reply) > { > - if (sms->sms.split_parts != NULL) { > + if (reason != SMSCCONN_FAILED_TEMPORARILY && sms->sms.split_parts != NULL) { > handle_split(conn, sms, reason); > octstr_destroy(reply); > return; > > > > > > <bb_smscconn.diff> > > > > > > Index: gw/bb_smscconn.c > =================================================================== > --- gw/bb_smscconn.c (revision 4838) > +++ gw/bb_smscconn.c (working copy) > @@ -207,11 +207,34 @@ > * Because if connection is not active we will loop for ever here consuming 100% CPU > * time due to internal queue cleanup in smsc module that call bb_smscconn_failed. > */ > - if (reason == SMSCCONN_FAILED_TEMPORARILY && smscconn_status(conn) == SMSCCONN_ACTIVE && > - smscconn_send(conn, msg) == 0) { > + /*if (reason == SMSCCONN_FAILED_TEMPORARILY && smscconn_status(conn) == SMSCCONN_ACTIVE && > + smscconn_send(conn, msg) == 0) { */ > /* destroy this message because it will be duplicated in smsc module */ > - msg_destroy(msg); > + /* msg_destroy(msg); > return; > + }*/ > + > + > + > + if (reason == SMSCCONN_FAILED_TEMPORARILY && smscconn_status(conn) == SMSCCONN_ACTIVE) > + { > + if (sms_resend_retry >= 0 && msg->sms.resend_try >= sms_resend_retry) /*check how many times the split message has been sent */ > + { > + warning(0, "SMPP[%s] :Maximum retries for message [%s] exceeded resend %ld times, discarding it!", octstr_get_cstr(conn->id), octstr_get_cstr(msg->sms.msgdata),msg->sms.resend_try); > + handle_split(conn, msg, SMSCCONN_FAILED_DISCARDED); > + /* msg_destroy(msg); */ > + return; > + } > + > + warning(0, "SMPP[%s] :Split message failed temporarily sending back to sms_router", octstr_get_cstr(conn->id)); > + > + msg->sms.resend_try = (msg->sms.resend_try > 0 ? msg->sms.resend_try + 1 : 1); > + debug("bb.sms.splits", 0, "SMPP[%s] :Resend counter of message set to %ld", octstr_get_cstr(conn->id),msg->sms.resend_try); > + time(&msg->sms.resend_time); > + msg->sms.msg_ConnId = octstr_duplicate(conn->id); > + > + gwlist_produce(outgoing_sms, msg); /* write it back to global queue */ > + return; > } > > /* > @@ -1229,6 +1252,28 @@ > bp_load = bo_load = queue_length = 0; > > conn = NULL; > + /* if msg was a failed part then route to the defined SMSC connection */ > + if (msg->sms.msg_ConnId != NULL) > + { > + for (i=0; i < gwlist_len(smsc_list); i++) > + { > + conn = gwlist_get(smsc_list, i); > + smscconn_info(conn, &info); > + queue_length += (info.queued > 0 ? info.queued : 0); > + > + if(octstr_compare(msg->sms.msg_ConnId, conn->id) == 0) > + { > + if((smscconn_usable(conn,msg) != -1) && (info.status == SMSCCONN_ACTIVE && info.queued < max_queue)) > + { > + best_preferred = conn; > + debug("bb.sms",0,"Message with pre-set connection to SMSC[%s] found",octstr_get_cstr(msg->sms.msg_ConnId)); > + continue; > + } > + } > + } > + } > + else > + { > for (i=0; i < gwlist_len(smsc_list); i++) { > conn = gwlist_get(smsc_list, (i+s) % gwlist_len(smsc_list)); > > @@ -1265,6 +1310,8 @@ > bo_load = info.load; > } > } > + } > + > queue_length += gwlist_len(outgoing_sms); > if (max_outgoing_sms_qlength > 0 && !resend && > queue_length > gwlist_len(smsc_list) * max_outgoing_sms_qlength) { > Index: gw/msg-decl.h > =================================================================== > --- gw/msg-decl.h (revision 4838) > +++ gw/msg-decl.h (working copy) > @@ -85,6 +85,7 @@ > OCTSTR(msgdata) > INTEGER(time) > OCTSTR(smsc_id) > + OCTSTR(msg_ConnId); /* field required to reroute concatenated Msg parts via the same SMSC connection. Bug 529*/ > OCTSTR(smsc_number) > OCTSTR(foreign_id) > OCTSTR(service)
bug529.patch
(text/plain, 6.3 KB)
Index: bb_smscconn.c
===================================================================
--- bb_smscconn.c (revision 4843)
+++ bb_smscconn.c (working copy)
@@ -201,17 +201,46 @@
static void handle_split(SMSCConn *conn, Msg *msg, long reason)
{
struct split_parts *split = msg->sms.split_parts;
+ char id[UUID_STR_LEN + 1];
+
/*
* If temporarely failed, try again immediately but only if connection active.
* Because if connection is not active we will loop for ever here consuming 100% CPU
* time due to internal queue cleanup in smsc module that call bb_smscconn_failed.
*/
- if (reason == SMSCCONN_FAILED_TEMPORARILY && smscconn_status(conn) == SMSCCONN_ACTIVE &&
- smscconn_send(conn, msg) == 0) {
+ /*if (reason == SMSCCONN_FAILED_TEMPORARILY && smscconn_status(conn) == SMSCCONN_ACTIVE &&
+ smscconn_send(conn, msg) == 0) { */
/* destroy this message because it will be duplicated in smsc module */
- msg_destroy(msg);
+ /* msg_destroy(msg);
return;
+ }*/
+
+
+ if (reason == SMSCCONN_FAILED_TEMPORARILY && smscconn_status(conn) == SMSCCONN_ACTIVE)
+ {
+ if (sms_resend_retry >= 0 && msg->sms.resend_try >= sms_resend_retry) /*check how many times the split message has been sent */
+ {
+ uuid_unparse(msg->sms.id, id);
+ warning(0, "SMPP[%s] :Maximum retries for message [%s] exceeded resend %ld times, discarding it!", octstr_get_cstr(conn->id), id, msg->sms.resend_try);
+ handle_split(conn, msg, SMSCCONN_FAILED_DISCARDED);
+ return;
+ }
+
+
+ uuid_unparse(msg->sms.id, id);
+ warning(0, "SMPP[%s] :Split message [%s] failed temporary sending it to sms_router but with fixed connection", octstr_get_cstr(conn->id), id);
+
+ msg->sms.resend_try = (msg->sms.resend_try > 0 ? msg->sms.resend_try + 1 : 1); /* inc its retries */
+ debug("bb.sms.splits", 0, "SMPP[%s] :Resend counter of message [%s] set to %ld", octstr_get_cstr(conn->id), id, msg->sms.resend_try);
+ time(&msg->sms.resend_time); /* take resend time - this will be used in sms_router */
+
+
+ if(uuid_is_null(msg->msg_ConnId) == 1)
+ uuid_copy(msg->msg_ConnId, conn->smscconn_uuid);
+
+ gwlist_produce(outgoing_sms, msg); /* send it back to global queue */
+ return;
}
/*
@@ -1187,8 +1216,8 @@
long bp_load, bo_load;
int i, s, ret, bad_found, full_found;
long max_queue, queue_length;
- char *uf;
-
+ char *uf, id[UUID_STR_LEN + 1];
+
/* XXX handle ack here? */
if (msg_type(msg) != sms) {
error(0, "Attempt to route non SMS message through smsc2_rout!");
@@ -1229,6 +1258,29 @@
bp_load = bo_load = queue_length = 0;
conn = NULL;
+ /*if msg was a split and failed temporary then use stored connection */
+ if(uuid_is_null(msg->msg_ConnId) != 1)
+ {
+ for (i=0; i < gwlist_len(smsc_list); i++)
+ {
+ conn = gwlist_get(smsc_list, i);
+ smscconn_info(conn, &info);
+ queue_length += (info.queued > 0 ? info.queued : 0);
+
+ if(uuid_compare(msg->msg_ConnId, conn->smscconn_uuid) == 0)
+ {
+ if((smscconn_usable(conn,msg) != -1) && (info.status == SMSCCONN_ACTIVE && info.queued < max_queue))
+ {
+ best_preferred = conn;
+ uuid_unparse(msg->sms.id, id);
+ debug("bb.sms",0,"Message [%s] with pre-set connection found", id);
+ continue;
+ }
+ }
+ }
+ }
+ else
+ {
for (i=0; i < gwlist_len(smsc_list); i++) {
conn = gwlist_get(smsc_list, (i+s) % gwlist_len(smsc_list));
@@ -1265,6 +1317,8 @@
bo_load = info.load;
}
}
+ }
+
queue_length += gwlist_len(outgoing_sms);
if (max_outgoing_sms_qlength > 0 && !resend &&
queue_length > gwlist_len(smsc_list) * max_outgoing_sms_qlength) {
Index: msg.c
===================================================================
--- msg.c (revision 4843)
+++ msg.c (working copy)
@@ -98,6 +98,7 @@
msg = gw_malloc_trace(sizeof(Msg), file, line, func);
msg->type = type;
+ uuid_clear(msg->msg_ConnId);
#define INTEGER(name) p->name = MSG_PARAM_UNDEFINED;
#define OCTSTR(name) p->name = NULL;
#define UUID(name) uuid_generate(p->name);
@@ -113,7 +114,7 @@
Msg *new;
new = msg_create(msg->type);
-
+ uuid_copy(new->msg_ConnId, msg->msg_ConnId);
#define INTEGER(name) p->name = q->name;
#define OCTSTR(name) \
if (q->name == NULL) p->name = NULL; \
Index: msg.h
===================================================================
--- msg.h (revision 4843)
+++ msg.h (working copy)
@@ -78,7 +78,7 @@
typedef struct {
enum msg_type type;
-
+ uuid_t msg_ConnId;
#define INTEGER(name) long name;
#define OCTSTR(name) Octstr *name;
#define UUID(name) uuid_t name;
Index: smscconn.c
===================================================================
--- smscconn.c (revision 4843)
+++ smscconn.c (working copy)
@@ -157,13 +157,16 @@
Octstr *denied_prefix_regex;
Octstr *preferred_prefix_regex;
Octstr *tmp;
-
+ char id[UUID_STR_LEN + 1];
+
if (grp == NULL)
return NULL;
conn = gw_malloc(sizeof(*conn));
memset(conn, 0, sizeof(*conn));
+ uuid_generate(conn->smscconn_uuid);
+
conn->why_killed = SMSCCONN_ALIVE;
conn->status = SMSCCONN_CONNECTING;
conn->connect_time = -1;
@@ -295,7 +298,10 @@
gw_assert(conn->send_msg != NULL);
bb_smscconn_ready(conn);
-
+
+ uuid_unparse(conn->smscconn_uuid, id);
+ debug("smscconn",0,"Adding smsc connection [%s] with id [%s]", octstr_get_cstr(conn->id), id);
+
return conn;
}
Index: smscconn_p.h
===================================================================
--- smscconn_p.h (revision 4843)
+++ smscconn_p.h (working copy)
@@ -146,6 +146,8 @@
#include "smscconn.h"
struct smscconn {
+ /* variable added in order to uniquely identify each smsc connection */
+ uuid_t smscconn_uuid;
/* variables set by appropriate SMSCConn driver */
smscconn_status_t status; /* see smscconn.h */
int load; /* load factor, 0 = no load */