From 5b8b4845488b07007a311a0702107d6d8abe0052 Mon Sep 17 00:00:00 2001 From: Bryan Duxbury Date: Wed, 1 Apr 2009 20:10:15 +0000 Subject: [PATCH] THRIFT-417. rb: BufferedTransport can enter an infinite loop Switch native proto implementations to use read_all instead of read. Add a bunch of tests to verify. Also: - removed some commented code in binary_protocol_accelerated.c - struct.c was missing one of the possible native method calls - updates gem manifest (included files that didn't exist) - fixed svn:ignores of test/rb/gen-rb and lib/java/gen-java git-svn-id: https://svn.apache.org/repos/asf/incubator/thrift/trunk@761037 13f79535-47bb-0310-9956-ffa450edef68 --- .../thrift/test/TCompactProtocolTest.java | 15 ++++ lib/rb/Manifest | 2 - lib/rb/ext/binary_protocol_accelerated.c | 2 +- lib/rb/ext/constants.h | 2 +- lib/rb/ext/macros.h | 2 +- lib/rb/ext/struct.c | 7 +- lib/rb/ext/thrift_native.c | 4 +- lib/rb/spec/binaryprotocol_spec_shared.rb | 73 +++++++++++++++++++ lib/rb/spec/spec_helper.rb | 2 + test/DebugProtoTest.thrift | 47 +++++++----- 10 files changed, 124 insertions(+), 32 deletions(-) diff --git a/lib/java/test/org/apache/thrift/test/TCompactProtocolTest.java b/lib/java/test/org/apache/thrift/test/TCompactProtocolTest.java index 450711b4..5d1c3cba 100755 --- a/lib/java/test/org/apache/thrift/test/TCompactProtocolTest.java +++ b/lib/java/test/org/apache/thrift/test/TCompactProtocolTest.java @@ -365,6 +365,21 @@ public class TCompactProtocolTest { public int Janky(int i32arg) throws TException { return i32arg * 2; } + + public int primitiveMethod() throws TException { + // TODO Auto-generated method stub + return 0; + } + + public CompactProtoTestStruct structMethod() throws TException { + // TODO Auto-generated method stub + return null; + } + + public void voidMethod() throws TException { + // TODO Auto-generated method stub + + } }; Srv.Processor testProcessor = new Srv.Processor(handler); diff --git a/lib/rb/Manifest b/lib/rb/Manifest index eacaaee9..17439ed2 100644 --- a/lib/rb/Manifest +++ b/lib/rb/Manifest @@ -7,7 +7,6 @@ benchmark/gen-rb/BenchmarkService.rb benchmark/server.rb benchmark/thin_server.rb CHANGELOG -COPYING ext/binary_protocol_accelerated.c ext/binary_protocol_accelerated.h ext/compact_protocol.c @@ -43,7 +42,6 @@ lib/thrift/transport/unixsocket.rb lib/thrift/transport.rb lib/thrift/types.rb lib/thrift.rb -LICENSE Manifest README spec/binaryprotocol_spec.rb diff --git a/lib/rb/ext/binary_protocol_accelerated.c b/lib/rb/ext/binary_protocol_accelerated.c index 751281e2..728a0572 100644 --- a/lib/rb/ext/binary_protocol_accelerated.c +++ b/lib/rb/ext/binary_protocol_accelerated.c @@ -336,7 +336,7 @@ VALUE rb_thrift_binary_proto_read_set_begin(VALUE self) { VALUE rb_thrift_binary_proto_read_bool(VALUE self) { char byte = read_byte_direct(self); - return byte == 1 ? Qtrue : Qfalse; + return byte != 0 ? Qtrue : Qfalse; } VALUE rb_thrift_binary_proto_read_byte(VALUE self) { diff --git a/lib/rb/ext/constants.h b/lib/rb/ext/constants.h index 2b180818..57df544b 100644 --- a/lib/rb/ext/constants.h +++ b/lib/rb/ext/constants.h @@ -73,7 +73,7 @@ extern ID sort_method_id; extern ID write_field_stop_method_id; extern ID skip_method_id; extern ID write_method_id; -extern ID read_method_id; +extern ID read_all_method_id; extern ID native_qmark_method_id; extern ID fields_const_id; diff --git a/lib/rb/ext/macros.h b/lib/rb/ext/macros.h index 300b1cc1..265f6930 100644 --- a/lib/rb/ext/macros.h +++ b/lib/rb/ext/macros.h @@ -22,7 +22,7 @@ #define GET_STRICT_WRITE(obj) rb_ivar_get(obj, strict_write_ivar_id) #define WRITE(obj, data, length) rb_funcall(obj, write_method_id, 1, rb_str_new(data, length)) #define CHECK_NIL(obj) if (NIL_P(obj)) { rb_raise(rb_eStandardError, "nil argument not allowed!");} -#define READ(obj, length) rb_funcall(GET_TRANSPORT(obj), read_method_id, 1, INT2FIX(length)) +#define READ(obj, length) rb_funcall(GET_TRANSPORT(obj), read_all_method_id, 1, INT2FIX(length)) #ifndef RFLOAT_VALUE # define RFLOAT_VALUE(v) RFLOAT(rb_Float(v))->value diff --git a/lib/rb/ext/struct.c b/lib/rb/ext/struct.c index 8dd5c3ea..fee285e9 100644 --- a/lib/rb/ext/struct.c +++ b/lib/rb/ext/struct.c @@ -416,11 +416,6 @@ static VALUE rb_thrift_struct_write(VALUE self, VALUE protocol) { // call validate rb_funcall(self, validate_method_id, 0); - // if (rb_funcall(protocol, native_qmark_method_id, 0) == Qtrue) { - // set_native_proto_function_pointers(protocol); - // } else { - // set_default_proto_function_pointers(); - // } check_native_proto_method_table(protocol); // write struct begin @@ -565,7 +560,7 @@ static VALUE rb_thrift_struct_read(VALUE self, VALUE protocol) { // read each field while (true) { - VALUE field_header = rb_funcall(protocol, read_field_begin_method_id, 0); + VALUE field_header = mt->read_field_begin(protocol); VALUE field_type_value = rb_ary_entry(field_header, 1); int field_type = FIX2INT(field_type_value); diff --git a/lib/rb/ext/thrift_native.c b/lib/rb/ext/thrift_native.c index 7e71ad39..effa202c 100644 --- a/lib/rb/ext/thrift_native.c +++ b/lib/rb/ext/thrift_native.c @@ -87,7 +87,7 @@ ID sort_method_id; ID write_field_stop_method_id; ID skip_method_id; ID write_method_id; -ID read_method_id; +ID read_all_method_id; ID native_qmark_method_id; // constant ids @@ -169,7 +169,7 @@ void Init_thrift_native() { write_field_stop_method_id = rb_intern("write_field_stop"); skip_method_id = rb_intern("skip"); write_method_id = rb_intern("write"); - read_method_id = rb_intern("read"); + read_all_method_id = rb_intern("read_all"); native_qmark_method_id = rb_intern("native?"); // constant ids diff --git a/lib/rb/spec/binaryprotocol_spec_shared.rb b/lib/rb/spec/binaryprotocol_spec_shared.rb index 90371b35..18ea8e8d 100644 --- a/lib/rb/spec/binaryprotocol_spec_shared.rb +++ b/lib/rb/spec/binaryprotocol_spec_shared.rb @@ -299,4 +299,77 @@ shared_examples_for 'a binary protocol' do @trans.write([str.size].pack("N") + str) @prot.read_string.should == str end + + it "should perform a complete rpc with no args or return" do + srv_test( + proc {|client| client.send_voidMethod()}, + proc {|client| client.recv_voidMethod.should == nil} + ) + end + + it "should perform a complete rpc with a primitive return type" do + srv_test( + proc {|client| client.send_primitiveMethod()}, + proc {|client| client.recv_primitiveMethod.should == 1} + ) + end + + it "should perform a complete rpc with a struct return type" do + srv_test( + proc {|client| client.send_structMethod()}, + proc {|client| + result = client.recv_structMethod + result.set_byte_map = nil + result.map_byte_map = nil + result.should == Fixtures::COMPACT_PROTOCOL_TEST_STRUCT + } + ) + end + + def get_socket_connection + server = Thrift::ServerSocket.new("localhost", 9090) + server.listen + + clientside = Thrift::Socket.new("localhost", 9090) + clientside.open + serverside = server.accept + [clientside, serverside, server] + end + + def srv_test(firstblock, secondblock) + clientside, serverside, server = get_socket_connection + + clientproto = protocol_class.new(clientside) + serverproto = protocol_class.new(serverside) + + processor = Srv::Processor.new(SrvHandler.new) + + client = Srv::Client.new(clientproto, clientproto) + + # first block + firstblock.call(client) + + processor.process(serverproto, serverproto) + + # second block + secondblock.call(client) + ensure + clientside.close + serverside.close + server.close + end + + class SrvHandler + def voidMethod() + end + + def primitiveMethod + 1 + end + + def structMethod + Fixtures::COMPACT_PROTOCOL_TEST_STRUCT + end + end + end diff --git a/lib/rb/spec/spec_helper.rb b/lib/rb/spec/spec_helper.rb index 19db02a5..d0994914 100644 --- a/lib/rb/spec/spec_helper.rb +++ b/lib/rb/spec/spec_helper.rb @@ -48,4 +48,6 @@ require File.dirname(__FILE__) + "/../debug_proto_test/gen-rb/Srv" module Fixtures COMPACT_PROTOCOL_TEST_STRUCT = CompactProtoTestStruct.new(:a_binary => [0,1,2,3,4,5,6,7,8].pack('c*')) + COMPACT_PROTOCOL_TEST_STRUCT.set_byte_map = nil + COMPACT_PROTOCOL_TEST_STRUCT.map_byte_map = nil end \ No newline at end of file diff --git a/test/DebugProtoTest.thrift b/test/DebugProtoTest.thrift index d85bd522..48a8d6ff 100644 --- a/test/DebugProtoTest.thrift +++ b/test/DebugProtoTest.thrift @@ -97,25 +97,6 @@ struct Base64 { 7: binary b6, } -service Srv { - i32 Janky(i32 arg) -} - -service Inherited extends Srv { - i32 identity(i32 arg) -} - -service EmptyService {} - -// The only purpose of this thing is to increase the size of the generated code -// so that ZlibTest has more highly compressible data to play with. -struct BlowUp { - 1: map,set>> b1; - 2: map,set>> b2; - 3: map,set>> b3; - 4: map,set>> b4; -} - struct CompactProtoTestStruct { // primitive fields 1: byte a_byte = 127; @@ -178,3 +159,31 @@ struct CompactProtoTestStruct { 48: map> byte_set_map = {0 : [], 1 : [1], 2 : [1, 2]}; 49: map> byte_list_map = {0 : [], 1 : [1], 2 : [1, 2]}; } + + + +service Srv { + i32 Janky(i32 arg); + + // return type only methods + + void voidMethod(); + i32 primitiveMethod(); + CompactProtoTestStruct structMethod(); +} + +service Inherited extends Srv { + i32 identity(i32 arg) +} + +service EmptyService {} + +// The only purpose of this thing is to increase the size of the generated code +// so that ZlibTest has more highly compressible data to play with. +struct BlowUp { + 1: map,set>> b1; + 2: map,set>> b2; + 3: map,set>> b3; + 4: map,set>> b4; +} + -- 2.17.1