Client: go
Starting from go 1.15, go test starts to complain about wrongly used int
to string conversions:
./field.go:58:83: conversion from int to string yields a string of one rune, not a string of digits (did you mean fmt.Sprint(x)?)
./numeric.go:72:12: conversion from int64 to string yields a string of one rune, not a string of digits (did you mean fmt.Sprint(x)?)
./json_protocol_test.go:612:92: conversion from int to string yields a string of one rune, not a string of digits (did you mean fmt.Sprint(x)?)
./simple_json_protocol_test.go:685:96: conversion from int to string yields a string of one rune, not a string of digits (did you mean fmt.Sprint(x)?)
json_protocol_test and simple_json_protocol_test usages are actually
already in format arg so just remove the conversion is good enough.
field is no longer used anywhere so just removed it (there's one line of
commented out code in compact_protocol so remove that line as well). The
one in numeric.go is actually a bug. We didn't set sValue correctly in
NewNumericFromI64 and NewNumericFromI32 functions.
Client: py
This is inspired by changes to the Go library (THRIFT-5214) and, by
proxy, this blog post[1]. The idea is that if the other end of the
socket has closed their end of the connection, we can figure that out by
doing a non-blocking read on our socket before we waste time serializing
and sending a message just to find out the socket is closed when we try
to read the response.
[1]: https://github.blog/2020-05-20-three-bugs-in-the-go-mysql-driver/
Client: go
In previous implementation of socket connectivity check, we try to read
1 byte and put it into buffer when succeeded. The buffer complicates
the implementation of Read function. Change to use syscall.Recvfrom with
MSG_PEEK flag instead so that the buffer is no longer needed.
Client: go
In the current implementation, we only call endOfFrame when we hit EOF
when reading from the frameReader. The problem is in go stdlib the Read
call finished reading the remaining data from frameReader will not
return EOF, the next Read will. This caused us in most cases only call
endOfFrame at the beginning of the next frame, which could cause
troubles because we didn't read the beginning of the frame properly.
Client: go
In compiler generated TProcessorFunction implementations, add a
goroutine after read the request to do connectivity check on the input
transport. If the transport is no longer open, cancel the context object
passed into the handler implementation.
Also define ErrAbandonRequest error, to help TSimpleServer closing
client connections that's already closed on the other end.
Client: go
We separated timeout in go's TSocket into connect timeout and socket
timeout in 81334cd, this change does the same for TSSLSocket to keep
them consistent.
Also rename the arg in NewTSocketFromConnTimeout from connTimeout to
socketTimeout, because in that function we already have a connection,
so connect timeout is never used again. The timeout passed into that
function is really for socket timeout, not connect timeout. The name of
that function actually means "New TSocket From Conn (with) Timeout", not
"New TSocket From ConnTimeout" (I guess that's where the original
confusion came from).
Also add the missing change note for the breaking change.
Client: go
As discussed in the JIRA ticket, this commit changes how we handle I/O
timeouts in the go library.
This is a breaking change that adds context to all Read*, Write*, and
Skip functions to TProtocol, along with the compiler change to support
that, and also adds context to TStandardClient.Recv, TDeserializer,
TStruct, and a few others.
Along with the function signature changes, this commit also implements
context cancellation check in the following TProtocol's ReadMessageBegin
implementations:
- TBinaryProtocol
- TCompactProtocol
- THeaderProtocol
In those ReadMessageBegin implementations, if the passed in context
object has a deadline attached, it will keep retrying the I/O timeout
errors, until the deadline on the context object passed. They won't
retry I/O timeout errors if the passed in context does not have a
deadline attached (still return on the first error).
Client: go
As discussed in the JIRA ticket, this commit changes how we handle I/O
timeouts in the go library.
This is a breaking change that adds context to all Read*, Write*, and
Skip functions to TProtocol, along with the compiler change to support
that, and also adds context to TStandardClient.Recv, TDeserializer,
TStruct, and a few others.
Along with the function signature changes, this commit also implements
context cancellation check in the following TProtocol's ReadMessageBegin
implementations:
- TBinaryProtocol
- TCompactProtocol
- THeaderProtocol
In those ReadMessageBegin implementations, if the passed in context
object has a deadline attached, it will keep retrying the I/O timeout
errors, until the deadline on the context object passed. They won't
retry I/O timeout errors if the passed in context does not have a
deadline attached (still return on the first error).
Client: go
This is a slightly different, and less error-prone approach from the
fix in e382275b.
The previous approach relies on passing the set socket timeout into the
underlying socketConn from TSocket and TSSLSocket. But since we have so
many different constructors for TSocket and TSSLSocket, some makes the
initial connection in the constructor and some does not, there are so
many different places we would need to remember to pass socketTimeout
into socketConn. In the future, when we add another constructor to them,
we could either forget to pass the socket timeout into socketConn, or
try to pass it while we haven't constructed socketConn yet (which will
cause panic), both are bad.
In this approach we just clear the read deadline in the connectivity
check read. Because that's a non-blocking read, it would work just fine
without a read deadline.
Client: go
We added socketConn to go library for connectivity check in
https://github.com/apache/thrift/pull/2153, but forgot to push read
deadline on the socket when doing the connectivity checks. This caused
the issue of large number of connectivity checks to fail with I/O
timeout errors.
Client: go
While debugging the issue in THRIFT-5214, I original thought that was a
bug in TFramedTransport implementation, so I took some time scrutinize
the TFramedTransport code. Although in the end there's no bug, the
current implementation of TFramedTransport, especially in the Read
function, has some weird handling while at frame boundary, which is both
error-prone and hard to follow (I did found and fixed one bug there in
https://github.com/apache/thrift/pull/1810 before).
The new implementation reads the whole frame into a buffer, which would
use slightly more memory, but it follows the pattern of TFramedTransport
implementation of other languages (e.g. Java), and also the pattern we
handle frame in THeaderTransport. It also reduced the complexity and
weirdness of the frame boundary handling in Read implementation.
This rewrite also removes the print call from library code.
Client: go
In THeaderTransport implementation, use io.CopyN instead of
io.Copy+io.LimitReader.
Underlying io.CopyN is actually implemented with io.Copy+io.LimitReader
[0], but it also does some extra checks we can take advantage of. It
also simplifies the code in thrift repo.
[0]: 83b181c68b/src/io/io.go (L340)
