[
https://issues.apache.org/jira/browse/THRIFT-1642?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14537130#comment-14537130
]
James E. King, III commented on THRIFT-1642:
--------------------------------------------
My guess is this code path is rarely taken since the bug has been there for a
while. There are far too many modes of operation inside TSocket that
complicate the code tremendously. This loop is definitely one of them. I took
your signedness fix but I have left the sleep and the timeout calculation
alone, given this code is likely not tested in the current set of tests. To
test these code paths properly we would need a low level socket API abstraction
that we could use something like GMock on, so that a call to recv() could mock
return EAGAIN reliably; also gettimeofday would need a similar mock. These are
all quite possible to do with a little refactoring...
> Miscalculation lead to throw unexpected "TTransportException::TIMED_OUT"(or
> called "EAGAIN (timed out)") exception
> ------------------------------------------------------------------------------------------------------------------
>
> Key: THRIFT-1642
> URL: https://issues.apache.org/jira/browse/THRIFT-1642
> Project: Thrift
> Issue Type: Bug
> Components: C++ - Library
> Affects Versions: 0.8
> Environment: GNU/Linux x86_64
> Reporter: catxl
> Priority: Critical
> Attachments: TSocket.cpp.patch
>
> Original Estimate: 1h
> Remaining Estimate: 1h
>
> In function 'uint32_t TSocket::read(uint8_t* buf, uint32_t len)', there's an
> miscalculation which can lead to wrong judgment:
> struct timeval begin;
> gettimeofday(&begin, NULL);
> //
> // do some thing
> //
> struct timeval end;
> gettimeofday(&end, NULL);
> uint32_t readElapsedMicros = (((end.tv_sec - begin.tv_sec) * 1000 * 1000)
> + (((uint64_t)(end.tv_usec - begin.tv_usec))));
> 'readElapsedMicros' will be very large when 'end.tv_usec < begin.tv_usec'.
> This will lead to throw unexpected "TTransportException::TIMED_OUT"(or called
> "EAGAIN (timed out)") exception sometimes.
> Besides,
> (1)I don't think the "usleep(50);" call in the same function above is
> necessary, I think delete this call could improve performance.
> (2)I don't know why 'readElapsedMicros' compare with 'eagainThresholdMicros',
> which is not the value (recvTimeout_*1000), but:
> eagainThresholdMicros = (recvTimeout_*1000)/ ((maxRecvRetries_>0) ?
> maxRecvRetries_ : 2);
> can anyone tell me someting about this? Thank you!
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
