[PATCH] dlr-retry with no-remove option

Giulio Giovannini <[email protected]> Tue, 26 Sep 2017 11:54:59 +0200

Newsgroups	gmane.comp.mobile.kannel.devel
Message-ID	<CAPe=cWYH+SMgfm7u5XX_w1mVqSgJiDmOM=aH_nxJq52rffgchg@mail.gmail.com>

Hello all,

I propose a patch with is actually an enhancement of the
kannel-dlr-retry.patch available here:
http://www.blogalex.com/wp-content/uploads/2009/05/kannel-dlr-retry.patch

This patch includes the kannel-dlr-retry.patch and adds the feature
described below. I could not produce an independent patch because it
touches the same lines of code of kannel-dlr-retry.patch and I could not
produce an independent patch that could be successfully applied on top of
it.

*New feature*
Gives the possibility to the user to avoid deleting the rows from the dlr
persistence resource for sms that have received the final DLR. Instead, it
just updates the 'status' field as done for intermediate DLR.
Introduces a new core parameter 'dlr-no-remove'. If defined and set to 1,
dlr will not be removed. If you leave it out or set to 0, behaviour reverts
to non-patched kannel. That is, final dlr will be removed.

*Reason for the patch*
We use the dlr-retry patch which is good to lower the number of DLRs that
get discarded due to the missing row in the persistence resource.
Unfortunately, the retries slow down the reception of DLR as kannel
performs the retries before sending the ACK to the SMSC for the DLR.
If it happens that, as it happens, a SMSC starts to re-send many times the
same DLRs which have already been received and deleted, the connection
stops working due to the very slow response for each re-sent DLR not found
and retried.
This patch helps to fix this problem as DLR that have been already received
can be found again with a single select without retries. That allows kannel
to reply quickly with the ACK. At the same time you can have long retries
to make sure that kannel waits long enough to find the DLR in its
persistence resource.

Some important details on this patch:

1) The patch is able to understand if it is handling a DLR for the first
time of it is a retry. In case of retries, it does not trigger any further
handling of the DLR and just replies to the SMSC. In this way any DLR
processing you might have will only see the first DLR reception and will
not be aware of retries.
2) Using the dlr-no.remove parameter will increase sensibly the size of
rows in your persistence resource. Of course some mechanism must be put in
place to delete the final dlrs that don't risk to be received again. Final
dlrs can be selected based on the value of the 'status' field.

Best regards,
Giulio

p.s.: is there any plan to include the  kannel-dlr-retry.patch in the head
of kannel?

dlr-retry-no-remove.patch (application/octet-stream, 11.1 KB)

Index: gw/dlr.c
===================================================================
--- gw/dlr.c	(revision 5188)
+++ gw/dlr.c	(working copy)
@@ -234,6 +234,7 @@
 {
     CfgGroup *grp;
     Octstr *dlr_type;
+    long retry_delay;
 
     /* check which DLR storage type we are using */
     grp = cfg_get_single_group(cfg, octstr_imm("core"));
@@ -282,10 +283,35 @@
    	    panic(0, "DLR: storage type '%s' is not supported!", octstr_get_cstr(dlr_type));
     }
 
+    if (cfg_get_integer(&handles->no_remove, grp, octstr_imm("dlr-no-remove")) == -1) {
+        handles->no_remove = 0;
+    }
+
     /* check needed function pointers */
-    if (handles->dlr_add == NULL || handles->dlr_get == NULL || handles->dlr_remove == NULL)
-        panic(0, "DLR: storage type '%s' don't implement needed functions", octstr_get_cstr(dlr_type));
+    if (handles->dlr_add == NULL)
+        panic(0, "DLR: storage type '%s' don't implement needed function dlr_add()", octstr_get_cstr(dlr_type));
 
+    if (handles->dlr_get == NULL)
+        panic(0, "DLR: storage type '%s' don't implement needed function dlr_get()", octstr_get_cstr(dlr_type));
+
+    if (handles->no_remove){
+        /* DLR are only update and not removed. Check for dlr_update() */
+        if (handles->dlr_update == NULL)
+            panic(0, "DLR: storage type '%s' don't implement needed function dlr_update()", octstr_get_cstr(dlr_type));
+    } else {
+        /* DLR are removed. Check for dlr_remove() */
+        if (handles->dlr_remove == NULL)
+            panic(0, "DLR: storage type '%s' don't implement needed function dlr_remove()", octstr_get_cstr(dlr_type));
+    }
+
+    if (cfg_get_integer(&handles->retry_count, grp, octstr_imm("dlr-retry-count")) == -1) {
+       handles->retry_count = 1;
+    }
+    if (cfg_get_integer(&retry_delay, grp, octstr_imm("dlr-retry-delay")) == -1) {
+       retry_delay = 0;
+    }
+    handles->retry_delay = (double)retry_delay/1000.0;
+
     /* get info from storage */
     info(0, "DLR using storage type: %s", handles->type);
 
@@ -390,6 +416,7 @@
     struct dlr_entry *dlr = NULL;
     Octstr *dst_min = NULL;
     Octstr *dlr_mask;
+    int retry = 0;
     
     if(octstr_len(smsc) == 0) {
 	warning(0, "DLR[%s]: Can't find a dlr without smsc-id", dlr_type());
@@ -410,13 +437,31 @@
     debug("dlr.dlr", 0, "DLR[%s]: Looking for DLR smsc=%s, ts=%s, dst=%s, type=%d",
                                  dlr_type(), octstr_get_cstr(smsc), octstr_get_cstr(ts), octstr_get_cstr(dst), typ);
 
-    dlr = handles->dlr_get(smsc, ts, dst_min);
-    if (dlr == NULL)  {
-        warning(0, "DLR[%s]: DLR from SMSC<%s> for DST<%s> not found.",
-                dlr_type(), octstr_get_cstr(smsc), octstr_get_cstr(dst));         
-        return NULL;
+       /*
+        * Retry the dlr search on the DB if not found at first attempt.
+        * This could happen specially with DB storage when using separate
+        * binds for sending and receiving.
+        */
+    while(retry < handles->retry_count) {
+       if (retry++ > 0) {
+               debug("dlr.dlr", 0, "Sleeping for %1.3f seconds", handles->retry_delay);
+               gwthread_sleep(handles->retry_delay);
+       }
+       dlr = handles->dlr_get(smsc, ts, dst_min);
+
+       if (dlr != NULL)
+                       break;
+
+               debug("dlr.dlr", 0, "DLR from SMSC<%s> for DST<%s>. Attempt %d of %d.",
+                               octstr_get_cstr(smsc), octstr_get_cstr(dst), retry, handles->retry_count);
     }
 
+    if (dlr == NULL) {
+               warning(0, "DLR[%s]: DLR from SMSC<%s> for DST<%s> not found after %d attempts.",
+                               dlr_type(), octstr_get_cstr(smsc), octstr_get_cstr(dst), handles->retry_count);
+               return NULL;
+    }
+
 #define O_SET(x, val) if (octstr_len(val) > 0) { x = val; val = NULL; }
 
     if ((typ & dlr->mask) > 0) {
@@ -437,26 +482,43 @@
          * later in the smsc module 
          */
         msg->sms.msgdata = NULL;
-        /* 
-         * If a boxc_id is available, then instruct bearerbox to 
-         * route this msg back to originating smsbox
-         */
-        O_SET(msg->sms.boxc_id, dlr->boxc_id);
-        /*
-         * We will provide the DLR request bitmask in the DLR going back
-         * to the smsbox connection for processing. This allows smsbox
-         * connection type applications to avoid temporary storing and
-         * resolving data to pull up this information.
-         */
-        dlr_mask = octstr_format("%ld", dlr->mask);
-        msg->sms.meta_data = octstr_create("");
-        meta_data_set_value(msg->sms.meta_data, METADATA_ORIG_MSG_GROUP,
-                            octstr_imm(METADATA_ORIG_MSG_GROUP_DLR_MASK), dlr_mask, 1);
-        octstr_destroy(dlr_mask);
 
-        time(&msg->sms.time);
-        debug("dlr.dlr", 0, "DLR[%s]: created DLR message for URL <%s>",
-                      dlr_type(), (msg->sms.dlr_url?octstr_get_cstr(msg->sms.dlr_url):""));
+        if (DLR_IS_SUCCESS_OR_FAIL(dlr->status)) {
+            /* This is a final DLR that we have already received and updated status
+             * (probably due to usage of dlr-no-remove). It is a DLR we have
+             * already processed. Do not process it again */
+            if (dlr->status != typ){
+                /* ERROR! SMSC has sent a further DLR after final one and dlr-mask is different! */
+                error(0, "DLR[%s]: DLR re-sent after final DLR and dlr-mask is different! [current mask:%d] [new mask:%d]",
+                        dlr_type(), dlr->status, typ);
+            } else {
+                debug("dlr.dlr", 0, "DLR[%s]: Final DLR has already status set to %d (alreayd received?). Do not process further.",
+                          dlr_type(), typ, dlr->status);
+            }
+            msg = NULL;
+        } else {
+            /* 
+             * If a boxc_id is available, then instruct bearerbox to 
+             * route this msg back to originating smsbox
+             */
+            O_SET(msg->sms.boxc_id, dlr->boxc_id);
+
+            /*
+             * We will provide the DLR request bitmask in the DLR going back
+             * to the smsbox connection for processing. This allows smsbox
+             * connection type applications to avoid temporary storing and
+             * resolving data to pull up this information.
+             */
+            dlr_mask = octstr_format("%ld", dlr->mask);
+            msg->sms.meta_data = octstr_create("");
+            meta_data_set_value(msg->sms.meta_data, METADATA_ORIG_MSG_GROUP,
+                                octstr_imm(METADATA_ORIG_MSG_GROUP_DLR_MASK), dlr_mask, 1);
+            octstr_destroy(dlr_mask);
+
+            time(&msg->sms.time);
+            debug("dlr.dlr", 0, "DLR[%s]: created DLR message for URL <%s>",
+                          dlr_type(), (msg->sms.dlr_url?octstr_get_cstr(msg->sms.dlr_url):""));
+        }
     } else {
         debug("dlr.dlr", 0, "DLR[%s]: Ignoring DLR message because of mask type=%d dlr->mask=%d", dlr_type(), typ, dlr->mask);
         /* ok that was a status report but we where not interested in having it */
@@ -468,16 +530,27 @@
     /* check for end status and if so remove from storage */
     if (DLR_IS_NOT_FINAL(typ) && DLR_IS_SUCCESS_OR_FAIL(dlr->mask)) {
         debug("dlr.dlr", 0, "DLR[%s]: DLR not destroyed, still waiting for other delivery report", dlr_type());
-        /* update dlr entry status if function defined */
-        if (handles != NULL && handles->dlr_update != NULL){
-            handles->dlr_update(smsc, ts, dst_min, typ);
+        if (dlr->status != typ){
+            /* update dlr entry status if function defined */
+            if (handles != NULL && handles->dlr_update != NULL){
+                handles->dlr_update(smsc, ts, dst_min, typ);
+            }
         }
     } else {
-        if (handles != NULL && handles->dlr_remove != NULL){
-            /* it's not good for internal storage, but better for all others */
-            handles->dlr_remove(smsc, ts, dst_min);
+        if (handles->no_remove){
+            if (dlr->status != typ){
+                /* We just update status, we don't delete the DLR*/
+                if (handles != NULL && handles->dlr_update != NULL){
+                    handles->dlr_update(smsc, ts, dst_min, typ);
+                }
+            }
         } else {
-            warning(0, "DLR[%s]: Storage don't have remove operation defined", dlr_type());
+            if (handles != NULL && handles->dlr_remove != NULL){
+                /* it's not good for internal storage, but better for all others */
+                handles->dlr_remove(smsc, ts, dst_min);
+            } else {
+                warning(0, "DLR[%s]: Storage don't have remove operation defined", dlr_type());
+            }
         }
     }
 
Index: gw/dlr_mysql.c
===================================================================
--- gw/dlr_mysql.c	(revision 5188)
+++ gw/dlr_mysql.c	(working copy)
@@ -153,10 +153,11 @@
     else
         like = octstr_imm("");
 
-    sql = octstr_format("SELECT `%S`, `%S`, `%S`, `%S`, `%S`, `%S` FROM `%S` WHERE `%S`=? AND `%S`=? %S LIMIT 1",
+    sql = octstr_format("SELECT `%S`, `%S`, `%S`, `%S`, `%S`, `%S`, `%S` FROM `%S` WHERE `%S`=? AND `%S`=? %S LIMIT 1",
                         fields->field_mask, fields->field_serv,
                         fields->field_url, fields->field_src,
                         fields->field_dst, fields->field_boxc,
+                        fields->field_status,
                         fields->table, fields->field_smsc,
                         fields->field_ts, like);
 
@@ -193,6 +194,7 @@
         res->source = octstr_create(LO2CSTR(row, 3));
         res->destination = octstr_create(LO2CSTR(row, 4));
         res->boxc_id = octstr_create(LO2CSTR(row, 5));
+        res->status = atoi(LO2CSTR(row,6));
         gwlist_destroy(row, octstr_destroy_item);
         res->smsc = octstr_duplicate(smsc);
     }
Index: gw/dlr_p.h
===================================================================
--- gw/dlr_p.h	(revision 5188)
+++ gw/dlr_p.h	(working copy)
@@ -84,6 +84,7 @@
    Octstr *url;
    Octstr *boxc_id;
    int mask;
+   int status;
    int use_dst;
 };
 
@@ -111,6 +112,10 @@
      */
     const char* type;
     /*
+     * Do not delete DLR even when final
+     */
+    long no_remove;
+    /*
      * Add dlr entry into storage.
      * NOTE: this function is responsible to destroy struct dlr_entry
      */
@@ -140,6 +145,14 @@
      * Shutdown storage
      */
     void (*dlr_shutdown) (void);
+    /*
+     * How many times do we retry the dlr_find?
+     */
+    long retry_count;
+    /*
+     * Delay between the retries 
+     */
+    double retry_delay;
 };
 
 /*
Index: gwlib/cfg.def
===================================================================
--- gwlib/cfg.def	(revision 5188)
+++ gwlib/cfg.def	(working copy)
@@ -132,7 +132,10 @@
     OCTSTR(ssl-client-cipher-list)
     OCTSTR(ssl-server-cipher-list)
     OCTSTR(dlr-storage)
+    OCTSTR(dlr-retry-count)
+    OCTSTR(dlr-retry-delay)
     OCTSTR(dlr-spool)
+    OCTSTR(dlr-no-remove)
     OCTSTR(maximum-queue-length)    /* deprecated, supported until next major stable release */
     OCTSTR(sms-incoming-queue-limit)
     OCTSTR(sms-outgoing-queue-limit)