Re: timeout handling and server msg callback

2014-10-30 19:47 GMT+00:00 Tolga Ceylan <[email protected]>:

> It doesn't look wrapped unless I'm looking at the wrong files/code.
>
> READSOCKET is defined as recv() or read() depending on arch.
>
> It's called from tds_goodread() in tds/net.c.
>
>
I think you are referring to 0.91.

Yes, read/recv is in a for which exit when you read all bytes expected,
unless unfinished is true.

Frediano


>
> On Thu, Oct 30, 2014 at 12:39 PM, Frediano Ziglio <[email protected]>
> wrote:
> > 2014-10-30 19:23 GMT+00:00 Tolga Ceylan <[email protected]>:
> >
> >> In other words, (except maybe in encrypted case), a TCP read can
> >> return 1 byte. The underlying
> >> call is recv(). There's no guarantee that arrivals will always be >= 8
> >> bytes or whole packets. It's TCP which
> >> is a stream of data where application (freetds) is responsible with
> >> assembling application level
> >> data. These tiny reads could happen if the client slows down and
> >> adjusts its receive window size.
> >>
> >>
> > I know... but we don't just call recv, it's wrapped to handle such cases.
> >
> >
> >> For the 'tds packet timeout', yes, I was basically suggesting to move
> >> the timeout logic to higher level from
> >> tds_goodread() to tds_read_packet().  Full packet timeout basically...
> >>
> >>
> > Understood. Still the point remains, you are catching corner cases. We
> are
> > trying to detect if server is not sending anything. I understood that in
> > some situations packets could get a lot fragmented and every fragment can
> > have different reasons to be delayed so the total timeout can be quite
> big
> > but we are talking about packets that are mostly 4kb transmitted in
> network
> > packets which are more than 1kb.
> >
> > Good code anyway is always welcome. Remember to use the master branch.
> >
> > Frediano
> >
> >
> >
> >> On Thu, Oct 30, 2014 at 12:01 PM, Frediano Ziglio <[email protected]>
> >> wrote:
> >> > 2014-10-30 17:56 GMT+00:00 Tolga Ceylan <[email protected]>:
> >> >
> >> >> I mostly agree with this. I think app level timeouts is easy to add
> >> >> since callbacks are firing
> >> >> and the app code can keep track of the overall time there.
> >> >>
> >> >> However, 'query_timeout' is misleading. This really is a
> >> >> 'socket_activity_timeout'
> >> >> since it's based on arriving bits of data from the socket.
> >> >>
> >> >> Let say 'query_timeout' is 300 secs. If the TDS packet can be
> >> >> assembled after 5 calls to socket
> >> >> read, then this could get blown up to 5*300 secs. This means even if
> >> >> we rename 'query_timeout', it's not
> >> >> really meaningful.
> >> >>
> >> >>
> >> > yes, probably query_timeout is quite misleading.
> >> >
> >> >
> >> >> I also see that the TDS header (8 bytes) is expected to arrive in one
> >> >> read. If the header
> >> >> arrives in more than one read, then the connection gets closed. This
> >> >> is a bug. TCP code
> >> >> needs to handle tiny read cases even if this is rare.
> >> >>
> >> >>
> >> > I don't think so. The only case tds_goodread returns < 8 is when user
> >> > requested to close connection or on a socket error. Never for less
> bytes
> >> > readed. And if crypted all packets are crypted so is impossible it
> does
> >> not
> >> > return at least 8 bytes.
> >> >
> >> >
> >> >> I think 'query_timeout' can be renamed to 'tds packet timeout.'
> >> >> Instead of tds_select(), the higher
> >> >> level goodread() function can keep track of this timer and decide if
> >> >> there is a packet timeout or not.
> >> >>
> >> >> I can put together a patch if you like.
> >> >>
> >> >> Cheers,
> >> >> Tolga
> >> >>
> >> >>
> >> > Would be useful. I didn't actually understand what you want to achieve
> >> > however. A kind of full packet timeout? Server tends to send packet
> >> > together so you would catch some network delay or problem. If you
> remove
> >> > the timeout from tds_select how can it return if server does not send
> >> data?
> >> >
> >> > Frediano
> >> >
> >> >
> >> >> On Thu, Oct 30, 2014 at 3:02 AM, Frediano Ziglio <[email protected]
> >
> >> >> wrote:
> >> >> > 2014-10-30 4:10 GMT+00:00 Tolga Ceylan <[email protected]>:
> >> >> >
> >> >> >> Hi All,
> >> >> >>
> >> >> >> Recently, I've stumbled on a potential issue with the timeout
> >> handling
> >> >> >> in ct_lib.
> >> >> >>
> >> >> >> Given these two cases of SQL executing;
> >> >> >>
> >> >> >> DECLARE @counter int
> >> >> >> WHILE (@counter < 99999999)
> >> >> >> BEGIN
> >> >> >>     PRINT @counter
> >> >> >>     SET @counter = @counter + 1
> >> >> >> END
> >> >> >>
> >> >> >> versus;
> >> >> >>
> >> >> >> DECLARE @counter int
> >> >> >> WHILE (@counter < 99999999)
> >> >> >> BEGIN
> >> >> >>     SET @counter = @counter + 1
> >> >> >> END
> >> >> >>
> >> >> >> (only difference is print or no-print basically)
> >> >> >>
> >> >> >> seems to affect timeout handling since this trips the ::select()
> (aka
> >> >> >> tds_select) based query timeout.
> >> >> >>
> >> >> >> In other words, continuously feeding such server messages back to
> the
> >> >> >> client bypasses
> >> >> >> the query timeout. The issue is tricky because there has to be
> enough
> >> >> >> 'print' executions
> >> >> >> to actually trigger this. (Probably due to Nagle or other
> buffering.)
> >> >> >>
> >> >> >> Has anybody seen this before? Any work around for it(*) ?
> >> >> >>
> >> >> >> (*) the fix probably is to consider beginning/end of the actual
> >> >> >> queries as opposed to blocking on select
> >> >> >> with query timeout.
> >> >> >>
> >> >> >> I'm using 0.82 on linux centos 6.5 although I couldn't see changes
> >> >> >> related to this in 0.91.
> >> >> >>
> >> >> >> Cheers,
> >> >> >> Tolga
> >> >> >>
> >> >> >
> >> >> >
> >> >> > Hi,
> >> >> >   It's not a bug, it's a feature :)
> >> >> >
> >> >> > Really, this is the intention. Also considering that you should
> handle
> >> >> > results (print IS a result) the library should return so your
> timeout
> >> >> > should be managed by application.
> >> >> >
> >> >> > Frediano
> >> >> > _______________________________________________
> >> >> > FreeTDS mailing list
> >> >> > [email protected]
> >> >> > http://lists.ibiblio.org/mailman/listinfo/freetds
> >> >> _______________________________________________
> >> >> FreeTDS mailing list
> >> >> [email protected]
> >> >> http://lists.ibiblio.org/mailman/listinfo/freetds
> >> >>
> >> > _______________________________________________
> >> > FreeTDS mailing list
> >> > [email protected]
> >> > http://lists.ibiblio.org/mailman/listinfo/freetds
> >> _______________________________________________
> >> FreeTDS mailing list
> >> [email protected]
> >> http://lists.ibiblio.org/mailman/listinfo/freetds
> >>
> > _______________________________________________
> > FreeTDS mailing list
> > [email protected]
> > http://lists.ibiblio.org/mailman/listinfo/freetds
> _______________________________________________
> FreeTDS mailing list
> [email protected]
> http://lists.ibiblio.org/mailman/listinfo/freetds
>