Re: smsbox - url_result_thread

Hi Fred,

you changes may be really bug fixes and usefull but when you send you 
code sniplets it's really hard to follow. Please send it as patches 
(unified diff) and then we would be abe to get some response to your 
changes.

Thanks,
Alex


fred wrote:
> 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] 
> <mailto:[email protected]>>
> To: "fred" <[email protected] <mailto:[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] <mailto:[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 <%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