Re: [PATCH] Guaranteed throughput smsc-independent
Damian Viano <[email protected]>
| Newsgroups | gmane.comp.mobile.kannel.devel |
|---|---|
| Message-ID | <[email protected]> |
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.
#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...)
@@ -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)
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.
+ /* 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).