[PATCH] UCP/EMI keepalive/UCP31 bug.

Andreas Fink afink at list.fink.org
Tue Aug 5 14:42:13 CEST 2008


On 05.08.2008, at 12:57, Vincent CHAVANIS wrote:

>
> Hi andreas,
>
>
> Andreas Fink a écrit :
>> On 05.08.2008, at 12:17, Vincent CHAVANIS wrote:
>>>                                                   \
>>> - (time(NULL) > (PRIVDATA(conn)->last_activity_time +  
>>> PRIVDATA(conn)->keepalive)))
>>> + (time(NULL) >= (PRIVDATA(conn)->last_activity_time +  
>>> PRIVDATA(conn)->keepalive)))
>> so this is keepalive always being +1... sounds good to me.
>
> So you aggree to that fact that keepalive will be keepalive + 1 ?

yes because of >= versus >.

>
> The impact is that each UCP31 will be done with 1 second inlate.  
> This is not what users want.
> If I specify keepalive each 5 sec, i don't want it each 6 sec.
>
>
>>>               } else if (emimsg->ot == 31) {
>>> +                    PRIVDATA(conn)->last_activity_time = time  
>>> (NULL);                         /* XXX Process error codes here
>>>                   if (octstr_get_char(emimsg->fields[0], 0) ==  
>>> 'N') {
>>>                       long errorcode;
>> not sure why this is needed. Would sound like it would have never  
>> worked otherwise...
>
> This is absolutly needed!
> Take this exemple
> 00:00:00 you sent a UCP31
> 00:00:30 you received an ack of your UCP31
> (the SMSC is overloaded in this case)
> Then if your keepalive is set to 31 you will send just after 1 sec  
> an other UCP31. And will result to block the sender thread  
> (can_write = 0)

This should never happen if you have window properly set. And while we  
have traffic, last activity is counting up so keepalive will not be  
sent out anyway as the activity keeps the link alive. Keepalive is  
ONLY sent if there is NO activity. But counting keepalive as activity  
itself doesnt sound wrong neither.




>>> @@ -1269,7 +1317,7 @@
>>> */
>>> static double emi2_get_timeouttime (SMSCConn *conn, Connection  
>>> *server)
>>> {
>>> -    double ka_timeouttime = PRIVDATA(conn)->keepalive ?  
>>> PRIVDATA(conn)->keepalive + 1 : DBL_MAX;
>>> +    double ka_timeouttime = PRIVDATA(conn)->keepalive ?  
>>> PRIVDATA(conn)->keepalive : DBL_MAX;
>> shouldn't timeout be longer than keepalive? could above change not  
>> create a race condition to the fact that the keepalive was "just  
>> about to be sent" but timeout hit first?
>>>
>>>    double idle_timeouttime = (PRIVDATA(conn)->idle_timeout &&  
>>> server) ? PRIVDATA(conn)->idle_timeout : DBL_MAX;
>>>    double result = ka_timeouttime < idle_timeouttime ?  
>>> ka_timeouttime : idle_timeouttime;
>>> @@ -1626,7 +1674,7 @@
>>>    privdata->listening_socket = -1;
>>>    privdata->can_write = 1;
>>>    privdata->priv_nexttrn = 0;
>>> -    privdata->last_activity_time = 0;
>>> +    privdata->last_activity_time = time (NULL); /* to *NOT* force  
>>> keepalive after login */
>> as I said I would leave keepalive at login.
>
> This is not so trivial.
>
> Vincent




More information about the devel mailing list