Re: timeout handling and server msg callback

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.


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