[
https://issues.apache.org/jira/browse/THRIFT-3473?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15053895#comment-15053895
]
Jens Geyer edited comment on THRIFT-3473 at 12/12/15 1:12 AM:
--------------------------------------------------------------
I did another look into that issue, and it is not a real bug, but caused by
design. I say "caused", because the generated C++ code is a bit error prone in
that regard, but probably by design. Assumed this IDL:
{code}
struct Xtruct2
{
2: Xtruct struct_thing,
}
{code}
For Xtruct, this produces an {{__isset}} flag and the field like so:
{code}
Xtruct struct_thing;
{code}
This does not change, if we opt-in into optional, with {{2: optional Xtruct
struct_thing}} we get the (almost) same code generated. The only subtle
difference is in the {{write()}} method, because code like this will suddenly
stop to work:
{code}
Xtruct2 xs2;
xs2.struct_thing.i32_thing = 42;
xs2.write(oprot);
{code}
With default requiredness, accessing the unset field {{struct_thing}} works
even without {{__isset}} flags being properly maintained. Because it is written
anyways, nothing bad happens (unless someone checks the flag of course).
However, this code hits us really bad, when switching to optional. What makes
it so ugly is the fact, that *you will not notice that something is missing
until you try to deserialize the data*. One basically has no chance to detect
that early, other than performing the time-consuming task of revising every
single statement in your code which accesses any of the now {{optional}}
field(s). Plus you have to know what you are looking for.
*Question:* Should we do something about it? Or should we just close it as "by
design" and hope that we never fall into that trap ourselves some day?
was (Author: jensg):
I did another look into that issue, and it is not a real bug, but caused by
design. I say "caused", because the generated C++ code is a bit error prone in
that regard, but probably by design. Assumed this IDL:
{code}
struct Xtruct2
{
2: Xtruct struct_thing,
}
{code}
For Xtruct, this produces an {{__isset}} flag and the field like so:
{code}
Xtruct struct_thing;
{code}
This does not change, if we opt-in into optional, with {{2: optional Xtruct
struct_thing}} we get the (almost) same code generated. The only subtle
difference is in the {{write()}} method, because code like this will suddenly
stop to work:
{code}
Xtruct2 xs2;
xs2.struct_thing.i32_thing = 42;
xs2.write(oprot);
{code}
With default requiredness, accessing the unset field {{struct_thing}} works
even without {{__isset}} flags being properly maintained. Because it is written
anyways, nothing bad happens (unless someone checks the flag of course).
However, this code hits us really bad, when switching to optional. What makes
it so ugly is the fact, that *you will not notice that something is missing
until you try to deserialize the data*. One basically has no chance to detect
that early, other than performing the time-consuming task of revising every
single statement in your code which accesses any of the now {{optional}}
field(s).
*Question:* Should we do something about it? Or should we just close it as "by
design" and hope that we never fall into that trap ourselves some day?
> When "optional' is used with a struct member, C++ server seems to not return
> it correctly
> -----------------------------------------------------------------------------------------
>
> Key: THRIFT-3473
> URL: https://issues.apache.org/jira/browse/THRIFT-3473
> Project: Thrift
> Issue Type: Bug
> Components: C++ - Library
> Affects Versions: 0.9.3
> Environment: CentOS 7, GCC 4.8.3, Perl 5.16
> Reporter: YU Chang
>
> Any "optional" modifier relates to a list will cause to a null/undef return
> value.
> Here is thrift file:
> {code:title=test.thrift|borderStyle=solid}
> namespace cpp thrifttest
> namespace perl thrifttest
> struct Pair {
> 1: optional string a,
> 2: optional string b
> }
> struct Result {
> 1: optional list<string> strList,
> 2: optional list<Pair> pairList
> }
> service Test {
> Result listTest()
> }
> {code}
> Here is C++ code (server):
> {code:title=Test_server.cpp|borderStyle=solid}
> #include "gen-cpp/Test.h"
> #include <thrift/protocol/TBinaryProtocol.h>
> #include <thrift/server/TSimpleServer.h>
> #include <thrift/transport/TServerSocket.h>
> #include <thrift/transport/TBufferTransports.h>
> #include <iostream>
> using namespace std;
> using namespace ::apache::thrift;
> using namespace ::apache::thrift::protocol;
> using namespace ::apache::thrift::transport;
> using namespace ::apache::thrift::server;
> using boost::shared_ptr;
> using namespace ::thrifttest;
> class TestHandler : virtual public TestIf {
> public:
> TestHandler() {
> // Your initialization goes here
> }
> void listTest(Result& _return) {
> _return.strList.push_back("Test");
> _return.strList.push_back("one level list");
> cout << "strList size: " << _return.strList.size() << endl;
> Pair pair;
> pair.a = "Test";
> pair.b = "two level list";
> _return.pairList.push_back(pair);
> cout << "pairList size: " << _return.pairList.size() << endl;
> printf("call listTest\n");
> }
> };
> int main(int argc, char **argv) {
> int port = 9595;
> shared_ptr<TestHandler> handler(new TestHandler());
> shared_ptr<TProcessor> processor(new TestProcessor(handler));
> shared_ptr<TServerTransport> serverTransport(new TServerSocket(port));
> shared_ptr<TTransportFactory> transportFactory(new
> TBufferedTransportFactory());
> shared_ptr<TProtocolFactory> protocolFactory(new
> TBinaryProtocolFactory());
> TSimpleServer server(processor, serverTransport, transportFactory,
> protocolFactory);
> server.serve();
> return 0;
> }
> {code}
> Here is perl code (client):
> {code:title=Test_client.pl|borderStyle=solid}
> #!/usr/bin/perl
> use v5.12;
> use warnings;
> use autodie;
> use utf8;
> use Data::Dumper;
> use lib 'gen-perl';
> use thrifttest::Test;
> use thrifttest::Constants;
> use thrifttest::Types;
> use Thrift;
> use Thrift::BinaryProtocol;
> use Thrift::Socket;
> use Thrift::BufferedTransport;
> my $socket = new Thrift::Socket('localhost', 9595);
> my $transport = new Thrift::BufferedTransport($socket, 1024, 1024);
> my $protocol = new Thrift::BinaryProtocol($transport);
> my $client = new thrifttest::TestClient($protocol);
> eval {
> $transport->open();
> my $result = $client->listTest;
> say Dumper($result);
> $transport->close();
> };
> say $@ if $@;
> {code}
> {code:title=C++ server output|borderStyle=solid}
> strList size: 2
> pairList size: 1
> call listTest
> {code}
> {code:title=perl client output|borderStyle=solid}
> $VAR1 = bless( {
> 'pairList' => undef,
> 'strList' => undef
> }, 'thrifttest::Result' );
> {code}
> In this test case, every "optional" corresponding to a undef return value.
> Only If I delete all the "optional" in the thrift file, I will get right
> return value.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
