From 6d9131337f8addd0cb9a50b73201772802addacd Mon Sep 17 00:00:00 2001 From: Yuxuan 'fishy' Wang Date: Fri, 22 Jan 2021 09:37:18 -0800 Subject: [PATCH] THRIFT-4914: Make TClient.Call to return the response headers Client: go Make a breaking change so that TClient.Call returns the response headers, and make a compiler change to compiler generated clients to take advantage of that and provide access to response headers to users. --- CHANGES.md | 2 ++ .../cpp/src/thrift/generate/t_go_generator.cc | 26 +++++++++++++++---- lib/go/thrift/client.go | 18 +++++++++---- .../thrift/example_client_middleware_test.go | 6 ++--- lib/go/thrift/header_protocol.go | 14 ---------- lib/go/thrift/middleware.go | 4 +-- lib/go/thrift/middleware_test.go | 6 ++--- 7 files changed, 44 insertions(+), 32 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 8e4d08edd95..d69abc55420 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -18,6 +18,7 @@ - [THRIFT-5152](https://issues.apache.org/jira/browse/THRIFT-5152) - go: TSocket and TSSLSocket now have separated connect timeout and socket timeout - c++: dropped support for Windows XP - [THRIFT-5326](https://issues.apache.org/jira/browse/THRIFT-5326) - go: TException interface now has a new function: TExceptionType +- [THRIFT-4914](https://issues.apache.org/jira/browse/THRIFT-4914) - go: TClient.Call now returns two values: THeaderMap and error ### Java @@ -31,6 +32,7 @@ - [THRIFT-5233](https://issues.apache.org/jira/browse/THRIFT-5233) - Add context deadline check to ReadMessageBegin in TBinaryProtocol, TCompactProtocol, and THeaderProtocol. - [THRIFT-5240](https://issues.apache.org/jira/browse/THRIFT-5240) - The context passed into server handler implementations will be canceled when we detected that the client closed the connection. - [THRIFT-5322](https://issues.apache.org/jira/browse/THRIFT-5322) - Add support to TConfiguration, and also fix a bug that could cause excessive memory usage when reading malformed messages from TCompactProtocol. +- [THRIFT-4914](https://issues.apache.org/jira/browse/THRIFT-4914) - Compiler generated service clients now provide a new function, LastResponseHeaders_(), to get the response headers from the last client call (if the transport/protocol used was THeader). ## 0.13.0 diff --git a/compiler/cpp/src/thrift/generate/t_go_generator.cc b/compiler/cpp/src/thrift/generate/t_go_generator.cc index 49d8bc11977..398d50e7a07 100644 --- a/compiler/cpp/src/thrift/generate/t_go_generator.cc +++ b/compiler/cpp/src/thrift/generate/t_go_generator.cc @@ -1990,6 +1990,7 @@ void t_go_generator::generate_service_client(t_service* tservice) { f_types_ << indent() << "*" << extends_client << endl; } else { f_types_ << indent() << "c thrift.TClient" << endl; + f_types_ << indent() << "headers thrift.THeaderMap" << endl; } indent_down(); @@ -2059,7 +2060,19 @@ void t_go_generator::generate_service_client(t_service* tservice) { indent_up(); f_types_ << indent() << "return p.c" << endl; indent_down(); - f_types_ << indent() << "}" << endl; + f_types_ << indent() << "}" << endl << endl; + + f_types_ << indent() << "func (p *" << serviceName << "Client) LastResponseHeaders_() thrift.THeaderMap {" << endl; + indent_up(); + f_types_ << indent() << "return p.headers" << endl; + indent_down(); + f_types_ << indent() << "}" << endl << endl; + + f_types_ << indent() << "func (p *" << serviceName << "Client) SetLastResponseHeaders_(headers thrift.THeaderMap) {" << endl; + indent_up(); + f_types_ << indent() << "p.headers = headers" << endl; + indent_down(); + f_types_ << indent() << "}" << endl << endl; } // Generate client method implementations @@ -2091,8 +2104,11 @@ void t_go_generator::generate_service_client(t_service* tservice) { std::string resultName = tmp("_result"); std::string resultType = publicize(method + "_result", true); f_types_ << indent() << "var " << resultName << " " << resultType << endl; - f_types_ << indent() << "if err = p.Client_().Call(ctx, \"" - << method << "\", &" << argsName << ", &" << resultName << "); err != nil {" << endl; + f_types_ << indent() << "var headers thrift.THeaderMap" << endl; + f_types_ << indent() << "headers, err = p.Client_().Call(ctx, \"" + << method << "\", &" << argsName << ", &" << resultName << ")" << endl; + f_types_ << indent() << "p.SetLastResponseHeaders_(headers)" << endl; + f_types_ << indent() << "if err != nil {" << endl; indent_up(); f_types_ << indent() << "return" << endl; @@ -2132,8 +2148,8 @@ void t_go_generator::generate_service_client(t_service* tservice) { } } else { // TODO: would be nice to not to duplicate the call generation - f_types_ << indent() << "if err := p.Client_().Call(ctx, \"" - << method << "\", &"<< argsName << ", nil); err != nil {" << endl; + f_types_ << indent() << "if _, err := p.Client_().Call(ctx, \"" + << method << "\", &"<< argsName << ", nil); err != nil {" << endl; indent_up(); f_types_ << indent() << "return err" << endl; diff --git a/lib/go/thrift/client.go b/lib/go/thrift/client.go index 1c5705d5522..1a31edee3b8 100644 --- a/lib/go/thrift/client.go +++ b/lib/go/thrift/client.go @@ -6,7 +6,10 @@ import ( ) type TClient interface { - Call(ctx context.Context, method string, args, result TStruct) error + // The THeaderMap return value is the headers in the response, if any. + // If the underlying transport/protocol is not THeader, + // the returned THeaderMap will always be nil. + Call(ctx context.Context, method string, args, result TStruct) (THeaderMap, error) } type TStandardClient struct { @@ -78,18 +81,23 @@ func (p *TStandardClient) Recv(ctx context.Context, iprot TProtocol, seqId int32 return iprot.ReadMessageEnd(ctx) } -func (p *TStandardClient) Call(ctx context.Context, method string, args, result TStruct) error { +func (p *TStandardClient) Call(ctx context.Context, method string, args, result TStruct) (THeaderMap, error) { p.seqId++ seqId := p.seqId if err := p.Send(ctx, p.oprot, seqId, method, args); err != nil { - return err + return nil, err } // method is oneway if result == nil { - return nil + return nil, nil } - return p.Recv(ctx, p.iprot, seqId, method, result) + err := p.Recv(ctx, p.iprot, seqId, method, result) + var headers THeaderMap + if hp, ok := p.iprot.(*THeaderProtocol); ok { + headers = hp.transport.readHeaders + } + return headers, err } diff --git a/lib/go/thrift/example_client_middleware_test.go b/lib/go/thrift/example_client_middleware_test.go index 8a29083c088..52e987bacae 100644 --- a/lib/go/thrift/example_client_middleware_test.go +++ b/lib/go/thrift/example_client_middleware_test.go @@ -45,16 +45,16 @@ func NewMyServiceClient(_ TClient) MyService { func simpleClientLoggingMiddleware(next TClient) TClient { return WrappedTClient{ - Wrapped: func(ctx context.Context, method string, args, result TStruct) error { + Wrapped: func(ctx context.Context, method string, args, result TStruct) (THeaderMap, error) { log.Printf("Before: %q", method) log.Printf("Args: %#v", args) - err := next.Call(ctx, method, args, result) + headers, err := next.Call(ctx, method, args, result) log.Printf("After: %q", method) log.Printf("Result: %#v", result) if err != nil { log.Printf("Error: %v", err) } - return err + return headers, err }, } } diff --git a/lib/go/thrift/header_protocol.go b/lib/go/thrift/header_protocol.go index 35e0458051c..878041f8df1 100644 --- a/lib/go/thrift/header_protocol.go +++ b/lib/go/thrift/header_protocol.go @@ -345,20 +345,6 @@ func (p *THeaderProtocol) SetTConfiguration(cfg *TConfiguration) { p.cfg = cfg } -// GetResponseHeadersFromClient is a helper function to get the read THeaderMap -// from the last response received from the given client. -// -// If the last response was not sent over THeader protocol, -// a nil map will be returned. -func GetResponseHeadersFromClient(c TClient) THeaderMap { - if sc, ok := c.(*TStandardClient); ok { - if hp, ok := sc.iprot.(*THeaderProtocol); ok { - return hp.transport.readHeaders - } - } - return nil -} - var ( _ TConfigurationSetter = (*tHeaderProtocolFactory)(nil) _ TConfigurationSetter = (*THeaderProtocol)(nil) diff --git a/lib/go/thrift/middleware.go b/lib/go/thrift/middleware.go index b575e16c42a..a0233e865f2 100644 --- a/lib/go/thrift/middleware.go +++ b/lib/go/thrift/middleware.go @@ -78,11 +78,11 @@ type ClientMiddleware func(TClient) TClient // // This is provided to aid in developing ClientMiddleware. type WrappedTClient struct { - Wrapped func(ctx context.Context, method string, args, result TStruct) error + Wrapped func(ctx context.Context, method string, args, result TStruct) (THeaderMap, error) } // Call implements the TClient interface by calling and returning c.Wrapped. -func (c WrappedTClient) Call(ctx context.Context, method string, args, result TStruct) error { +func (c WrappedTClient) Call(ctx context.Context, method string, args, result TStruct) (THeaderMap, error) { return c.Wrapped(ctx, method, args, result) } diff --git a/lib/go/thrift/middleware_test.go b/lib/go/thrift/middleware_test.go index 2a4d1f9574d..cfd4659f96b 100644 --- a/lib/go/thrift/middleware_test.go +++ b/lib/go/thrift/middleware_test.go @@ -54,7 +54,7 @@ func testProcessorMiddleware(c *counter) ProcessorMiddleware { func testClientMiddleware(c *counter) ClientMiddleware { return func(next TClient) TClient { return WrappedTClient{ - Wrapped: func(ctx context.Context, method string, args, result TStruct) error { + Wrapped: func(ctx context.Context, method string, args, result TStruct) (THeaderMap, error) { c.incr() return next.Call(ctx, method, args, result) }, @@ -122,8 +122,8 @@ func TestWrapTMultiplexedProcessor(t *testing.T) { func TestWrapClient(t *testing.T) { client := WrappedTClient{ - Wrapped: func(ctx context.Context, method string, args, result TStruct) error { - return nil + Wrapped: func(ctx context.Context, method string, args, result TStruct) (THeaderMap, error) { + return nil, nil }, } c := newCounter(t)