Re: [PATCH] preliminary confirmed sendsms
> > To allow old behavior, add to the smsbox group: > > immediate-sendsms-reply = true > > I think this is a bit confusing and we should look for a better name and > explanation. Also we need proper changes in user guide I think. There is changes in user guide (second commit), but if you come up with better name, surely it can be changed.. But hopefully no one really needs that variable anyway =] -- &Kalle Marjola ::: Development ::: Helsinki ::: Enpocket
Re: [PATCH] preliminary confirmed sendsms
Enver ALTIN wrote:
On Fri, 2005-01-28 at 10:10 +0200, Kalle Marjola wrote:
On Fri, 2005-01-28 at 09:29, Kalle Marjola wrote:
AFAIK, this has yet not been commited to cvs, right?
Nope, I just digged it out..
I can commit it to CVS (change things commented when I last time sent it
out) right now..
This is now committed to CVS.
Please all note that this is COMPATIBILITY breaker to those sendsms HTTP
clients who are actively investigating the content of the reply sent by
Kannel.
To allow old behavior, add to the smsbox group:
immediate-sendsms-reply = true
I think this is a bit confusing and we should look for a better name and
explanation. Also we need proper changes in user guide I think.
hmm, I think it is pretty semantic, but too long for my taste :|
Any other ideas for it?
Stipe
mailto:stolj_{at}_wapme.de
---
Wapme Systems AG
Vogelsanger Weg 80
40470 Düsseldorf, NRW, Germany
phone: +49.211.74845.0
fax: +49.211.74845.299
mailto:info_{at}_wapme-systems.de
http://www.wapme-systems.de/
---
Re: [PATCH] preliminary confirmed sendsms
Kalle Marjola wrote:
On Fri, 2005-01-28 at 09:29, Kalle Marjola wrote:
AFAIK, this has yet not been commited to cvs, right?
Nope, I just digged it out..
I can commit it to CVS (change things commented when I last time sent it
out) right now..
This is now committed to CVS.
Please all note that this is COMPATIBILITY breaker to those sendsms HTTP
clients who are actively investigating the content of the reply sent by
Kannel.
ok, I will update NEWS file already in preparation for 1.4.1 stable, so we don't
get this major compatibility breaker forgotten.
Stipe
mailto:stolj_{at}_wapme.de
---
Wapme Systems AG
Vogelsanger Weg 80
40470 Düsseldorf, NRW, Germany
phone: +49.211.74845.0
fax: +49.211.74845.299
mailto:info_{at}_wapme-systems.de
http://www.wapme-systems.de/
---
Re: [PATCH] preliminary confirmed sendsms
On Fri, 2005-01-28 at 10:10 +0200, Kalle Marjola wrote: > On Fri, 2005-01-28 at 09:29, Kalle Marjola wrote: > > > > > > AFAIK, this has yet not been commited to cvs, right? > > > > > Nope, I just digged it out.. > > I can commit it to CVS (change things commented when I last time sent it > > out) right now.. > > > This is now committed to CVS. > Please all note that this is COMPATIBILITY breaker to those sendsms HTTP > clients who are actively investigating the content of the reply sent by > Kannel. > > To allow old behavior, add to the smsbox group: > immediate-sendsms-reply = true I think this is a bit confusing and we should look for a better name and explanation. Also we need proper changes in user guide I think. -- Enver ALTIN |http://skyblue.gen.tr/ Software developer @ Parkyeri | http://www.parkyeri.com/ signature.asc Description: This is a digitally signed message part
Re: [PATCH] preliminary confirmed sendsms
On Fri, 2005-01-28 at 09:29, Kalle Marjola wrote: > > > > AFAIK, this has yet not been commited to cvs, right? > > > Nope, I just digged it out.. > I can commit it to CVS (change things commented when I last time sent it > out) right now.. > This is now committed to CVS. Please all note that this is COMPATIBILITY breaker to those sendsms HTTP clients who are actively investigating the content of the reply sent by Kannel. To allow old behavior, add to the smsbox group: immediate-sendsms-reply = true -- &Kalle Marjola ::: Development ::: Helsinki ::: Enpocket
Re: [PATCH] preliminary confirmed sendsms
> I'm +1 for a)! without reviewing the code, but from the semantical > perspective. > > AFAIK, this has yet not been commited to cvs, right? > Nope, I just digged it out.. I can commit it to CVS (change things commented when I last time sent it out) right now.. > Kalle, what is the consensus about it in the group?` > As normally, consensus is based on 2-3 voices =] So I implemented new configuration variable for this, 'immediate-sendsms-reply' which must be set to 'true' to make it behave like it did earlier; as a default, new system is going to be used. (it is a compatibility breaker then as it changes the HTTP reply text content/code) -- &Kalle Marjola ::: Development ::: Helsinki ::: Enpocket
Re: [PATCH] preliminary confirmed sendsms
Alexander Malysh wrote:
Kalle Marjola wrote:
On Thu, 2004-10-07 at 12:52, Alexander Malysh wrote:
Hi Kalle,
patch looks good except this "really horrible kludge"...
Well I thought that first check it out and if people like it
and we decide what to do about it (see choices a, b and c)
then I bother fixing that horrible kludge :]
oops sorry...
I'm +1 for (a)!
[ok, ages beyond the guys who are on the left lane ;)]
I'm +1 for a)! without reviewing the code, but from the semantical perspective.
AFAIK, this has yet not been commited to cvs, right?
Kalle, what is the consensus about it in the group?`
Stipe
mailto:stolj_{at}_wapme.de
---
Wapme Systems AG
Vogelsanger Weg 80
40470 Düsseldorf, NRW, Germany
phone: +49.211.74845.0
fax: +49.211.74845.299
mailto:info_{at}_wapme-systems.de
http://www.wapme-systems.de/
---
Re: [PATCH] preliminary confirmed sendsms
Yes I supposed that was the reason... implementing some kind of asynchronous processing (freeing the thread and processing the reject/accept when it arrives) would be a lot more complicated and DLR's already do that well. I suppose the "accepted-rejected by SMSC" DLRs are accomplished by setting a proper dlr-mask isnt't it? Anyone have tried this? Any hints? Regards, On Thu, 14 Oct 2004 10:57:59 +0300, Kalle Marjola <[EMAIL PROTECTED]> wrote: > On Thu, 2004-10-14 at 04:17, Alejandro Guerrieri wrote: > > Well, I've just patched my 1.3.2 with this and so far it seems to work ok. > > > > I've checked trying to send messages without defining an smsc and I've got: > > > > "Not routable. Do not try again." > > > > What I'm missing is the capability to detect wrong numbers, ie. if I > > try to send a message to "12345678" or "000" I've got an "0: > > Accepted for delivery", but I think I need DLR's for that isn't it? > > > > Isn't there any way to detect that kind of errors without DLR's? > > > Yes and no. What I understood from previous posts, these can be detected > with these 'accepted/rejected by SMSC' "DLRs" (not real DLRs, i.e. work > even when operator does not allow DLRs) > > However, this patch does not wait for that data as it can be delayed > lots more (queue inside SMSC driver etc.) - this will be the next patch > and even then that would be optional as it could easily result in HTTP > timeouts etc. (moreover, that patch is far more complicated because of > split messages etc.) > > > > > > > -- > &Kalle Marjola ::: Development ::: Helsinki ::: Enpocket > > -- Alejandro Guerrieri Magicom http://www.magicom-bcn.net/
Re: [PATCH] preliminary confirmed sendsms
On Thu, 2004-10-14 at 04:17, Alejandro Guerrieri wrote: > Well, I've just patched my 1.3.2 with this and so far it seems to work ok. > > I've checked trying to send messages without defining an smsc and I've got: > > "Not routable. Do not try again." > > What I'm missing is the capability to detect wrong numbers, ie. if I > try to send a message to "12345678" or "000" I've got an "0: > Accepted for delivery", but I think I need DLR's for that isn't it? > > Isn't there any way to detect that kind of errors without DLR's? > Yes and no. What I understood from previous posts, these can be detected with these 'accepted/rejected by SMSC' "DLRs" (not real DLRs, i.e. work even when operator does not allow DLRs) However, this patch does not wait for that data as it can be delayed lots more (queue inside SMSC driver etc.) - this will be the next patch and even then that would be optional as it could easily result in HTTP timeouts etc. (moreover, that patch is far more complicated because of split messages etc.) > > -- &Kalle Marjola ::: Development ::: Helsinki ::: Enpocket
Re: [PATCH] preliminary confirmed sendsms
On Thu, 2004-10-07 at 23:36, Alan McNatty wrote: > Hi Kalle, > > It would be good to understand more the impact on doing this (as you > note). Please consider the following scenario ... > > Often connections to SMSC are bandwidth limited (we have some SMPP links > over limited FR for example). What this can mean is (under load) that > there is not as much traffic inbound as out. > Please note that this patch does NOT wait that message is handled by SMSC driver (which would take time if there are queues), but instead gets an immediate response as soon as bearerbox replies. Thus the delay is milliseconds when compared to old one. The idea of waiting for SMSC ack/nack is in my plan. However, as this could be delayed even seconds, THAT feature would only be used if so configured/requested as it cannot be used with heavy traffic. (that feature would be based on current DLR implementation and there would be a list of possible problematic scenarios) > Also if load balancing with internal queues - in the event one SMSC link > is dropped the message queues are effectively shuffled. What would > happen if we're waiting to hear back from SMSC about message and the > link is dropped, etc? We would need to cope with these scenarios. > See above. This current reply is based on what bearerbox _immediately_ replies when it receives SMS from smsbox (and which was previously just discarded by smsbox). So, in exchange for some milliseconds, the caller gets information if there is some basic problems.. (no SMSCes connected, bad configuration,...) If you check out the original code (bb_boxc.c, line 253 and smsbox.c, line 191-196) these messages are already flying around (in fact, have been there for over 4 years...). They were just not used... (blame me =) -- &Kalle Marjola ::: Development ::: Helsinki ::: Enpocket
Re: [PATCH] preliminary confirmed sendsms
Hi Kalle, On Wed, 2004-10-06 at 15:52 +0300, Kalle Marjola wrote: > So, what does this first version do: > Instead of immediately saying '202 Send' for valid sendsms request, this > patch waits for bearerbox ACK before replying to HTTP client. > Valid replies are based on msg.h and are now: > 202 0: Accepted for delivery (routed to SMSC driver) > 202 3: Queued for later delivery (all SMSCes currently down) > 403 Forbidden (no SMSC comfigured to accept this, fix something) > 503 Service unavailable (queue full or other problem. Try again later) > > This is still far from ideal (to get SMSC reply), but better > than earlier. And this is how it should have been, always, > as why else these is those 'ack' type messages flying around... > (currently they are just discarded) I'm all for this. Actually I had to use DLR for an application I have developed, this gives a way better and immediate result about what happened about the transaction. So, choice-a is good enough and if I get the patch correctly, it is indeed backwards compatible; which is better. Thanks a lot, keep up the great work. -- Enver ALTIN |http://skyblue.gen.tr/ Software developer @ Parkyeri | http://www.parkyeri.com/ signature.asc Description: This is a digitally signed message part
Re: [PATCH] preliminary confirmed sendsms
Hi Kalle, I'm hoping I've understood your post. Apologies if I've lost the plot - I've put some questions/comments in-line below. On Thu, 2004-10-07 at 01:52, Kalle Marjola wrote: > So, what does this first version do: > Instead of immediately saying '202 Send' for valid sendsms request, this > patch waits for bearerbox ACK before replying to HTTP client. > Valid replies are based on msg.h and are now: > 202 0: Accepted for delivery (routed to SMSC driver) > 202 3: Queued for later delivery (all SMSCes currently down) > 403 Forbidden (no SMSC comfigured to accept this, fix something) > 503 Service unavailable (queue full or other problem. Try again later) Knowing up front that your message has been routed to SMSC (when not queued) would be IMHO a great improvement. > This is still far from ideal (to get SMSC reply), but better > than earlier. And this is how it should have been, always, > as why else these is those 'ack' type messages flying around... > (currently they are just discarded) > Performance notes: > * yes, it takes a microsecond longer for Kannel to reply to >sendsms. Moreover, if system is completyl bogged down, HTTP >queries start to build up, too. > * if SMS is multisent (several receivers), status and reply >is based on first ACK from bearerbox, rest are discarded It would be good to understand more the impact on doing this (as you note). Please consider the following scenario ... Often connections to SMSC are bandwidth limited (we have some SMPP links over limited FR for example). What this can mean is (under load) that there is not as much traffic inbound as out. With Kannel as it currently operates - we send and get a 202, the message is queued as SMSC's links are all busy. As a result internal queues can build up faster than messages get popped and sent. Success and failure is therefore measured by using DLR's (which of course don't help the throughput rates ;). Basically put Kannel can queue at a rate easily an order of magnitude greater than it can send let allow have messages acknowledged. What happens if actual delivery of messages takes longer than HTTP timeout? Would we be actually be making things more complicated? Also if load balancing with internal queues - in the event one SMSC link is dropped the message queues are effectively shuffled. What would happen if we're waiting to hear back from SMSC about message and the link is dropped, etc? We would need to cope with these scenarios. Cheers, Alan
Re: [PATCH] preliminary confirmed sendsms
+1 fo b) or c). This will change the default behavior and may broke alot of aplications that depend on the 202. You should be allowed to configure kannel to have the same behavior. On Wed, 06 Oct 2004 15:52:40 +0300, Kalle Marjola <[EMAIL PROTECTED]> wrote: > This patch is the first version of sendsms confirmations. > > This patch actually does what smsbox should have done like 4 years ago > but I was too lazy to implement it back then because it is a bit > complicated in logic. Later on I did that to NMGW, but as NMGW is > single-process, it was simpler in that. But now it is, at last, here for > original Kannel. > > So, what does this first version do: > Instead of immediately saying '202 Send' for valid sendsms request, this > patch waits for bearerbox ACK before replying to HTTP client. > Valid replies are based on msg.h and are now: > 202 0: Accepted for delivery (routed to SMSC driver) > 202 3: Queued for later delivery (all SMSCes currently down) > 403 Forbidden (no SMSC comfigured to accept this, fix something) > 503 Service unavailable (queue full or other problem. Try again later) > > This is still far from ideal (to get SMSC reply), but better > than earlier. And this is how it should have been, always, > as why else these is those 'ack' type messages flying around... > (currently they are just discarded) > > Performance notes: > * yes, it takes a microsecond longer for Kannel to reply to >sendsms. Moreover, if system is completyl bogged down, HTTP >queries start to build up, too. > * if SMS is multisent (several receivers), status and reply >is based on first ACK from bearerbox, rest are discarded > > This patch also includes one horrible kludge, which could be cleaned > away... (I also did not do documentation updates because this first > needs approval etc.) > > In addition, there is always the question how this should be used: > a) always on > b) used unless so configured (fast-reply = true ??) > c) used if so configured (waited-reply = true ??) > I didn't add config value for b/c, yet, as I strongly press that it > should be a) :] Those who are afraid of change should vote for b and c, > but if c is selected, at least smskannel.conf should be updated to > normally enable this... (I know that most Kannel installations use > smskannel.conf as base for configuration - well at least most I have > seen =) > > And why should this be used: > * better knowledge what did happen > * better responses to bad configuration or SMSC problems > * messages cannot be lost if store-file on! >(earlier they could be lost before bearerbox could store them) > * allows later update to include 'confirmed-send = true' type >of service, i.e. wait for SMSC ack/nack before replying > > PS: What happened to clean 'release all' code? I compiled with checking > alloc and there always seems to be some memory not freed in end. > Moreover, there seemed to be some leaks around... > > -- > &Kalle Marjola ::: Development ::: Helsinki ::: Enpocket > >
Re: [PATCH] preliminary confirmed sendsms
Kalle Marjola wrote: > On Thu, 2004-10-07 at 12:52, Alexander Malysh wrote: >> Hi Kalle, >> >> patch looks good except this "really horrible kludge"... >> > Well I thought that first check it out and if people like it > and we decide what to do about it (see choices a, b and c) > then I bother fixing that horrible kludge :] oops sorry... I'm +1 for (a)! -- Thanks, Alex
Re: [PATCH] preliminary confirmed sendsms
On Thu, 2004-10-07 at 12:52, Alexander Malysh wrote: > Hi Kalle, > > patch looks good except this "really horrible kludge"... > Well I thought that first check it out and if people like it and we decide what to do about it (see choices a, b and c) then I bother fixing that horrible kludge :] -- &Kalle Marjola ::: Development ::: Helsinki ::: Enpocket
Re: [PATCH] preliminary confirmed sendsms
Hi Kalle, patch looks good except this "really horrible kludge"... Kalle Marjola wrote: > And here is the patch itself =] > -- Thanks, Alex
Re: [PATCH] preliminary confirmed sendsms
And here is the patch itself =]
--
&Kalle Marjola ::: Development ::: Helsinki ::: Enpocket
? gwlib/gw_uuid_types.h
? test/test_boxc
? test/test_mime_multipart
? test/test_pcre
? test/test_prioqueue
? test/test_regex
? test/test_uuid
Index: gw/smsbox.c
===
RCS file: /home/cvs/gateway/gw/smsbox.c,v
retrieving revision 1.252
diff -u -r1.252 smsbox.c
--- gw/smsbox.c 3 Sep 2004 12:42:33 - 1.252
+++ gw/smsbox.c 6 Oct 2004 12:34:55 -
@@ -130,6 +130,15 @@
int charset_processing (Octstr *charset, Octstr *text, int coding);
static long get_tag(Octstr *body, Octstr *tag, Octstr **value, long pos, int nostrip);
+/* for delayed HTTP answers.
+ * Dict key is uuid, value is HTTPClient pointer
+ * of open transaction
+ */
+
+static Dict *client_dict = NULL;
+static Octstr *stored_uuid_os = NULL; // horrible kludge!
+static List *sendsms_reply_hdrs = NULL;
+
/***
* Communication with the bearerbox.
*/
@@ -150,6 +159,60 @@
write_to_bearerbox(msg);
}
+/*
+ * Handle delayed reply to HTTP sendsms client, if any
+ */
+static void delayed_http_reply(Msg *msg)
+{
+HTTPClient *client;
+Octstr *os, *answer;
+char id[UUID_STR_LEN + 1];
+int status;
+
+uuid_unparse(msg->ack.id, id);
+os = octstr_create(id);
+debug("sms.http", 0, "Got ACK (%d) of %s", msg->ack.nack, octstr_get_cstr(os));
+client = dict_remove(client_dict, os);
+if (client == NULL) {
+warning(0, "No client? (multi-send?)");
+octstr_destroy(os);
+return;
+}
+/* XXX this should be fixed so that we really wait for DLR
+ * SMSC accept/deny before doing this - but that is far
+ * more slower, a bit more complex, and is done later on
+ */
+
+switch (msg->ack.nack) {
+ case ack_success:
+status = HTTP_ACCEPTED;
+answer = octstr_create("0: Accepted for delivery");
+break;
+ case ack_buffered:
+status = HTTP_ACCEPTED;
+answer = octstr_create("3: Queued for later delivery");
+break;
+ case ack_failed:
+status = HTTP_FORBIDDEN;
+answer = octstr_create("Not routable. Do not try again.");
+break;
+ case ack_failed_tmp:
+status = HTTP_SERVICE_UNAVAILABLE;
+answer = octstr_create("Temporal failure, try again later.");
+break;
+ default:
+ error(0, "Strange reply from bearerbox!");
+status = HTTP_SERVICE_UNAVAILABLE;
+answer = octstr_create("Temporal failure, try again later.");
+break;
+}
+
+http_send_reply(client, status, sendsms_reply_hdrs, answer);
+
+octstr_destroy(answer);
+octstr_destroy(os);
+}
+
/*
* Read an Msg from the bearerbox and send it to the proper receiver
@@ -189,11 +252,9 @@
total++;
list_produce(smsbox_requests, msg);
} else if (msg_type(msg) == ack) {
- /*
-* do nothing for now. Later we will handle this
-* gracefully...
-*/
- msg_destroy(msg);
+
+ delayed_http_reply(msg);
+ msg_destroy(msg);
} else {
warning(0, "Received other message than sms/admin, ignoring!");
msg_destroy(msg);
@@ -2231,6 +2292,19 @@
udh == NULL ? ( text == NULL ? "" : octstr_get_cstr(text) ) : "<<
UDH >>");
}
}
+/* XXX UGLY kludge, to be used in sendsms_thread */
+if (stored_uuid_os == NULL) {
+char id[UUID_STR_LEN + 1];
+
+uuid_unparse(msg->sms.id, id);
+stored_uuid_os = octstr_create(id);
+
+debug("sms.http", 0, "Stored UUID %s", octstr_get_cstr(stored_uuid_os));
+
+/* this octstr is then used to store the HTTP client into
+* client_dict, if need to, in sendsms_thread */
+}
+
msg_destroy(msg);
list_destroy(receiver, octstr_destroy_item);
list_destroy(allowed, octstr_destroy_item);
@@ -3018,17 +3092,12 @@
static void sendsms_thread(void *arg)
-{
+ {
HTTPClient *client;
Octstr *ip, *url, *body, *answer;
-List *hdrs, *args, *reply_hdrs;
+List *hdrs, *args;
int status;
-reply_hdrs = http_create_empty_headers();
-http_header_add(reply_hdrs, "Content-type", "text/html");
-http_header_add(reply_hdrs, "Pragma", "no-cache");
-http_header_add(reply_hdrs, "Cache-Control", "no-cache");
-
for (;;) {
client = http_accept_request(sendsms_port, &ip, &url, &hdrs, &body,
&args);
@@ -3084,18 +3153,23 @@
debug("sms.http", 0, "Status: %d Answer: <%s>", status,
octstr_get_cstr(answer));
-octstr_destroy(ip);
-octstr_destroy(url);
-http_destroy_headers(hdrs);
-octstr_destroy(body);
-http_destroy_cgiargs(args);
-
-http_send_reply(client, status, reply_hdrs, answer);
+ octst
[PATCH] preliminary confirmed sendsms
This patch is the first version of sendsms confirmations. This patch actually does what smsbox should have done like 4 years ago but I was too lazy to implement it back then because it is a bit complicated in logic. Later on I did that to NMGW, but as NMGW is single-process, it was simpler in that. But now it is, at last, here for original Kannel. So, what does this first version do: Instead of immediately saying '202 Send' for valid sendsms request, this patch waits for bearerbox ACK before replying to HTTP client. Valid replies are based on msg.h and are now: 202 0: Accepted for delivery (routed to SMSC driver) 202 3: Queued for later delivery (all SMSCes currently down) 403 Forbidden (no SMSC comfigured to accept this, fix something) 503 Service unavailable (queue full or other problem. Try again later) This is still far from ideal (to get SMSC reply), but better than earlier. And this is how it should have been, always, as why else these is those 'ack' type messages flying around... (currently they are just discarded) Performance notes: * yes, it takes a microsecond longer for Kannel to reply to sendsms. Moreover, if system is completyl bogged down, HTTP queries start to build up, too. * if SMS is multisent (several receivers), status and reply is based on first ACK from bearerbox, rest are discarded This patch also includes one horrible kludge, which could be cleaned away... (I also did not do documentation updates because this first needs approval etc.) In addition, there is always the question how this should be used: a) always on b) used unless so configured (fast-reply = true ??) c) used if so configured (waited-reply = true ??) I didn't add config value for b/c, yet, as I strongly press that it should be a) :] Those who are afraid of change should vote for b and c, but if c is selected, at least smskannel.conf should be updated to normally enable this... (I know that most Kannel installations use smskannel.conf as base for configuration - well at least most I have seen =) And why should this be used: * better knowledge what did happen * better responses to bad configuration or SMSC problems * messages cannot be lost if store-file on! (earlier they could be lost before bearerbox could store them) * allows later update to include 'confirmed-send = true' type of service, i.e. wait for SMSC ack/nack before replying PS: What happened to clean 'release all' code? I compiled with checking alloc and there always seems to be some memory not freed in end. Moreover, there seemed to be some leaks around... -- &Kalle Marjola ::: Development ::: Helsinki ::: Enpocket
