Re: smsbox - url_result_thread

Hi Juan,
yes you are correct, but that is not the only code I did for that
....what I did....used msg_duplicate  instead


static void *remember_receiver(Msg *msg, URLTranslation *trans, int method,
                               Octstr *url, List *headers, Octstr *body,
                               unsigned int retries)
{
    struct receiver *receiver;

    counter_increase(num_outstanding_requests);
    receiver = gw_malloc(sizeof(*receiver));

    receiver->msg = msg_duplicate (msg);   // why create another clean sms struct? why?why?
  
  //msg_create(sms); see above

.............

I tested it with a smpp simulator (logica) probably done 1mill+ mesgs thru it already......
killed smsbox with big queue of messages, and all recovers on restarting smsbox!!!
Cheers
Fred





----- Original Message ----- 
From: "Juan Ignacio Germano" <[email protected]>
To: "fred" <[email protected]>
Sent: Thursday, December 08, 2005 1:45 AM
Subject: Re: smsbox - url_result_thread


Hi Fred. About sending the ACK after the smsbox has actually processed
the message, I posted not long ago a similar change, and this is how I
have been using it for a while. I wuold like to ask about your
implementation, since it wasn't so simple for me, maybe I am doing
something wrong, but let me know what you think about a couple of
issues I see. In your implementation you do this:

        get_receiver(id, &msg, &trans, &method, &req_url,
&req_headers, &req_body, &retries);

        if (status == HTTP_OK || status == HTTP_ACCEPTED)
        {
         mack =  msg_create(ack);  /* now that successfully sent,
write ack back to bearer (fz)*/
           gw_assert(mack != NULL);
           uuid_copy(mack->ack.id, msg->sms.id);
           mack->ack.time = msg->sms.time;
           mack->ack.nack = ack_success;

Thing is, msg->sms.id is not the original id (the one that should be
sent back in the ACK). Check the code for remember_receiver and
get_receiver:

in remember_receiver:

    receiver->msg = msg_create(sms);

    receiver->msg->sms.sender = octstr_duplicate(msg->sms.sender);
    receiver->msg->sms.receiver = octstr_duplicate(msg->sms.receiver);
    /* ppg_service_name should always be not NULL here */
    if (trans != NULL && (msg->sms.service == NULL || ppg_service_name
== NULL ||
        octstr_compare(msg->sms.service, ppg_service_name) != 0)) {
        receiver->msg->sms.service = octstr_duplicate(urltrans_name(trans));
    } else {
        receiver->msg->sms.service = octstr_duplicate(msg->sms.service);
    }
    receiver->msg->sms.smsc_id = octstr_duplicate(msg->sms.smsc_id);
    /* to remember if it's a DLR http get */
    receiver->msg->sms.sms_type = msg->sms.sms_type;

And is called:

            id = remember_receiver(msg, trans, method, req_url,
req_headers, req_body, ++retries);
// [ ... ]
        msg_destroy(msg);

So, the msg in receiver structure is not the received one, but a new
one, with a new sms.id. The original sms.id is lost in msg_destroy
after calling remember_receiver. Check your bearerbox log. You should
be getting something like

WARNING: bb_store: get ACK of message not found from store, strange?

Also, if the request gets not re queued (because max_http_retries >
retries), I think an ACK should be sent too (I use ack_failed), so the
message is removed from store file in bearerbox. Mind you this check
is performed both in url_result_thread and http_queue_thread.

What I did to overcome the first issue was modified the receiver
structure to store the sms.id to acknowledge once the request is
handled:

struct receiver {
    Msg *msg;
    URLTranslation *trans;
    int method;  /* the HTTP method to use */
    Octstr *url; /* the after pattern URL */
    List *http_headers;
    Octstr *body; /* body content of the request */
    unsigned long retries; /* number of performed retries */
    uuid_t uuid;
};

and wrote a function to send acks since has to be done from (at
least), three places in smsbox:

static void send_ack(uuid_t uuid, int ack_type, Msg *msg){
        Octstr *os;
        char my_id[UUID_STR_LEN + 1];
        Msg *mack;

        uuid_unparse(uuid, my_id);
        os = octstr_create(my_id);
        debug("smsbox.c", 0, "Sending ack id = %s", octstr_get_cstr(os));
        octstr_destroy(os);
        mack = msg_create(ack);
        mack->ack.nack = ack_type;
        mack->ack.time = msg->sms.time;
        uuid_copy(mack->ack.id,uuid);
        write_to_bearerbox(mack); /* implicit msg_destroy */
}

and would use:

        get_receiver(id, &msg, &trans, &method, &req_url,
&req_headers, &req_body, &retries, uuid);

        if (status == HTTP_OK || status == HTTP_ACCEPTED) {
                        send_ack(uuid, ack_success, msg);

There are some parts of my own code I don't like (like passing msg to
send_ack just to use msg->sms.time), but It works for me.

Strange that when I wrote to the list about this, I wouldn't get any
comment either. I think this change is good because should the smsbox
died, you would still have your msgs in storefile, which would be
re-routed by bearerbox to another smsbox that could handle the msgs.

Tell me what you think, maybe I understood something incorrectly from
source? Anyone in the list care to comment about this?

Cheers!

Juan

On 12/5/05, fred <[email protected]> wrote:
>
> I have a problem with kannel sending mt messages without the application
> knowing about it...i am not sure what
> the reasoning for this is, but it seems a whole lot of code in
> url_result_thread() is quite incongruous to me!!
>
> here is my revamped version
>
> static void url_result_thread(void *arg)
> {
>     Octstr *final_url, *req_body, *type, *replytext;
>     List *reply_headers;
>     List *req_headers;
>     int status, method;
>     void *id;
>     Msg *msg;
>     Msg *mack;
>     URLTranslation *trans;
>     Octstr *req_url;
>     Octstr *text_html, *text_plain, *text_wml, *text_xml;
>     Octstr *octet_stream;
>     int octets;
>     unsigned long retries;
>
>     Octstr *reply_body;
>
>     for (;;)
>     {
>         id = http_receive_result(caller, &status, &final_url,
> &reply_headers,
>                  &reply_body);
>         if (id == NULL)
>             break;
>
>         get_receiver(id, &msg, &trans, &method, &req_url, &req_headers,
> &req_body, &retries);
>
>         if (status == HTTP_OK || status == HTTP_ACCEPTED)
>         {
>            mack =  msg_create(ack);  /* now that successfully sent, write
> ack back to bearer (fz)*/
>            gw_assert(mack != NULL);
>            uuid_copy(mack->ack.id, msg->sms.id);
>            mack->ack.time = msg->sms.time;
>            mack->ack.nack = ack_success;
>
>            write_to_bearerbox(mack); /* implicit msg_destroy */
>
>         }
>         else if (max_http_retries > retries)
>         {
>             id = remember_receiver(msg, trans, method, req_url, req_headers,
> req_body, retries);
>             gwlist_produce(smsbox_http_requests, id);
>             goto requeued;
>         }
>         else
>         {
>              warning(0,"could not fetch url:%s;retried %d, msg; receiver %s,
> sender %s",
>                        octstr_get_cstr(req_url),
>
> retries,octstr_get_cstr(msg->sms.receiver),octstr_get_cstr(msg->sms.sender)
>                    );
>         }
>
> requeued:
>         octstr_destroy(final_url);
>         http_destroy_headers(reply_headers);
>         octstr_destroy(reply_body);
>         octstr_destroy(req_url);
>         http_destroy_headers(req_headers);
>         octstr_destroy(req_body);
>         req_body=req_url=NULL;
>         req_headers=reply_headers=NULL;
>         msg_destroy(msg);
>     }
> }
>
>
>
> Yes, it has also been changed so that the ack goes back to bearerbox only
> after smsbox has successfully
> passed the message on, which is how it should be.
>
> So this function is now much cleaner, have I lost the plot about something
> ?? please advise ?
>
>


--
Juan