RE: [PATCH] fix bug #529 (sms-resend-* ignored for concatenated messages)

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)