FreeTDS Development Group

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

> 
> On Fri, 2004-01-16 at 15:28, ZIGLIO, Frediano, VF-IT wrote:
> > > These lines don't fix the bug, but workaround it - but at 
> the cost of
> > > creating other bugs... So, it should be corrected.
> 
> > I fully agree...
> > 
> > However I still do not understand the problem. From you mail:
> > 
> > "At the moment, when cancelling a query, a cancel packet is
> > being sent (that is, an empty packet with the headers 
> out_flag set to
> > 0x06 = attention packet). After this, all packets are read until an
> > acknowledgement of the cancel has been received.
> > 
> > Now, the wire does not contain more data - but as the 
> cancel has been
> > sent asynchroneously, a client can (and e.g. sqsh often 
> does) call one
> > of ct_results, ct_fetch and probably more. Thus, this all 
> comes down to
> > goodread(); freetds tries to read data from the wire where 
> there is no
> > data available. Due to the timeout handling code - this is 
> being done
> > over and over again."
> > 
> > However ctlib send cancel and then process it... from ct.c
> > 
> >     if (conn && !IS_TDSDEAD(conn->tds_socket)) {
> >             tds_send_cancel(conn->tds_socket);
> >             tds_process_cancel(conn->tds_socket);
> >     }
> > 
> > Perhaps cancel take very long so goodread exit before the 
> done/cancel ?
> 
> No, cancel is being processed quite fast and very relyable as you can
> see from the tdsdump.
> 
> > However with your patch behaviour would be worse...
> 
> Yes.
> 
> > If tds_process_cancel return success state is IDLE so
> > ct_fetch/ct_results should just return CS_FAIL...
> 
> I added the check to ct_results/ct_fetch, that improves the situation.
> However, it doesn't work reliable, though it worked in 80% of 
> my tests.
> 

This means you are able to reproduce it :) Good point. I never used sqsh, do 
you just issue a query that returns a lot of data and then press ctrl-c ??

> Two problems remain:
> a) sometimes I get an "unknown marker"-client message (followed by
> tds_close_socket)
> b) sometimes the old behaviour still occurs: it just hangs
> 
> This is a code snipplet from ct_results:
>               /* Check for idle state */
>               if (tds->state == TDS_IDLE)
>                       return CS_FAIL;
>                       

Perhaps test should be

if (tds->state != TDS_PENDING)
        return CS_FAIL;

>               tdsret = tds_process_result_tokens(tds, 
> &res_type, &done_flags);
> 
> When killing sqsh with SIGSEGV and looking at the coredump, I can see
> the following bt:
> #0  0x28194fe3 in select () from /usr/lib/libc.so.5
> (gdb) bt
> #0  0x28194fe3 in select () from /usr/lib/libc.so.5
> #1  0x280a42d7 in goodread (tds=0x8092000, buf=0x28089000 "z¸PÕ\001",
> buflen=-1077940316)
>     at read.c:115
> #2  0x280a49cd in tds_read_packet (tds=0x8092000) at read.c:501
> #3  0x280a45a7 in tds_get_byte (tds=0x8092000) at read.c:210

This is expected.

> #4  0x2809e371 in tds_process_result_tokens (tds=0x8092000,
> result_type=0xbfbfeff4, 
>     done_flags=0xbfbfeff8) at token.c:505

:( tds_process_result_tokens should not hang..
If you have still the core file you can try to print state

(gdb) print ((struct tds_socket*)0x8092000)->state

In token.c we have

        if (tds->state == TDS_IDLE) {
                tdsdump_log(TDS_DBG_FUNC, "%L tds_process_result_tokens() 
state is COMPLETED\n");
                *result_type = TDS_DONE_RESULT;
                return TDS_NO_MORE_RESULTS;
        }

and in read.c we have

        if (tds->in_pos >= tds->in_len) {
                do {
                        if (IS_TDSDEAD(tds) || (rc = tds_read_packet(tds)) < 
0)
                                return 0;
                } while (!rc);
        }

so if state == TDS_IDLE or state == TDS_DEAD should not fail... TDS_CANCELED 
is not used...

> #5  0x280969e8 in ct_results (cmd=0x807de00, 
> result_type=0xbfbff02c) at
> ct.c:908
> #6  0x08055f29 in dsp_horiz (output=0x80ca000, cmd=0x807de00, flags=0)
> at dsp_horiz.c:335
> [...]
> 
> (Please note, the line numbers may be different to your)
> 
> It comes down to goodread() which hangs - even I have checked the
> tds-state before. It seems the ct_cancel can be invoked between the
> non-idle check and the actual select.
> 
> Maybe goodread() needs to check the tds-state, too? 
> 

No, it's a bad design here. goodread is a low level function and should not 
use state (it's not expected to have knowledge of TDS state). You already 
added code to check TDS_IDLE however or state get set in 
tds_process_result_tokens or state is not TDS_IDLE (nor TDS_DEAD)... I don't 
think sqsh be a multi-thread application... or perhaps it use alarm() and 
issue cancel inside alarm signal ??

freddy77