FreeTDS Development Group

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

> 
> Hi,
> 
> We use the FreeTDS C library on a Solaris 10 system to send a 
> call to a 
> database procedure on a SQL Server database on a Windoze box. 
>  We were 
> using the 0.63 stable release, but were getting an occasional 
> (~ once per 2 
> months) timeout issue whereby the dbsqlexec() function would never 
> return.  With the recent improvements to timeout we changed to the 
> development release 0.65 from 9th January 2007.
> 
> All of our testing worked fine, but when we went live with 
> it, we got the 
> dbsqlexec() function not returning about once a day.  Turning 
> on debugging 
> showed the problem to be with tds_goodwrite().
> 
> I looked at the code, and saw what I thought could be several 
> problems.  So 
> I made a few changes, and added some extra debugging, and 
> installed the new 
> version.  We have not had a problem since (10 days now).  
> Unfortunately, I 
> am unable to ascertain which of the changes I made resolved 
> the actual 
> problem that we had, because the problem only occurs 
> intermittently on the 
> live system, and not on the test system.  So I thought I 
> would detail the 
> changes that were made so that someone else may be able to 
> make use of them.
> 
> Firstly, the Solaris man page for select(3c) states that if a 
> select() call 
> times out, then the three fds parameters have their bits 
> cleared.  In the 
> FreeTDS tds_select() function (in net.c), the select() call 
> is in a loop 
> with a 1 second time out, so if the socket is not available 
> for writing 
> within the first second, then it will pointlessly wait the 
> remaining period 
> of the configuration timeout, and then timeout.  I fixed this 
> by always 
> setting poll_seconds to timeout_seconds (line 334), and 

yes, but doing so dbinterrupt cannot work

> removed the end_ms 
> and diff_time code (lines 371 to 380), just leaving "if (ptv) break;".
> 

quite complicated, yes... I noted some problems... if diff_time if 0
(which is quite probable) we do not exit but we set timeout to 0 which
can be considered infinite (this can be the cause of the infinite
loop!!). Also on EINTR we should compute again timeout. See my patch at
http://freetds.cvs.sourceforge.net/freetds/freetds/src/tds/net.c?r1=1.60
&r2=1.61&sortby=date, please consider a new snapshot (tomorrow one). I'd
be glad if you could test this.

> Secondly, at line 659 of net.c, the result of the send() (#ifdef 
> USE_MSGMORE) is stored in a variable called "nput", but then 
> a variable 
> called "len" is checked, and reset on the next two lines.  I 
> changed "len" 
> to "nput" on these two lines.
> 

this was really, really broken, applied.

> Other changes I made were :
> Line 681 of net.c, for TDS_INT_TIMEOUT, I replaced the 
> "continue" with a 
> "return 0".

no, the idea is, on TDS_INT_TIMEOUT send cancel and wait reply from
server. On next timeout library should say, "ok, we waited quite a lot
after cancel, now stop and close". We can't return safely without
loosing syncronization.

> Line 689 of net.c, just before "p += nput" I added an error 
> report and 
> "return 0" if "nput <= 0".
> 

Mmm... here nput <= 0 should be mathematically impossible...

> A diff of those changes is attached.
> 
> Hope this is of use.
> 
> Cheers,
>          Brent.
> 
> *** old/net.c   Tue Jan  9 05:18:41 2007 UTC
> --- new/net.c   Thu May 21 04:37:49 2007 UTC
> ***************
> *** 321,324 ****
> --- 321,326 ----
>    {
>      int rc, seconds;
> + // brent 21-May-07 - Always select for full timeout
> +   const unsigned int poll_seconds = timeout_seconds;
> 
>      assert(tds != NULL);
> ***************
> *** 332,341 ****
>       */
>      seconds = timeout_seconds;
> -   const unsigned int poll_seconds = (tds->tds_ctx && 
> tds->tds_ctx->int_handler)? 1 : timeout_seconds;
>      assert(seconds >= 0);
>      do {
>          struct timeval tv, *ptv = timeout_seconds? &tv : NULL;
> -       unsigned int end_ms = poll_seconds * 1000 + tds_gettime_ms();
> -
>          /*
>           * The poll loop. We exit on the first of these events:
> --- 334,340 ----
> ***************
> *** 369,382 ****
> 
>              if (ptv) {
> -               const int diff_time = (int) (end_ms - 
> tds_gettime_ms());
> -
> -               if (diff_time < 0)
>                      break;
> -
> -               tv.tv_sec  = diff_time / 1000;
> -               tv.tv_usec = (diff_time % 1000) * 1000;
> -
> -               if (tv.tv_sec > poll_seconds || tv.tv_usec > 1000000)
> -                   break;  /* shouldn't happen; start again */
>              }
>          }

Yes, it's ugly... and perhaps is useful only for EINTR !

> --- 368,372 ----
> ***************
> *** 657,662 ****
>                  nput = send(tds->s, p, remaining, last ? 
> MSG_NOSIGNAL : 
> MSG_NOSIGNAL|MSG_MORE);
>                  /* In case the kernel does not support 
> MSG_MORE, try again 
> without it */
> !               if (len < 0 && errno == EINVAL && !last)
> !                   len = send(tds->s, p, remaining, MSG_NOSIGNAL);
>    #elif !defined(MSG_NOSIGNAL)
>                  nput = WRITESOCKET(tds->s, p, remaining);
> --- 647,653 ----
>                  nput = send(tds->s, p, remaining, last ? 
> MSG_NOSIGNAL : 
> MSG_NOSIGNAL|MSG_MORE);
>                  /* In case the kernel does not support 
> MSG_MORE, try again 
> without it */
> ! // brent 21-May-07 - len should be nput on the next two lines
> !               if (nput < 0 && errno == EINVAL && !last)
> !                   nput = send(tds->s, p, remaining, MSG_NOSIGNAL);
>    #elif !defined(MSG_NOSIGNAL)
>                  nput = WRITESOCKET(tds->s, p, remaining);
> ***************
> *** 679,683 ****
>              case TDS_INT_TIMEOUT:
>                  tds_send_cancel(tds);
> !               continue; /* fixme: or return? */
>              default:
>              case TDS_INT_CANCEL:
> --- 670,675 ----
>              case TDS_INT_TIMEOUT:
>                  tds_send_cancel(tds);
> ! // brent 21-May-07 - Return fail seems more sensible then continue
> !               return 0;
>              default:
>              case TDS_INT_CANCEL:
> ***************
> *** 687,690 ****
> --- 679,689 ----
>              assert(0); /* not reached */
>          }
> + // brent 21-May-07 If nothing sent, then abort (avoid a 
> possible infinite 
> loop)
> +       if ( nput <= 0 )
> +       {
> +           tdsdump_log(TDS_DBG_ERROR, "Send sent %d bytes (of %d)\n",
> +                   nput, remaining);
> +           return 0;
> +       }
> 
>          p += nput;

freddy77