From: Jens Geyer Date: Mon, 14 Apr 2014 20:36:50 +0000 (+0200) Subject: THRIFT-2458 Generated golang server code for "oneway" methods is incorrect X-Git-Url: https://source.supwisdom.com/gerrit/gitweb?a=commitdiff_plain;h=c2ccca8e4b4fd1b421e2d30b754aee8c24e90b34;p=common%2Fthrift.git THRIFT-2458 Generated golang server code for "oneway" methods is incorrect Client: Go compiler Patch: Jens Geyer & Aleksey Pesternikov --- diff --git a/compiler/cpp/src/generate/t_go_generator.cc b/compiler/cpp/src/generate/t_go_generator.cc index 0cba09dc..94e1db61 100644 --- a/compiler/cpp/src/generate/t_go_generator.cc +++ b/compiler/cpp/src/generate/t_go_generator.cc @@ -1444,7 +1444,7 @@ void t_go_generator::generate_service_helpers(t_service* tservice) */ void t_go_generator::generate_go_function_helpers(t_function* tfunction) { - if (true || !tfunction->is_oneway()) { + if (!tfunction->is_oneway()) { t_struct result(program_, tfunction->get_name() + "_result"); t_field success(tfunction->get_returntype(), "success", 0); @@ -1707,7 +1707,7 @@ void t_go_generator::generate_service_client(t_service* tservice) f_service_ << indent() << "}" << endl << endl; - if (true) { //!(*f_iter)->is_oneway() || true) {} + if (!(*f_iter)->is_oneway()) { std::string resultname = publicize((*f_iter)->get_name() + "_result",true); // Open function f_service_ << endl << @@ -2346,16 +2346,26 @@ void t_go_generator::generate_process_function(t_service* tservice, f_service_ << indent() << "args := New" << argsname << "()" << endl << indent() << "if err = args.Read(iprot); err != nil {" << endl << - indent() << " iprot.ReadMessageEnd()" << endl << - indent() << " x := thrift.NewTApplicationException(thrift.PROTOCOL_ERROR, err.Error())" << endl << - indent() << " oprot.WriteMessageBegin(\"" << escape_string(tfunction->get_name()) << "\", thrift.EXCEPTION, seqId)" << endl << - indent() << " x.Write(oprot)" << endl << - indent() << " oprot.WriteMessageEnd()" << endl << - indent() << " oprot.Flush()" << endl << - indent() << " return" << endl << - indent() << "}" << endl << - indent() << "iprot.ReadMessageEnd()" << endl << - indent() << "result := New" << resultname << "()" << endl << + indent() << " iprot.ReadMessageEnd()" << endl; + if (!tfunction->is_oneway()) { + f_service_ << + indent() << " x := thrift.NewTApplicationException(thrift.PROTOCOL_ERROR, err.Error())" << endl << + indent() << " oprot.WriteMessageBegin(\"" << escape_string(tfunction->get_name()) << "\", thrift.EXCEPTION, seqId)" << endl << + indent() << " x.Write(oprot)" << endl << + indent() << " oprot.WriteMessageEnd()" << endl << + indent() << " oprot.Flush()" << endl; + } + f_service_ << + indent() << " return false, err" << endl << + indent() << "}" << endl << endl << + indent() << "iprot.ReadMessageEnd()" << endl; + + if (!tfunction->is_oneway()) { + f_service_ << + indent() << "result := New" << resultname << "()" << endl; + } + + f_service_ << indent() << "var err2 error" << endl << indent() << "if "; @@ -2387,7 +2397,7 @@ void t_go_generator::generate_process_function(t_service* tservice, t_struct* exceptions = tfunction->get_xceptions(); const vector& x_fields = exceptions->get_members(); - if( ! x_fields.empty()) { + if( ! x_fields.empty()) { f_service_ << indent() << "switch v := err2.(type) {" << endl; vector::const_iterator xf_iter; @@ -2398,95 +2408,57 @@ void t_go_generator::generate_process_function(t_service* tservice, indent() << "result." << publicize(variable_name_to_go_name((*xf_iter)->get_name())) << " = v" << endl; } - f_service_ << + f_service_ << indent() << " default:" << endl; } + if (!tfunction->is_oneway()) { + f_service_ << + indent() << " x := thrift.NewTApplicationException(thrift.INTERNAL_ERROR, \"Internal error processing " << + escape_string(tfunction->get_name()) << ": \" + err2.Error())" << endl << + indent() << " oprot.WriteMessageBegin(\"" << escape_string(tfunction->get_name()) << "\", thrift.EXCEPTION, seqId)" << endl << + indent() << " x.Write(oprot)" << endl << + indent() << " oprot.WriteMessageEnd()" << endl << + indent() << " oprot.Flush()" << endl ; + } + f_service_ << - indent() << " x := thrift.NewTApplicationException(thrift.INTERNAL_ERROR, \"Internal error processing " << escape_string(tfunction->get_name()) << ": \" + err2.Error())" << endl << - indent() << " oprot.WriteMessageBegin(\"" << escape_string(tfunction->get_name()) << "\", thrift.EXCEPTION, seqId)" << endl << - indent() << " x.Write(oprot)" << endl << - indent() << " oprot.WriteMessageEnd()" << endl << - indent() << " oprot.Flush()" << endl << indent() << " return false, err2" << endl ; - + if( ! x_fields.empty()) { - f_service_ << + f_service_ << indent() << "}" << endl; - } + } f_service_ << - indent() << "}" << endl << - indent() << "if err2 = oprot.WriteMessageBegin(\"" << escape_string(tfunction->get_name()) << "\", thrift.REPLY, seqId); err2 != nil {" << endl << - indent() << " err = err2" << endl << - indent() << "}" << endl << - indent() << "if err2 = result.Write(oprot); err == nil && err2 != nil {" << endl << - indent() << " err = err2" << endl << - indent() << "}" << endl << - indent() << "if err2 = oprot.WriteMessageEnd(); err == nil && err2 != nil {" << endl << - indent() << " err = err2" << endl << - indent() << "}" << endl << - indent() << "if err2 = oprot.Flush(); err == nil && err2 != nil {" << endl << - indent() << " err = err2" << endl << - indent() << "}" << endl << - indent() << "if err != nil {" << endl << - indent() << " return" << endl << - indent() << "}" << endl << - indent() << "return true, err" << endl; + indent() << "}" << endl; + + if (!tfunction->is_oneway()) { + f_service_ << + indent() << "if err2 = oprot.WriteMessageBegin(\"" << escape_string(tfunction->get_name()) << + "\", thrift.REPLY, seqId); err2 != nil {" << endl << + indent() << " err = err2" << endl << + indent() << "}" << endl << + indent() << "if err2 = result.Write(oprot); err == nil && err2 != nil {" << endl << + indent() << " err = err2" << endl << + indent() << "}" << endl << + indent() << "if err2 = oprot.WriteMessageEnd(); err == nil && err2 != nil {" << endl << + indent() << " err = err2" << endl << + indent() << "}" << endl << + indent() << "if err2 = oprot.Flush(); err == nil && err2 != nil {" << endl << + indent() << " err = err2" << endl << + indent() << "}" << endl << + indent() << "if err != nil {" << endl << + indent() << " return" << endl << + indent() << "}" << endl << + indent() << "return true, err" << endl; + } else { + f_service_ << + indent() << "return true, nil" << endl; + } indent_down(); f_service_ << indent() << "}" << endl << endl; - /* - indent(f_service_) << - "func (p *" << publicize(tservice->get_name()) << "Client) WriteResultsSuccess" << publicize(tfunction->get_name()) << - "(success bool, result " << publicize(tfunction->get_name()) << "Result, seqid int32, oprot thrift.TProtocol) (err error) {" << endl; - indent_up(); - f_service_ << - indent() << "result.Success = success" << endl << - indent() << "oprot.WriteMessageBegin(\"" << escape_string(tfunction->get_name()) << "\", thrift.REPLY, seqid)" << endl << - indent() << "result.Write(oprot)" << endl << - indent() << "oprot.WriteMessageEnd()" << endl << - indent() << "oprot.Flush()" << endl << - indent() << "return" << endl; - indent_down(); - f_service_ << - indent() << "}" << endl << endl; - */ - // Try block for a function with exceptions - /* - if (!tfunction->is_oneway() && xceptions.size() > 0) { - indent(f_service_) << - "func (p *" << publicize(tservice->get_name()) << "Client) WriteResultsException" << publicize(tfunction->get_name()) << - "(error *" << publicize(tfunction->get_name()) << ", result *, seqid, oprot) (err error) {" << endl; - indent_up(); - - // Kinda absurd - for (x_iter = xceptions.begin(); x_iter != xceptions.end(); ++x_iter) { - f_service_ << - indent() << "except " << type_name((*x_iter)->get_type()) << ", " << (*x_iter)->get_name() << ":" << endl; - if (!tfunction->is_oneway()) { - indent_up(); - f_service_ << - indent() << "result." << (*x_iter)->get_name() << " = " << (*x_iter)->get_name() << endl; - indent_down(); - } else { - f_service_ << - indent() << "pass" << endl; - } - } - f_service_ << - indent() << "err = oprot.WriteMessageBegin(\"" << escape_string(tfunction->get_name()) << "\", thrift.REPLY, seqid)" << endl << - indent() << "if err != nil { return err }" << endl << - indent() << "err = result.Write(oprot)" << endl << - indent() << "if err != nil { return err }" << endl << - indent() << "err = oprot.WriteMessageEnd()" << endl << - indent() << "if err != nil { return err }" << endl << - indent() << "err = oprot.Flush()" << endl << - indent() << "if err != nil { return err }" << endl; - indent_down(); - f_service_ << "}" << endl << endl; - } - */ } /** diff --git a/lib/go/test/Makefile.am b/lib/go/test/Makefile.am index cb6073c1..046448ce 100644 --- a/lib/go/test/Makefile.am +++ b/lib/go/test/Makefile.am @@ -21,11 +21,12 @@ THRIFT = $(top_srcdir)/compiler/cpp/thrift -out gopath/src/ --gen go:thrift_impo THRIFTTEST = $(top_srcdir)/test/ThriftTest.thrift # Thrift for GO has problems with complex map keys: THRIFT-2063 -gopath: $(top_srcdir)/compiler/cpp/thrift $(THRIFTTEST) IncludesTest.thrift NamespacedTest.thrift +gopath: $(top_srcdir)/compiler/cpp/thrift $(THRIFTTEST) IncludesTest.thrift NamespacedTest.thrift OnewayTest.thrift mkdir -p gopath/src grep -v list.*map.*list.*map $(THRIFTTEST) > ThriftTest.thrift $(THRIFT) -r IncludesTest.thrift $(THRIFT) BinaryKeyTest.thrift + $(THRIFT) OnewayTest.thrift ln -nfs ../../../thrift gopath/src/thrift ln -nfs ../../tests gopath/src/tests touch gopath diff --git a/lib/go/test/OnewayTest.thrift b/lib/go/test/OnewayTest.thrift new file mode 100644 index 00000000..9ba75253 --- /dev/null +++ b/lib/go/test/OnewayTest.thrift @@ -0,0 +1,5 @@ +service OneWay { + oneway void hi(1: i64 i, 2: string s) + void emptyfunc() + i64 echo_int(1: i64 param) +} diff --git a/lib/go/test/tests/one_way_test.go b/lib/go/test/tests/one_way_test.go new file mode 100644 index 00000000..5ffbbfe4 --- /dev/null +++ b/lib/go/test/tests/one_way_test.go @@ -0,0 +1,89 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package tests + +import ( + "OnewayTest" + "fmt" + "net" + "testing" + "thrift" + "time" +) + +func findPort() net.Addr { + if l, err := net.Listen("tcp", "127.0.0.1:0"); err != nil { + panic("Could not find available server port") + } else { + defer l.Close() + return l.Addr() + } +} + +type impl struct{} + +func (i *impl) Hi(in int64, s string) (err error) { fmt.Println("Hi!"); return } +func (i *impl) Emptyfunc() (err error) { return } +func (i *impl) EchoInt(param int64) (r int64, err error) { return param, nil } + +const TIMEOUT = time.Second + +var addr net.Addr +var server *thrift.TSimpleServer +var client *OnewayTest.OneWayClient + +func TestInitOneway(t *testing.T) { + var err error + addr = findPort() + serverTransport, err := thrift.NewTServerSocketTimeout(addr.String(), TIMEOUT) + if err != nil { + t.Fatal("Unable to create server socket", err) + } + processor := OnewayTest.NewOneWayProcessor(&impl{}) + server = thrift.NewTSimpleServer2(processor, serverTransport) + + go server.Serve() +} + +func TestInitOnewayClient(t *testing.T) { + transport := thrift.NewTSocketFromAddrTimeout(addr, TIMEOUT) + protocol := thrift.NewTBinaryProtocolTransport(transport) + client = OnewayTest.NewOneWayClientProtocol(transport, protocol, protocol) + err := transport.Open() + if err != nil { + t.Fatal("Unable to open client socket", err) + } +} + +func TestCallOnewayServer(t *testing.T) { + //call oneway function + err := client.Hi(1, "") + if err != nil { + t.Fatal("Unexpected error: ", err) + } + //There is no way to detect protocol problems with single oneway call so we call it second time + i, err := client.EchoInt(42) + if err != nil { + t.Fatal("Unexpected error: ", err) + } + if i != 42 { + t.Fatal("Unexpected returned value: ", i) + } +}