Re: [PATCH] UCP/EMI keepalive/UCP31 bug.

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 ?
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)



> 
>> @@ -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