Skip to Content.
Sympa Menu

freetds - RE: [freetds] goodread... not so fine

freetds AT lists.ibiblio.org

Subject: FreeTDS Development Group

List archive

Chronological Thread  
  • From: "ZIGLIO, Frediano, VF-IT" <Frediano.Ziglio AT vodafone.com>
  • To: "FreeTDS Development Group" <freetds AT lists.ibiblio.org>
  • Subject: RE: [freetds] goodread... not so fine
  • Date: Mon, 19 Jan 2004 11:47:10 +0100

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




Archive powered by MHonArc 2.6.24.

Top of Page