Re: [PATCH] Guaranteed throughput smsc-independent

Hi list,

as no objections were there, patch commited to cvs...

Thanks,
Alex

Am 29.06.2009 um 09:45 schrieb Alexander Malysh:

> Hi,
>
> see answers inline...
>
> Am 25.06.2009 um 22:55 schrieb Damian Viano:
>
>> On Fri, Jun 19, 2009 at 05:50:16PM +0200, Alexander Malysh wrote:
>>> Hi,
>>>
>>> ok, here we go. This is relative large patch because too much  
>>> things  must be
>>> merged from private version into official.
>>
>> Sorry to take this long to get back to you.  I've tested it and the  
>> results
>> looks promising, only some comments inlined below (I left only the  
>> parts I
>> considered relevant for the comments).
>>
>>> Please test attached patch against CVS head or checkout/follow  
>>> from/on:
>>> http://github.com/amalysh/kannel/tree/smpp-throttling
>>
>> I have to say I backported it to stable and test it there only  
>> since I need
>> this for production and I'm not inclined to put a full cvs version  
>> just yet.
>>
>> Also, I haven't tested it in production yet, only against a dumb  
>> smsc based
>> on Net::SMPP.
>>
>>> This patch implements following:
>>> - new main loop logic
>>> - some inlines for speed
>>
>> Nice catch :)
>>
>>> - more error handling
>>> - async shutdown
>>
>> I didn't test this.
>>
>> diff --git a/gw/smsc/smsc_smpp.c b/gw/smsc/smsc_smpp.c
>> index cf61122..60cd2df 100644
>> --- a/gw/smsc/smsc_smpp.c
>> +++ b/gw/smsc/smsc_smpp.c
>> @@ -79,6 +79,7 @@
>> #include "dlr.h"
>> #include "bearerbox.h"
>> #include "meta_data.h"
>> +#include "load.h"
>>
>> #define SMPP_DEFAULT_CHARSET "UTF-8"
>>
>> @@ -111,7 +112,7 @@
>> #define SMPP_MAX_PENDING_SUBMITS    10
>> #define SMPP_DEFAULT_VERSION        0x34
>> #define SMPP_DEFAULT_PRIORITY       0
>> -#define SMPP_THROTTLING_SLEEP_TIME  15
>> +#define SMPP_THROTTLING_SLEEP_TIME  1
>>
>>
>> Why change this? I know it's no big deal, but I just don't get why  
>> we don't
>> want to stop sending for a longer while if the smsc tell us to.
>
>
> normally you will receive throttling error only for 1 sec timeslot.  
> Therefore it doesn't make sense to sleep
> more as 1 second because next second you should be able to send again.
>
>
>>
>>
>> #define SMPP_DEFAULT_CONNECTION_TIMEOUT  10 *  
>> SMPP_ENQUIRE_LINK_INTERVAL
>> #define SMPP_DEFAULT_WAITACK        60
>> #define SMPP_DEFAULT_SHUTDOWN_TIMEOUT 30
>>
>>
>> Shouldn't be SMPP_DEFAULT_SHUTDOWN_TIMEOUT usually >=  
>> SMPP_DEFAULT_WAITACK so
>> we wait for the acks instead of shutting down and not getting them?  
>> (I know
>> this is not exactly your patch, but...)
>
> it could be but we don't want to wait forever for some buggy or  
> overloaded SMSCs. Again normally SMSC
> will send all responses within 30sec.
>
>>
>>
>> @@ -912,15 +917,15 @@ static SMPP_PDU *msg_to_pdu(SMPP *smpp, Msg  
>> *msg)
>>     * Note: we always send in UTC and just define "Time Difference"  
>> as 00 and
>>     *       direction '+'.
>>     */
>> -    validity = msg->sms.validity >= 0 ? msg->sms.validity : smpp- 
>> >validityperiod;
>> -    if (validity >= 0) {
>> +    validity = msg->sms.validity != SMS_PARAM_UNDEFINED ? msg- 
>> >sms.validity : smpp->validityperiod;
>> +    if (validity != SMS_PARAM_UNDEFINED) {
>>
>>
>> Could this still be SMS_PARAM_UNDEFINED after the line above? (not  
>> sure if
>> msg->sms.validity or smpp->validityperiod can be SMS_PARAM_UNDEFINED)
>
> I don't get your question... This line means: set validity from msg  
> struct or smsc config but validity from msg struct has priority.
> Sure can validity still be undefined therefore we check it...
>
>>
>>
>>        struct tm tm = gw_gmtime(time(NULL) + validity * 60);
>>        pdu->u.submit_sm.validity_period = octstr_format("%02d%02d 
>> %02d%02d%02d%02d000+",
>>                tm.tm_year % 100, tm.tm_mon + 1, tm.tm_mday,
>>                tm.tm_hour, tm.tm_min, tm.tm_sec);
>>    }
>>
>> -    if (msg->sms.deferred >= 0) {
>> +    if (msg->sms.deferred != SMS_PARAM_UNDEFINED) {
>>        struct tm tm = gw_gmtime(time(NULL) + msg->sms.deferred * 60);
>>        pdu->u.submit_sm.schedule_delivery_time = octstr_format("%02d 
>> %02d%02d%02d%02d%02d000+",
>>                tm.tm_year % 100, tm.tm_mon + 1, tm.tm_mday,
>> @@ -1860,94 +1897,125 @@ static void io_thread(void *arg)
>>            conn = open_transceiver(smpp);
>>        else
>>            conn = open_receiver(smpp);
>> -
>> -        last_enquire_sent = last_cleanup = last_response =  
>> date_universal_now();
>> +
>>        pending_submits = -1;
>>        len = 0;
>> -        smpp->throttling_err_time = 0;
>> -        for (;conn != NULL;) {
>> -            timeout = last_enquire_sent + smpp- 
>> >enquire_link_interval
>> -                        - date_universal_now();
>> -
>> -            if (conn_wait(conn, timeout) == -1)
>> +        last_response = last_cleanup = last_enquire_sent =  
>> time(NULL);
>> +        while(conn != NULL) {
>> +            ret = read_pdu(smpp, conn, &len, &pdu);
>> +            if (ret == -1) { /* connection broken */
>> +                error(0, "SMPP[%s]: I/O error or other error. Re- 
>> connecting.",
>> +                      octstr_get_cstr(smpp->conn->id));
>>                break;
>> -
>> -            /* unbind
>> -             * Read so long as unbind_resp received or timeout  
>> passed. Otherwise we have
>> -             * double delivered messages.
>> -             */
>> -            if (smpp->quitting) {
>> -                send_unbind(smpp, conn);
>> -                last_response = time(NULL);
>> -                while(conn_wait(conn, 1.00) != -1 &&
>> -                      difftime(time(NULL), last_response) <  
>> SMPP_DEFAULT_SHUTDOWN_TIMEOUT &&
>> -                      smpp->conn->status != SMSCCONN_DISCONNECTED) {
>> -                    if (read_pdu(smpp, conn, &len, &pdu) == 1) {
>> -                        dump_pdu("Got PDU:", smpp->conn->id, pdu);
>> -                        handle_pdu(smpp, conn, pdu,  
>> &pending_submits);
>> -                        smpp_pdu_destroy(pdu);
>> -                    }
>> +            } else if (ret == -2) {
>> +                /* wrong pdu length , send gnack */
>> +                len = 0;
>> +                if (send_gnack(smpp, conn, SMPP_ESME_RINVCMDLEN,  
>> 0) == -1) {
>> +                    error(0, "SMPP[%s]: I/O error or other error.  
>> Re-connecting.",
>> +                          octstr_get_cstr(smpp->conn->id));
>> +                    break;
>>                }
>> -                debug("bb.sms.smpp", 0, "SMPP[%s]: %s: break and  
>> shutting down",
>> -                      octstr_get_cstr(smpp->conn->id), __func__);
>> -
>> -                break;
>> -            }
>> -
>> -            send_enquire_link(smpp, conn, &last_enquire_sent);
>> -
>> -            while ((ret = read_pdu(smpp, conn, &len, &pdu)) == 1) {
>> -                last_response = time(NULL);
>> +            } else if (ret == 1) { /* data available */
>>                /* Deal with the PDU we just got */
>>                dump_pdu("Got PDU:", smpp->conn->id, pdu);
>> -                handle_pdu(smpp, conn, pdu, &pending_submits);
>> +                ret = handle_pdu(smpp, conn, pdu, &pending_submits);
>>                smpp_pdu_destroy(pdu);
>> -
>> +                if (ret == -1) {
>> +                    error(0, "SMPP[%s]: I/O error or other error.  
>> Re-connecting.",
>> +                          octstr_get_cstr(smpp->conn->id));
>> +                    break;
>> +                }
>> +
>> +                /*
>> +                 * check if we are still connected
>> +                 * Note: Function handle_pdu will set status to  
>> SMSCCONN_DISCONNECTED
>> +                 * when unbind was received.
>> +                 */
>> +                if (smpp->conn->status == SMSCCONN_DISCONNECTED)
>> +                    break;
>> +
>>                /*
>> -                 * check if we are still connected.
>> -                 * Note: smsc can send unbind request, so we must  
>> check it.
>> +                 * If we are not bounded then no PDU may coming  
>> from SMSC.
>> +                 * It's just a workaround for buggy SMSC's whoes  
>> send enquire_link's
>> +                 * although link is not bounded. Means: we doesn't  
>> notice these and if link
>> +                 * keep to be not bounden we are reconnect after  
>> defined timeout elapsed.
>>                 */
>> -                if (smpp->conn->status != SMSCCONN_ACTIVE && smpp- 
>> >conn->status != SMSCCONN_ACTIVE_RECV) {
>> -                     /* we are disconnected */
>> -                     ret = -1;
>> -                     break;
>> +                if (IS_ACTIVE) {
>> +                    /*
>> +                     * Store last response time.
>> +                     */
>> +                    time(&last_response);
>>                }
>> -
>> -                /* Make sure we send enquire_link even if we read  
>> a lot */
>> -                send_enquire_link(smpp, conn, &last_enquire_sent);
>> -
>> -                /* Make sure we send even if we read a lot */
>> -                if (transmitter && difftime(time(NULL), smpp- 
>> >throttling_err_time) > SMPP_THROTTLING_SLEEP_TIME) {
>> -                    smpp->throttling_err_time = 0;
>> -                    send_messages(smpp, conn, &pending_submits);
>> +            } else { /* no data available */
>> +                /* check last enquire_resp, if difftime > as  
>> idle_timeout
>> +                 * mark connection as broken.
>> +                 * We have some SMSC connections where connection  
>> seems to be OK, but
>> +                 * in reallity is broken, because no responses  
>> received.
>> +                 */
>> +                if (smpp->connection_timeout > 0 &&
>> +                    difftime(time(NULL), last_response) > smpp- 
>> >connection_timeout) {
>> +                    /* connection seems to be broken */
>> +                    warning(0, "Got no responses within %ld sec.,  
>> reconnecting...",
>> +                            (long) difftime(time(NULL),  
>> last_response));
>> +                    break;
>>                }
>> +
>> +                time(&now);
>> +                timeout = last_enquire_sent + smpp- 
>> >enquire_link_interval - now;
>> +                if (!IS_ACTIVE && timeout <= 0)
>> +                    timeout = smpp->enquire_link_interval;
>> +                if (transmitter && gw_prioqueue_len(smpp- 
>> >msgs_to_send) > 0 &&
>> +                    smpp->throttling_err_time > 0 &&  
>> pending_submits < smpp->max_pending_submits) {
>> +                    time_t tr_timeout = smpp->throttling_err_time  
>> + SMPP_THROTTLING_SLEEP_TIME - now;
>> +                    timeout = timeout > tr_timeout ? tr_timeout :  
>> timeout;
>> +                } else if (transmitter && gw_prioqueue_len(smpp- 
>> >msgs_to_send) > 0 && smpp->conn->throughput > 0 &&
>> +                           smpp->max_pending_submits >  
>> pending_submits) {
>> +                    double t = 1.0 / smpp->conn->throughput;
>> +                    timeout = t < timeout ? t : timeout;
>> +                }
>>
>>
>> I've struggled with this logic for a while and even if I understand  
>> what the
>> code does I can't seem to get a grasp of the semantic meaning or  
>> algorithm
>> behind it. So definitely some comments would be greatly appreciated.
>
> Sorry but I think it's just impossible to document... Just read code  
> and think about all possible cases. Some comments will give
> you a hint why such decision was taken.
>
>>
>>
>> +                /* sleep a while */
>> +                if (timeout > 0 && conn_wait(conn, timeout) == -1)
>> +                    break;
>>            }
>>
>>
>>
>> Thanks for your patch, and hope my comments are welcome.
>>
>> Damián Viano(Des).
>