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 (file)
--- 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<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 @@ 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 cb6073c..046448c 100644 (file)
--- 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 (file)
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 (file)
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)
+       }
+}