You are viewing a plain text version of this content. The canonical link for it is here.
Posted to user@thrift.apache.org by Piscium <gr...@gmail.com> on 2012/04/02 09:47:12 UTC
asymmetrical __isset
On C++ generated code, __isset is set to true in the read function
for the fields that have been received, however it is not looked at at
all by the write function which always writes all fields regardless of
the __isset value.
What is the reasoning behind this asymmetry? Is __isset intended only
to cater for the receiving side, in particular for the use case in
which the receiver is of a different software version than the sender?
Re: asymmetrical __isset
Posted by Piscium <gr...@gmail.com>.
On 10 April 2012 00:38, Mark Slee <ms...@fb.com> wrote:
> My gut reaction to adding "lite" is that it would probably increase
> confusion, as we'd now have another special keyword to learn and still
> have the same default behavior (which we need for backwards-compatibility,
> to your point).
>
> Better documentation seems like the fastest and simplest route to greater
> sanity, in my opinion.
Yes, I think you are right, it is better just to document how it works.
Thanks.
Re: asymmetrical __isset
Posted by Mark Slee <ms...@fb.com>.
Personally I defer to the community at this point as I'm not so actively
involved in development these days - we're open source! If you feel
strongly about this you should definitely feel welcome to join the dev
list and discuss it, or even sketch out a patch.
My gut reaction to adding "lite" is that it would probably increase
confusion, as we'd now have another special keyword to learn and still
have the same default behavior (which we need for backwards-compatibility,
to your point).
Better documentation seems like the fastest and simplest route to greater
sanity, in my opinion.
On 4/7/12 9:42 AM, "Piscium" <gr...@gmail.com> wrote:
>On 5 April 2012 21:04, Mark Slee <ms...@fb.com> wrote:
>
>> Your technical analysis is correct, as well as the historical inference!
>> The "required" and "optional" modifiers were not present in the first
>> version of the IDL, and the original intent was to make using Thrift as
>> lightweight and transparent as possible. There was an assumption that
>>many
>> clients would be written in scripting languages, where wrapping
>>everything
>> up in setters/getters would feel unnatural - we didn't want to make
>> modifying fields require a function call (via a setter) or require the
>> programmer to manually maintain all the isset flags. So all fields would
>> be sent regardless of isset, with one important exception - languages
>>with
>> nil/null don't send empty fields.
>>
>> The required/optional keywords were added later to make it easier to
>> automate some of this checking, and to make it possible for clients in
>> null-less languages (i.e. C++) to explicitly omit optional fields.
>>
>> The downside to this approach is that it does make the default behavior
>> more opaque. Hope that sheds some light on it.
>
>Hi Mark,
>
>Thanks a lot for that. I think it is very helpful to have a bit of
>knowledge about the history of Thrift as well as the ideas that guided
>its development, as it makes easier to understand it.
>
>Now one suggestion.
>
>The current situation with the field qualifiers (optional/required) is
>not ideal. A sign of that is that I thought that a field without a
>qualifier was optional while Rush thought it was required, and neither
>of us was correct, as it actually has different semantics (for
>historical reasons, as you explained).
>
>Changing the parser to disallow a field definition without a qualifier
>does not seem a good idea as it would break backward compatibility.
>
>A better solution IMO would be to add an extra qualifier, that could
>be named, for example, "lite". A field definition without a qualifier
>would be considered to have the default "lite" qualifier, and a "lite"
>field would be exactly the same as currently a field without a
>qualifier (i. e. always sent on transmission and not causing a
>exception if not present on reception for C++, and whatever default
>behaviour is currently used for the other supported languages).
>
>Obviously this would require code changes to the lexical analyser,
>parser and code generators for all the supported languages, so the
>question could be asked if the benefits justify the cost.
>Alternatively the current behaviour could just be explained on the
>wiki with 99.99% less effort!
>
>What do you think?
Re: asymmetrical __isset
Posted by Piscium <gr...@gmail.com>.
On 5 April 2012 21:04, Mark Slee <ms...@fb.com> wrote:
> Your technical analysis is correct, as well as the historical inference!
> The "required" and "optional" modifiers were not present in the first
> version of the IDL, and the original intent was to make using Thrift as
> lightweight and transparent as possible. There was an assumption that many
> clients would be written in scripting languages, where wrapping everything
> up in setters/getters would feel unnatural - we didn't want to make
> modifying fields require a function call (via a setter) or require the
> programmer to manually maintain all the isset flags. So all fields would
> be sent regardless of isset, with one important exception - languages with
> nil/null don't send empty fields.
>
> The required/optional keywords were added later to make it easier to
> automate some of this checking, and to make it possible for clients in
> null-less languages (i.e. C++) to explicitly omit optional fields.
>
> The downside to this approach is that it does make the default behavior
> more opaque. Hope that sheds some light on it.
Hi Mark,
Thanks a lot for that. I think it is very helpful to have a bit of
knowledge about the history of Thrift as well as the ideas that guided
its development, as it makes easier to understand it.
Now one suggestion.
The current situation with the field qualifiers (optional/required) is
not ideal. A sign of that is that I thought that a field without a
qualifier was optional while Rush thought it was required, and neither
of us was correct, as it actually has different semantics (for
historical reasons, as you explained).
Changing the parser to disallow a field definition without a qualifier
does not seem a good idea as it would break backward compatibility.
A better solution IMO would be to add an extra qualifier, that could
be named, for example, "lite". A field definition without a qualifier
would be considered to have the default "lite" qualifier, and a "lite"
field would be exactly the same as currently a field without a
qualifier (i. e. always sent on transmission and not causing a
exception if not present on reception for C++, and whatever default
behaviour is currently used for the other supported languages).
Obviously this would require code changes to the lexical analyser,
parser and code generators for all the supported languages, so the
question could be asked if the benefits justify the cost.
Alternatively the current behaviour could just be explained on the
wiki with 99.99% less effort!
What do you think?
Re: asymmetrical __isset
Posted by Piscium <gr...@gmail.com>.
On 5 April 2012 16:22, Rush Manbert <ru...@manbert.com> wrote:
> It certainly is very permissive on the client side as far as caring whether you really populate a required or "half-way" field, and I would suspect that minimal generated code is part of the reason. Facebook uses this internally, and they are very concerned with performance.
>
> Also, I think this helps with mutation of the struct definitions over time. If I have a server deployed that knows about your CountrySoup definition above, but I talk to it from a client that has this definition:
>
> struct CountrySoup {
> 1: optional i32 npotatoes,
> // nonions has been removed
> 3: required i32 cabbages,
> 4: i32 cupsOfWater,
> }
>
> then the server read routine will never see the Thrift ID value 2 when it receives a CountrySoup message, so in his CountrySoup struct, the __isset.nonions will be false, and since he doesn't have a case for ID value 4, he just ignores the cupsOfWater field. This way clients and servers of different versions can communicate, whether the combination really works or not. It's up to the server implementation to plan for this case and decide what to do if CountrySoup.__isset.nonions is false.
That makes sense. Thanks again, Rush.
Re: asymmetrical __isset
Posted by Rush Manbert <ru...@manbert.com>.
On Apr 5, 2012, at 8:00 AM, Piscium wrote:
> On 5 April 2012 15:05, Rush Manbert <ru...@manbert.com> wrote:
>
>> Hi Piscium,
>>
>> I'm using an older version of Thrift, but my code is all C++.
>>
>> Try it with your Thrift IDL written like this and I think you'll see what I meant:
>>
>>> ================
>>> namespace cpp test
>>>
>>> struct Test {
>>> 1: optional i32 npotatoes, // optional field because it has the optional keyword
>> 2: i32 nonions, // required field (but will be sent regardless of the __isset state)
>>> }
>
> Thanks, Rush. You are right, an optional field is sent only if __isset is set.
>
> But there is more. I realise now that there are actually three types
> of fields: optional, required and "half-way" (when not explicitly
> marked as either optional or required). "half-way" fields are a bit
> like half-pregnant, somewhere in-between optional and required!
>
> A "half-way" field is like a required field in that it is always sent
> on transmission, and it is like an optional field in that it does not
> cause an exception if not present on reception.
>
> For obvious reasons a "half-way" field is not symmetrical. Moreover a
> required field is not symmetrical either, because there is no
> exception on transmission if it is not set. The apparent conclusion is
> that the only type of field that is symmetrical is the optional.
>
> Still I wonder why it was designed this way. If I compare Thrift
> generated files with those created by Protobuf, it becomes immediately
> obvious how much smaller is the Thrift generated code. Maybe that is
> the reason why it was named "Thrift", because it is parsimonious! So
> maybe it was designed this way to keep the generated code to the bare
> minimum (for example, by not causing an exception on transmission if
> the field is not set, as that would require one check of variable
> every write).
>
> Below is the modified Thrift file and generated code.
>
> =========================
>
> namespace cpp test
>
> struct CountrySoup {
> 1: optional i32 npotatoes,
> 2: i32 nonions, // "half-way" field - neither required nor optional
> 3: required i32 cabbages,
> }
>
> ==================
>
<snip>
It certainly is very permissive on the client side as far as caring whether you really populate a required or "half-way" field, and I would suspect that minimal generated code is part of the reason. Facebook uses this internally, and they are very concerned with performance.
Also, I think this helps with mutation of the struct definitions over time. If I have a server deployed that knows about your CountrySoup definition above, but I talk to it from a client that has this definition:
struct CountrySoup {
1: optional i32 npotatoes,
// nonions has been removed
3: required i32 cabbages,
4: i32 cupsOfWater,
}
then the server read routine will never see the Thrift ID value 2 when it receives a CountrySoup message, so in his CountrySoup struct, the __isset.nonions will be false, and since he doesn't have a case for ID value 4, he just ignores the cupsOfWater field. This way clients and servers of different versions can communicate, whether the combination really works or not. It's up to the server implementation to plan for this case and decide what to do if CountrySoup.__isset.nonions is false.
Best regards,
Rush
Re: asymmetrical __isset
Posted by Mark Slee <ms...@fb.com>.
>> Still I wonder why it was designed this way. If I compare Thrift
>> generated files with those created by Protobuf, it becomes immediately
>> obvious how much smaller is the Thrift generated code. Maybe that is
>> the reason why it was named "Thrift", because it is parsimonious! So
>> maybe it was designed this way to keep the generated code to the bare
>> minimum (for example, by not causing an exception on transmission if
>> the field is not set, as that would require one check of variable
>> every write).
Your technical analysis is correct, as well as the historical inference!
The "required" and "optional" modifiers were not present in the first
version of the IDL, and the original intent was to make using Thrift as
lightweight and transparent as possible. There was an assumption that many
clients would be written in scripting languages, where wrapping everything
up in setters/getters would feel unnatural - we didn't want to make
modifying fields require a function call (via a setter) or require the
programmer to manually maintain all the isset flags. So all fields would
be sent regardless of isset, with one important exception - languages with
nil/null don't send empty fields.
The required/optional keywords were added later to make it easier to
automate some of this checking, and to make it possible for clients in
null-less languages (i.e. C++) to explicitly omit optional fields.
The downside to this approach is that it does make the default behavior
more opaque. Hope that sheds some light on it.
On 4/5/12 8:00 AM, "Piscium" <gr...@gmail.com> wrote:
>On 5 April 2012 15:05, Rush Manbert <ru...@manbert.com> wrote:
>
>> Hi Piscium,
>>
>> I'm using an older version of Thrift, but my code is all C++.
>>
>> Try it with your Thrift IDL written like this and I think you'll see
>>what I meant:
>>
>>> ================
>>> namespace cpp test
>>>
>>> struct Test {
>>> 1: optional i32 npotatoes, // optional field because it has the
>>>optional keyword
>> 2: i32 nonions, // required field (but will be sent
>>regardless of the __isset state)
>>> }
>
>Thanks, Rush. You are right, an optional field is sent only if __isset is
>set.
>
>But there is more. I realise now that there are actually three types
>of fields: optional, required and "half-way" (when not explicitly
>marked as either optional or required). "half-way" fields are a bit
>like half-pregnant, somewhere in-between optional and required!
>
>A "half-way" field is like a required field in that it is always sent
>on transmission, and it is like an optional field in that it does not
>cause an exception if not present on reception.
>
>For obvious reasons a "half-way" field is not symmetrical. Moreover a
>required field is not symmetrical either, because there is no
>exception on transmission if it is not set. The apparent conclusion is
>that the only type of field that is symmetrical is the optional.
>
>Still I wonder why it was designed this way. If I compare Thrift
>generated files with those created by Protobuf, it becomes immediately
>obvious how much smaller is the Thrift generated code. Maybe that is
>the reason why it was named "Thrift", because it is parsimonious! So
>maybe it was designed this way to keep the generated code to the bare
>minimum (for example, by not causing an exception on transmission if
>the field is not set, as that would require one check of variable
>every write).
>
>Below is the modified Thrift file and generated code.
>
>=========================
>
>namespace cpp test
>
>struct CountrySoup {
> 1: optional i32 npotatoes,
> 2: i32 nonions, // "half-way" field - neither required nor optional
> 3: required i32 cabbages,
>}
>
>==================
>
>
>namespace test {
>
>const char* CountrySoup::ascii_fingerprint =
>"A5D9FD141E77B608AA21FD4BD8487AC1";
>const uint8_t CountrySoup::binary_fingerprint[16] =
>{0xA5,0xD9,0xFD,0x14,0x1E,0x77,0xB6,0x08,0xAA,0x21,0xFD,0x4B,0xD8,0x48,0x7
>A,0xC1};
>
>uint32_t CountrySoup::read(::apache::thrift::protocol::TProtocol* iprot) {
>
> uint32_t xfer = 0;
> std::string fname;
> ::apache::thrift::protocol::TType ftype;
> int16_t fid;
>
> xfer += iprot->readStructBegin(fname);
>
> using ::apache::thrift::protocol::TProtocolException;
>
> bool isset_cabbages = false;
>
> while (true)
> {
> xfer += iprot->readFieldBegin(fname, ftype, fid);
> if (ftype == ::apache::thrift::protocol::T_STOP) {
> break;
> }
> switch (fid)
> {
> case 1:
> if (ftype == ::apache::thrift::protocol::T_I32) {
> xfer += iprot->readI32(this->npotatoes);
> this->__isset.npotatoes = true;
> } else {
> xfer += iprot->skip(ftype);
> }
> break;
> case 2:
> if (ftype == ::apache::thrift::protocol::T_I32) {
> xfer += iprot->readI32(this->nonions);
> this->__isset.nonions = true;
> } else {
> xfer += iprot->skip(ftype);
> }
> break;
> case 3:
> if (ftype == ::apache::thrift::protocol::T_I32) {
> xfer += iprot->readI32(this->cabbages);
> isset_cabbages = true;
> } else {
> xfer += iprot->skip(ftype);
> }
> break;
> default:
> xfer += iprot->skip(ftype);
> break;
> }
> xfer += iprot->readFieldEnd();
> }
>
> xfer += iprot->readStructEnd();
>
> if (!isset_cabbages)
> throw TProtocolException(TProtocolException::INVALID_DATA);
> return xfer;
>}
>
>// ===========
>
>uint32_t CountrySoup::write(::apache::thrift::protocol::TProtocol*
>oprot) const {
> uint32_t xfer = 0;
> xfer += oprot->writeStructBegin("CountrySoup");
> if (this->__isset.npotatoes) {
> xfer += oprot->writeFieldBegin("npotatoes",
>::apache::thrift::protocol::T_I32, 1);
> xfer += oprot->writeI32(this->npotatoes);
> xfer += oprot->writeFieldEnd();
> }
> xfer += oprot->writeFieldBegin("nonions",
>::apache::thrift::protocol::T_I32, 2);
> xfer += oprot->writeI32(this->nonions);
> xfer += oprot->writeFieldEnd();
> xfer += oprot->writeFieldBegin("cabbages",
>::apache::thrift::protocol::T_I32, 3);
> xfer += oprot->writeI32(this->cabbages);
> xfer += oprot->writeFieldEnd();
> xfer += oprot->writeFieldStop();
> xfer += oprot->writeStructEnd();
> return xfer;
>}
>
>} // namespace
Re: asymmetrical __isset
Posted by Piscium <gr...@gmail.com>.
On 5 April 2012 15:05, Rush Manbert <ru...@manbert.com> wrote:
> Hi Piscium,
>
> I'm using an older version of Thrift, but my code is all C++.
>
> Try it with your Thrift IDL written like this and I think you'll see what I meant:
>
>> ================
>> namespace cpp test
>>
>> struct Test {
>> 1: optional i32 npotatoes, // optional field because it has the optional keyword
> 2: i32 nonions, // required field (but will be sent regardless of the __isset state)
>> }
Thanks, Rush. You are right, an optional field is sent only if __isset is set.
But there is more. I realise now that there are actually three types
of fields: optional, required and "half-way" (when not explicitly
marked as either optional or required). "half-way" fields are a bit
like half-pregnant, somewhere in-between optional and required!
A "half-way" field is like a required field in that it is always sent
on transmission, and it is like an optional field in that it does not
cause an exception if not present on reception.
For obvious reasons a "half-way" field is not symmetrical. Moreover a
required field is not symmetrical either, because there is no
exception on transmission if it is not set. The apparent conclusion is
that the only type of field that is symmetrical is the optional.
Still I wonder why it was designed this way. If I compare Thrift
generated files with those created by Protobuf, it becomes immediately
obvious how much smaller is the Thrift generated code. Maybe that is
the reason why it was named "Thrift", because it is parsimonious! So
maybe it was designed this way to keep the generated code to the bare
minimum (for example, by not causing an exception on transmission if
the field is not set, as that would require one check of variable
every write).
Below is the modified Thrift file and generated code.
=========================
namespace cpp test
struct CountrySoup {
1: optional i32 npotatoes,
2: i32 nonions, // "half-way" field - neither required nor optional
3: required i32 cabbages,
}
==================
namespace test {
const char* CountrySoup::ascii_fingerprint = "A5D9FD141E77B608AA21FD4BD8487AC1";
const uint8_t CountrySoup::binary_fingerprint[16] =
{0xA5,0xD9,0xFD,0x14,0x1E,0x77,0xB6,0x08,0xAA,0x21,0xFD,0x4B,0xD8,0x48,0x7A,0xC1};
uint32_t CountrySoup::read(::apache::thrift::protocol::TProtocol* iprot) {
uint32_t xfer = 0;
std::string fname;
::apache::thrift::protocol::TType ftype;
int16_t fid;
xfer += iprot->readStructBegin(fname);
using ::apache::thrift::protocol::TProtocolException;
bool isset_cabbages = false;
while (true)
{
xfer += iprot->readFieldBegin(fname, ftype, fid);
if (ftype == ::apache::thrift::protocol::T_STOP) {
break;
}
switch (fid)
{
case 1:
if (ftype == ::apache::thrift::protocol::T_I32) {
xfer += iprot->readI32(this->npotatoes);
this->__isset.npotatoes = true;
} else {
xfer += iprot->skip(ftype);
}
break;
case 2:
if (ftype == ::apache::thrift::protocol::T_I32) {
xfer += iprot->readI32(this->nonions);
this->__isset.nonions = true;
} else {
xfer += iprot->skip(ftype);
}
break;
case 3:
if (ftype == ::apache::thrift::protocol::T_I32) {
xfer += iprot->readI32(this->cabbages);
isset_cabbages = true;
} else {
xfer += iprot->skip(ftype);
}
break;
default:
xfer += iprot->skip(ftype);
break;
}
xfer += iprot->readFieldEnd();
}
xfer += iprot->readStructEnd();
if (!isset_cabbages)
throw TProtocolException(TProtocolException::INVALID_DATA);
return xfer;
}
// ===========
uint32_t CountrySoup::write(::apache::thrift::protocol::TProtocol*
oprot) const {
uint32_t xfer = 0;
xfer += oprot->writeStructBegin("CountrySoup");
if (this->__isset.npotatoes) {
xfer += oprot->writeFieldBegin("npotatoes",
::apache::thrift::protocol::T_I32, 1);
xfer += oprot->writeI32(this->npotatoes);
xfer += oprot->writeFieldEnd();
}
xfer += oprot->writeFieldBegin("nonions",
::apache::thrift::protocol::T_I32, 2);
xfer += oprot->writeI32(this->nonions);
xfer += oprot->writeFieldEnd();
xfer += oprot->writeFieldBegin("cabbages",
::apache::thrift::protocol::T_I32, 3);
xfer += oprot->writeI32(this->cabbages);
xfer += oprot->writeFieldEnd();
xfer += oprot->writeFieldStop();
xfer += oprot->writeStructEnd();
return xfer;
}
} // namespace
Re: asymmetrical __isset
Posted by Rush Manbert <ru...@manbert.com>.
On Apr 5, 2012, at 6:27 AM, Piscium wrote:
> On 5 April 2012 01:02, Rush Manbert <ru...@manbert.com> wrote:
>>
>> On Apr 2, 2012, at 12:47 AM, Piscium wrote:
>>
>>> On C++ generated code, __isset is set to true in the read function
>>> for the fields that have been received, however it is not looked at at
>>> all by the write function which always writes all fields regardless of
>>> the __isset value.
>>>
>>> What is the reasoning behind this asymmetry? Is __isset intended only
>>> to cater for the receiving side, in particular for the use case in
>>> which the receiver is of a different software version than the sender?
>
>> I don't think this is quite true. All required fields are always written, regardless of the state of their __isset. Optional fields are only written if their __isset is true. The read side always sets the __isset for everything it receives, but it doesn't receive the fields that are optional and didn't have __isset true when written.
>
> Hi Rush,
>
> I described what I found out in my tests. I only tested with Thrift v.
> 0.8.0, and only with C++ (things may be different with other Thrift
> versions or with Java or Python, I wouldn't know).
>
> Anyway please see this minimal example (with comments):
>
> ================
> namespace cpp test
>
> struct Test {
> 1: i32 npotatoes, // optional field
> }
>
> ================
>
> This is the generated file (with some extra comments added by myself):
>
>
> uint32_t Test::write(::apache::thrift::protocol::TProtocol* oprot) const {
> uint32_t xfer = 0;
> xfer += oprot->writeStructBegin("Test");
> xfer += oprot->writeFieldBegin("npotatoes",
> ::apache::thrift::protocol::T_I32, 1);
> xfer += oprot->writeI32(this->npotatoes); // <<<<<<<< This optional
> field is always written, regardless of __isset
> xfer += oprot->writeFieldEnd();
> xfer += oprot->writeFieldStop();
> xfer += oprot->writeStructEnd();
> return xfer;
>
> =====
>
> uint32_t Test::read(::apache::thrift::protocol::TProtocol* iprot) {
> //snip
>
> switch (fid)
> {
> case 1:
> if (ftype == ::apache::thrift::protocol::T_I32) {
> xfer += iprot->readI32(this->npotatoes); // <<<<<<<< This
> optional field is always read, regardless of __isset
> // As __isset is not sent over the wire, it makes sense for
> the read function to set it when received
> this->__isset.npotatos = true;
> } else {
> xfer += iprot->skip(ftype);
> }
> break;
> default:
> xfer += iprot->skip(ftype);
> break;
> }
>
> //snip
Hi Piscium,
I'm using an older version of Thrift, but my code is all C++.
Try it with your Thrift IDL written like this and I think you'll see what I meant:
> ================
> namespace cpp test
>
> struct Test {
> 1: optional i32 npotatoes, // optional field because it has the optional keyword
2: i32 nonions, // required field (but will be sent regardless of the __isset state)
> }
>
- Rush
Re: asymmetrical __isset
Posted by Piscium <gr...@gmail.com>.
On 5 April 2012 01:02, Rush Manbert <ru...@manbert.com> wrote:
>
> On Apr 2, 2012, at 12:47 AM, Piscium wrote:
>
>> On C++ generated code, __isset is set to true in the read function
>> for the fields that have been received, however it is not looked at at
>> all by the write function which always writes all fields regardless of
>> the __isset value.
>>
>> What is the reasoning behind this asymmetry? Is __isset intended only
>> to cater for the receiving side, in particular for the use case in
>> which the receiver is of a different software version than the sender?
> I don't think this is quite true. All required fields are always written, regardless of the state of their __isset. Optional fields are only written if their __isset is true. The read side always sets the __isset for everything it receives, but it doesn't receive the fields that are optional and didn't have __isset true when written.
Hi Rush,
I described what I found out in my tests. I only tested with Thrift v.
0.8.0, and only with C++ (things may be different with other Thrift
versions or with Java or Python, I wouldn't know).
Anyway please see this minimal example (with comments):
================
namespace cpp test
struct Test {
1: i32 npotatoes, // optional field
}
================
This is the generated file (with some extra comments added by myself):
uint32_t Test::write(::apache::thrift::protocol::TProtocol* oprot) const {
uint32_t xfer = 0;
xfer += oprot->writeStructBegin("Test");
xfer += oprot->writeFieldBegin("npotatoes",
::apache::thrift::protocol::T_I32, 1);
xfer += oprot->writeI32(this->npotatoes); // <<<<<<<< This optional
field is always written, regardless of __isset
xfer += oprot->writeFieldEnd();
xfer += oprot->writeFieldStop();
xfer += oprot->writeStructEnd();
return xfer;
=====
uint32_t Test::read(::apache::thrift::protocol::TProtocol* iprot) {
//snip
switch (fid)
{
case 1:
if (ftype == ::apache::thrift::protocol::T_I32) {
xfer += iprot->readI32(this->npotatoes); // <<<<<<<< This
optional field is always read, regardless of __isset
// As __isset is not sent over the wire, it makes sense for
the read function to set it when received
this->__isset.npotatos = true;
} else {
xfer += iprot->skip(ftype);
}
break;
default:
xfer += iprot->skip(ftype);
break;
}
//snip
Re: asymmetrical __isset
Posted by Rush Manbert <ru...@manbert.com>.
On Apr 2, 2012, at 12:47 AM, Piscium wrote:
> On C++ generated code, __isset is set to true in the read function
> for the fields that have been received, however it is not looked at at
> all by the write function which always writes all fields regardless of
> the __isset value.
>
> What is the reasoning behind this asymmetry? Is __isset intended only
> to cater for the receiving side, in particular for the use case in
> which the receiver is of a different software version than the sender?
Anyone else feel free to correct me...
I don't think this is quite true. All required fields are always written, regardless of the state of their __isset. Optional fields are only written if their __isset is true. The read side always sets the __isset for everything it receives, but it doesn't receive the fields that are optional and didn't have __isset true when written.
So __isset on the read side of the wire really only means that an optional field is present.
We use Thrift classes in other ways that don't necessarily result in them being sent over the wire. In those cases our code generally requires that the __isset be true for all required fields, and we have a macro that both sets a field and sets its __isset true. This is designed to detect when someone forgot to initialize a required field. If the __isset is true, then we feel safe in assuming that the data value was written intentionally. (or is empty intentionally if it is a string, etc.)
- Rush