FreeTDS Development Group

["ZIGLIO, Frediano, VF-IT" <Frediano.Ziglio AT vodafone.com>]

> 
> All, 
> 
> As previously announced, I'm working on new timeout logic.  Tonight I
> committed some of it, but I want everyone to know it's not 
> done yet.  Like
> most such things, it will get worse before it gets better.  
> 
> Freddy, I would especially like your opinion on a new function:
> net.c::tds_select().  
> 

Well, I forget... why tdserror and not tds_error?? Why TDSECLOS and not
TDS_ECLOS (or TDS_ERR_CLOSE) ?
(yes I'm paranoid but I think it's more consistent)

Looking at diff it appear that there are many place where we catch
tdserror result and handle it. Also in many places the only options are
exiting and close socket (think at connection error or read socket
error) but on timeout/continue request we call exit. I would prefer a
connection close instead of a exit program default. Personally I think
that only dblib can return a TDS_INT_EXIT to I think it would be easier
to call exit directly from dblib (writing exit(EXIT_FAILURE) is as fast
as writing return TDS_INT_EXIT).
On tds_check_socket_write now the loop is not correct. As I know only
Linux (perhaps *BSD) update selecttimeout so is not correct to avoid
timeout computation again.
sleep?? why sleep??

> It's not called by anything yet, so it can't do any harm.  
> The idea is to
> localize all the timeout logic in one place.  (I started to add it
> wherever we could wait for the network.  That was too messy, 
> as you can
> still see, because I haven't backed it out yet.)  
> 
> The idea is libtds needs to be able to call back to the 
> client library for
> two purposes:
> 
> 1.  While waiting for a read from the server (cf. dbsetinterrupt).
> 2.  When the user-defined timeout expires (cf. dberrhandle)
> 
> The callback routines determine what libtds should do.  Docs say
> dbsetinterrupt applies only to reads.  Not sure why writes 
> wouldn't apply,
> too.  
> 

About tds_select
- do we really need another handler, this would be third timeout handler
as we have?
 - tdserror(TDSETIME)
 - int_handle
 - query_timeout_func
 or perhaps query_timeout_func should be removed? I though you would
remove query_timeout_func using just tdserror
- minor: end_ms should be unsigned
- minor: perhaps these lines
    case TDS_INT_TIMEOUT:           /* send a cancel packet */
      return 0;
    case TDS_INT_CANCEL:            /* close socket and abort function
*/
      return -1;
  should be
    case TDS_INT_CANCEL:           /* send a cancel packet */
      return 0;
    case TDS_INT_TIMEOUT:            /* close socket and abort function
*/
      return -1;
- minor: perror is terrible :)
- minor: in "error (cf. errno)" errno should be sock_errno
- tds_select is quite long... but perhaps is shorter than the functions
it replaces...
- have you considered moving close/send cancel/exit to err_handler?

> The error handler can return INT_TIMEOUT, meaning "send a 
> cancel packet to
> the server and wait for it to be acknowledged."  Of course, the cancel
> might never be acknowledged, and that read might timeout.  
> This leads to a
> weird kind of recursion:
> 
> 1.  dbnextrow()
> 2.  timeout, call tdserror()
> 3.  tdserror() returns TDS_INT_TIMEOUT
> 4.  libtds sends cancel packet
> 5.  libtds waits for cancel to be acknowledged
> 6.  timeout, call tdserror()
> 7.  goto #3.  
> 

I see no loop... wait for cancel it's just a continue. dbnextrow calls
tds_process_tokens which timeout, calls tdserror return than
tds_process_tokens handle (this is automatic!) cancellation (that is
waits for cancel) timeout again but it's the same tds_process_tokens
which dbnextrow calls so I don't see the recursion.

> I thought at first we should try to prevent this.  But on reflection I
> don't think so.  I'm not sure how the server would respond to 
> receiving
> two cancel packets.  But libtds can't know if the first (or 
> Nth) cancel
> packet was received if it wasn't acknowledged, so it's 
> actually reasonable
> to let errhandle() request we send as many as it likes. 
> 

from query.c

int
tds_send_cancel(TDSSOCKET * tds)
{
        CHECK_TDS_EXTRA(tds);

        /* one cancel it's sufficient */
        if (tds->in_cancel || tds->state == TDS_IDLE)
                return TDS_SUCCEED;

so 2 cancel sent are not possible

> If you're interested in this kind of programming, btw, I recommend
> Steven's Unix Network Programming.  
> 

freddy77