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 0cba09d..94e1db6 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_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 @@
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 @@
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 @@
t_struct* exceptions = tfunction->get_xceptions();
const vector<t_field*>& x_fields = exceptions->get_members();
- if( ! x_fields.empty()) {
+ if( ! x_fields.empty()) {
f_service_ << indent() << "switch v := err2.(type) {" << endl;
vector<t_field*>::const_iterator xf_iter;
@@ -2398,95 +2408,57 @@
indent() << "result." << publicize(variable_name_to_go_name((*xf_iter)->get_name())) << " = v" << endl;
}
- f_service_ <<
+ f_service_ <<
indent() << " default:" << 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_ <<
- indent() << "}" << 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() << "}" << 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() << " return false, err2" << endl ;
+
+ if( ! x_fields.empty()) {
+ f_service_ <<
+ indent() << "}" << endl;
+ }
+
+ f_service_ <<
+ 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 cb6073c..046448c 100644
--- a/lib/go/test/Makefile.am
+++ b/lib/go/test/Makefile.am
@@ -21,11 +21,12 @@
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 0000000..9ba7525
--- /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 0000000..5ffbbfe
--- /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)
+ }
+}