From e27e82c46ba4d46c2c3267701191cdc26614f739 Mon Sep 17 00:00:00 2001 From: Yuxuan 'fishy' Wang Date: Tue, 19 Jan 2021 11:07:58 -0800 Subject: [PATCH] THRIFT-5338: Raise minimal supported go version to 1.14.14 Client: go - Update Dockerfiles used by travis - Add go.mod file - Modify error handling code to take advantage of errors package updates in go1.13 --- LANGUAGES.md | 2 +- build/docker/README.md | 2 +- build/docker/ubuntu-bionic/Dockerfile | 4 +-- build/docker/ubuntu-disco/Dockerfile | 4 +-- build/docker/ubuntu-xenial/Dockerfile | 4 +-- lib/go/test/tests/client_error_test.go | 17 +++++++++++++ lib/go/thrift/exception.go | 6 +++-- lib/go/thrift/go.mod | 3 +++ lib/go/thrift/header_protocol.go | 5 ++-- lib/go/thrift/header_transport.go | 2 +- lib/go/thrift/http_client.go | 3 ++- lib/go/thrift/protocol_exception.go | 3 ++- lib/go/thrift/rich_transport.go | 7 ++++-- lib/go/thrift/simple_server.go | 30 ++++++----------------- lib/go/thrift/socket_unix_conn.go | 3 ++- lib/go/thrift/transport_exception.go | 25 ++++++++++--------- lib/go/thrift/transport_exception_test.go | 16 +++--------- 17 files changed, 71 insertions(+), 65 deletions(-) create mode 100644 lib/go/thrift/go.mod diff --git a/LANGUAGES.md b/LANGUAGES.md index 4ea86f78d68..08c431d38d9 100644 --- a/LANGUAGES.md +++ b/LANGUAGES.md @@ -160,7 +160,7 @@ Thrift's core protocol is TBinary, supported by all languages except for JavaScr Go 0.7.0 Yes -1.10.81.13.1 +1.14.141.15.7 YesYesYes YesYesYes YesYesYesYes diff --git a/build/docker/README.md b/build/docker/README.md index e3c2ec16971..08023a7b267 100644 --- a/build/docker/README.md +++ b/build/docker/README.md @@ -178,7 +178,7 @@ Last updated: October 1, 2017 | dart | 2.0.0 | 2.4.0 | | | delphi | | | Not in CI | | erlang | 18.3 | 22.0 | | -| go | 1.10.8 | 1.12.6 | | +| go | 1.14.14 | 1.15.7 | | | haskell | 7.10.3 | 8.0.2 | | | haxe | 3.2.1 | 3.4.4 | THRIFT-4352: avoid 3.4.2 | | java | 1.8.0_191 | 11.0.3 | | diff --git a/build/docker/ubuntu-bionic/Dockerfile b/build/docker/ubuntu-bionic/Dockerfile index 7deefcb8f31..c8ecd8ea9d3 100644 --- a/build/docker/ubuntu-bionic/Dockerfile +++ b/build/docker/ubuntu-bionic/Dockerfile @@ -140,9 +140,9 @@ RUN apt-get install -y --no-install-recommends \ libglib2.0-dev # golang -ENV GOLANG_VERSION 1.13.1 +ENV GOLANG_VERSION 1.15.7 ENV GOLANG_DOWNLOAD_URL https://golang.org/dl/go$GOLANG_VERSION.linux-amd64.tar.gz -ENV GOLANG_DOWNLOAD_SHA256 94f874037b82ea5353f4061e543681a0e79657f787437974214629af8407d124 +ENV GOLANG_DOWNLOAD_SHA256 0d142143794721bb63ce6c8a6180c4062bcf8ef4715e7d6d6609f3a8282629b3 RUN curl -fsSL "$GOLANG_DOWNLOAD_URL" -o golang.tar.gz && \ echo "$GOLANG_DOWNLOAD_SHA256 golang.tar.gz" | sha256sum -c - && \ tar -C /usr/local -xzf golang.tar.gz && \ diff --git a/build/docker/ubuntu-disco/Dockerfile b/build/docker/ubuntu-disco/Dockerfile index cfa40413ede..531718c2a90 100644 --- a/build/docker/ubuntu-disco/Dockerfile +++ b/build/docker/ubuntu-disco/Dockerfile @@ -140,9 +140,9 @@ RUN apt-get install -y --no-install-recommends \ libglib2.0-dev # golang -ENV GOLANG_VERSION 1.12.6 +ENV GOLANG_VERSION 1.15.7 ENV GOLANG_DOWNLOAD_URL https://golang.org/dl/go$GOLANG_VERSION.linux-amd64.tar.gz -ENV GOLANG_DOWNLOAD_SHA256 dbcf71a3c1ea53b8d54ef1b48c85a39a6c9a935d01fc8291ff2b92028e59913c +ENV GOLANG_DOWNLOAD_SHA256 0d142143794721bb63ce6c8a6180c4062bcf8ef4715e7d6d6609f3a8282629b3 RUN curl -fsSL "$GOLANG_DOWNLOAD_URL" -o golang.tar.gz && \ echo "$GOLANG_DOWNLOAD_SHA256 golang.tar.gz" | sha256sum -c - && \ tar -C /usr/local -xzf golang.tar.gz && \ diff --git a/build/docker/ubuntu-xenial/Dockerfile b/build/docker/ubuntu-xenial/Dockerfile index 535db419f4e..e554c53179c 100644 --- a/build/docker/ubuntu-xenial/Dockerfile +++ b/build/docker/ubuntu-xenial/Dockerfile @@ -137,9 +137,9 @@ RUN apt-get install -y --no-install-recommends \ libglib2.0-dev # golang -ENV GOLANG_VERSION 1.10.8 +ENV GOLANG_VERSION 1.14.14 ENV GOLANG_DOWNLOAD_URL https://golang.org/dl/go$GOLANG_VERSION.linux-amd64.tar.gz -ENV GOLANG_DOWNLOAD_SHA256 d8626fb6f9a3ab397d88c483b576be41fa81eefcec2fd18562c87626dbb3c39e +ENV GOLANG_DOWNLOAD_SHA256 6f1354c9040d65d1622b451f43c324c1e5197aa9242d00c5a117d0e2625f3e0d RUN curl -fsSL "$GOLANG_DOWNLOAD_URL" -o golang.tar.gz && \ echo "$GOLANG_DOWNLOAD_SHA256 golang.tar.gz" | sha256sum -c - && \ tar -C /usr/local -xzf golang.tar.gz && \ diff --git a/lib/go/test/tests/client_error_test.go b/lib/go/test/tests/client_error_test.go index a064163651f..8d720fff272 100644 --- a/lib/go/test/tests/client_error_test.go +++ b/lib/go/test/tests/client_error_test.go @@ -32,6 +32,23 @@ import ( // TestCase: Comprehensive call and reply workflow in the client. // Setup mock to fail at a certain position. Return true if position exists otherwise false. func prepareClientCallReply(protocol *MockTProtocol, failAt int, failWith error) bool { + // NOTE: here the number 50 is the same as the last number at the end of + // this function. If more function calls are added in the future, this + // number needs to be increased accordingly. + // + // It's needed for go 1.14+. Before go 1.14 we only call gomock + // controller's Finish function when this function returns true. + // Starting from go 1.14 the gomock will take advantage of + // testing.T.Cleanup interface, which means the Finish function will + // always be called by t.Cleanup, even if we return false here. + // As a result, in the case we need to return false by this function, + // we must return before calling any of the + // protocol.EXPECT().* functions. + const lastFailAt = 50 + if failAt > lastFailAt { + return false + } + var err error = nil if failAt == 0 { diff --git a/lib/go/thrift/exception.go b/lib/go/thrift/exception.go index 48364799570..53bf862ea5b 100644 --- a/lib/go/thrift/exception.go +++ b/lib/go/thrift/exception.go @@ -34,7 +34,8 @@ type TException interface { func PrependError(prepend string, err error) error { msg := prepend + err.Error() - if te, ok := err.(TException); ok { + var te TException + if errors.As(err, &te) { switch te.TExceptionType() { case TExceptionTypeTransport: if t, ok := err.(TTransportException); ok { @@ -45,7 +46,8 @@ func PrependError(prepend string, err error) error { return prependTProtocolException(prepend, t) } case TExceptionTypeApplication: - if t, ok := err.(TApplicationException); ok { + var t TApplicationException + if errors.As(err, &t) { return NewTApplicationException(t.TypeId(), msg) } } diff --git a/lib/go/thrift/go.mod b/lib/go/thrift/go.mod new file mode 100644 index 00000000000..430c255a862 --- /dev/null +++ b/lib/go/thrift/go.mod @@ -0,0 +1,3 @@ +module github.com/apache/thrift/lib/go/thrift + +go 1.14 diff --git a/lib/go/thrift/header_protocol.go b/lib/go/thrift/header_protocol.go index 5ad48e43bd6..35e0458051c 100644 --- a/lib/go/thrift/header_protocol.go +++ b/lib/go/thrift/header_protocol.go @@ -21,6 +21,7 @@ package thrift import ( "context" + "errors" ) // THeaderProtocol is a thrift protocol that implements THeader: @@ -233,8 +234,8 @@ func (p *THeaderProtocol) ReadMessageBegin(ctx context.Context) (name string, ty var newProto TProtocol newProto, err = p.transport.Protocol().GetProtocol(p.transport) if err != nil { - tAppExc, ok := err.(TApplicationException) - if !ok { + var tAppExc TApplicationException + if !errors.As(err, &tAppExc) { return } if e := p.protocol.WriteMessageBegin(ctx, "", EXCEPTION, seqID); e != nil { diff --git a/lib/go/thrift/header_transport.go b/lib/go/thrift/header_transport.go index 1e8e302447a..f1dc99ce3a5 100644 --- a/lib/go/thrift/header_transport.go +++ b/lib/go/thrift/header_transport.go @@ -488,7 +488,7 @@ func (t *THeaderTransport) parseHeaders(ctx context.Context, frameSize uint32) e headers := make(THeaderMap) for { infoType, err := hp.readVarint32() - if err == io.EOF { + if errors.Is(err, io.EOF) { break } if err != nil { diff --git a/lib/go/thrift/http_client.go b/lib/go/thrift/http_client.go index 26e52b387d7..15015864f0f 100644 --- a/lib/go/thrift/http_client.go +++ b/lib/go/thrift/http_client.go @@ -22,6 +22,7 @@ package thrift import ( "bytes" "context" + "errors" "io" "io/ioutil" "net/http" @@ -159,7 +160,7 @@ func (p *THttpClient) Read(buf []byte) (int, error) { return 0, NewTTransportException(NOT_OPEN, "Response buffer is empty, no request.") } n, err := p.response.Body.Read(buf) - if n > 0 && (err == nil || err == io.EOF) { + if n > 0 && (err == nil || errors.Is(err, io.EOF)) { return n, nil } return n, NewTTransportExceptionFromError(err) diff --git a/lib/go/thrift/protocol_exception.go b/lib/go/thrift/protocol_exception.go index c86a9ff47d0..9dcf4bfd94c 100644 --- a/lib/go/thrift/protocol_exception.go +++ b/lib/go/thrift/protocol_exception.go @@ -21,6 +21,7 @@ package thrift import ( "encoding/base64" + "errors" ) // Thrift Protocol exception @@ -76,7 +77,7 @@ func NewTProtocolException(err error) TProtocolException { return e } - if _, ok := err.(base64.CorruptInputError); ok { + if errors.As(err, new(base64.CorruptInputError)) { return NewTProtocolExceptionWithType(INVALID_DATA, err) } diff --git a/lib/go/thrift/rich_transport.go b/lib/go/thrift/rich_transport.go index 4025bebeaa4..83fdf29f5cb 100644 --- a/lib/go/thrift/rich_transport.go +++ b/lib/go/thrift/rich_transport.go @@ -19,7 +19,10 @@ package thrift -import "io" +import ( + "errors" + "io" +) type RichTransport struct { TTransport @@ -49,7 +52,7 @@ func (r *RichTransport) RemainingBytes() (num_bytes uint64) { func readByte(r io.Reader) (c byte, err error) { v := [1]byte{0} n, err := r.Read(v[0:1]) - if n > 0 && (err == nil || err == io.EOF) { + if n > 0 && (err == nil || errors.Is(err, io.EOF)) { return v[0], nil } if n > 0 && err != nil { diff --git a/lib/go/thrift/simple_server.go b/lib/go/thrift/simple_server.go index ca0e61d0ea9..563cbfc694a 100644 --- a/lib/go/thrift/simple_server.go +++ b/lib/go/thrift/simple_server.go @@ -243,12 +243,11 @@ func treatEOFErrorsAsNil(err error) error { if err == nil { return nil } - // err could be io.EOF wrapped with TProtocolException, - // so that err == io.EOF doesn't necessarily work in some cases. - if err.Error() == io.EOF.Error() { + if errors.Is(err, io.EOF) { return nil } - if err, ok := err.(TTransportException); ok && err.TypeId() == END_OF_FILE { + var te TTransportException + if errors.As(err, &te) && te.TypeId() == END_OF_FILE { return nil } return err @@ -315,15 +314,14 @@ func (p *TSimpleServer) processRequests(client TTransport) (err error) { } ok, err := processor.Process(ctx, inputProtocol, outputProtocol) - // Once we dropped support for pre-go1.13 this can be replaced by: - // errors.Is(err, ErrAbandonRequest) - if unwrapError(err) == ErrAbandonRequest { + if errors.Is(err, ErrAbandonRequest) { return client.Close() } - if _, ok := err.(TTransportException); ok && err != nil { + if errors.As(err, new(TTransportException)) && err != nil { return err } - if err, ok := err.(TApplicationException); ok && err.TypeId() == UNKNOWN_METHOD { + var tae TApplicationException + if errors.As(err, &tae) && tae.TypeId() == UNKNOWN_METHOD { continue } if !ok { @@ -332,17 +330,3 @@ func (p *TSimpleServer) processRequests(client TTransport) (err error) { } return nil } - -type unwrapper interface { - Unwrap() error -} - -func unwrapError(err error) error { - for { - if u, ok := err.(unwrapper); ok { - err = u.Unwrap() - } else { - return err - } - } -} diff --git a/lib/go/thrift/socket_unix_conn.go b/lib/go/thrift/socket_unix_conn.go index 98e5a04dd6c..f5fab3ab653 100644 --- a/lib/go/thrift/socket_unix_conn.go +++ b/lib/go/thrift/socket_unix_conn.go @@ -22,6 +22,7 @@ package thrift import ( + "errors" "io" "syscall" "time" @@ -67,7 +68,7 @@ func (sc *socketConn) checkConn() error { return nil } - if err == syscall.EAGAIN || err == syscall.EWOULDBLOCK { + if errors.Is(err, syscall.EAGAIN) || errors.Is(err, syscall.EWOULDBLOCK) { // This means the connection is still open but we don't have // anything to read right now. return nil diff --git a/lib/go/thrift/transport_exception.go b/lib/go/thrift/transport_exception.go index 08f7ce229f2..0a3f07646d3 100644 --- a/lib/go/thrift/transport_exception.go +++ b/lib/go/thrift/transport_exception.go @@ -92,23 +92,23 @@ func NewTTransportExceptionFromError(e error) TTransportException { return t } - newTTransportException := func(typeID int, err error, prefix string) TTransportException { - return &tTransportException{ - typeId: typeID, - err: err, - msg: prefix + err.Error(), - } + te := &tTransportException{ + typeId: UNKNOWN_TRANSPORT_EXCEPTION, + err: e, + msg: e.Error(), } if isTimeoutError(e) { - return newTTransportException(TIMED_OUT, e, "") + te.typeId = TIMED_OUT + return te } - if e == io.EOF { - return newTTransportException(END_OF_FILE, e, "") + if errors.Is(e, io.EOF) { + te.typeId = END_OF_FILE + return te } - return newTTransportException(UNKNOWN_TRANSPORT_EXCEPTION, e, "") + return te } func prependTTransportException(prepend string, e TTransportException) TTransportException { @@ -119,11 +119,12 @@ func prependTTransportException(prepend string, e TTransportException) TTranspor } } -// isTimeoutError returns true when err is a timeout error. +// isTimeoutError returns true when err is an error caused by timeout. // // Note that this also includes TTransportException wrapped timeout errors. func isTimeoutError(err error) bool { - if t, ok := err.(timeoutable); ok { + var t timeoutable + if errors.As(err, &t) { return t.Timeout() } return false diff --git a/lib/go/thrift/transport_exception_test.go b/lib/go/thrift/transport_exception_test.go index 57386cb28ce..0d79409e727 100644 --- a/lib/go/thrift/transport_exception_test.go +++ b/lib/go/thrift/transport_exception_test.go @@ -47,12 +47,8 @@ func TestTExceptionTimeout(t *testing.T) { t.Errorf("TypeId was not TIMED_OUT: expected %v, got %v", TIMED_OUT, exception.TypeId()) } - // NOTE: this can also be replaced by errors.Unwrap, but that requires - // go 1.13+. - if e, ok := exception.(unwrapper); !ok { - t.Error("Expected exception to be unwrappable, it is not.") - } else if e.Unwrap() != timeout { - t.Errorf("Unwrapped exception did not match: expected %v, got %v", timeout, e.Unwrap()) + if unwrapped := errors.Unwrap(exception); unwrapped != timeout { + t.Errorf("Unwrapped exception did not match: expected %v, got %v", timeout, unwrapped) } } @@ -66,12 +62,8 @@ func TestTExceptionEOF(t *testing.T) { t.Errorf("TypeId was not END_OF_FILE: expected %v, got %v", END_OF_FILE, exception.TypeId()) } - // NOTE: this can also be replaced by errors.Unwrap, but that requires - // go 1.13+. - if e, ok := exception.(unwrapper); !ok { - t.Error("Expected exception to be unwrappable, it is not.") - } else if e.Unwrap() != io.EOF { - t.Errorf("Unwrapped exception did not match: expected %v, got %v", io.EOF, e.Unwrap()) + if unwrapped := errors.Unwrap(exception); unwrapped != io.EOF { + t.Errorf("Unwrapped exception did not match: expected %v, got %v", io.EOF, unwrapped) } }