[
https://issues.apache.org/jira/browse/THRIFT-2933?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14277300#comment-14277300
]
Randy Abernethy commented on THRIFT-2933:
-----------------------------------------
Hey Tom,
Thanks for the comment. Perhaps we agree conceptually. Here's where I am going
with this:
I would like to see all node protocol code migrated into files something like
this:
- thrift/lib/nodejs/TBinaryProtocol.js
- thrift/lib/nodejs/TCompactProtocol.js
- thrift/lib/nodejs/TJSONProtocol.js
with protocol.js as a pass through requirer for compatibility.
and all nodejs transports in files something like this:
- thrift/lib/nodejs/TFramedTransport.js
- thrift/lib/nodejs/TBufferedTransport.js
same story with transport.js (we also need to allow transports to be stacked
like most other language implementations)
So at the end of the day TBinaryProtocol (the object inside protocol.js) and
binary.js (the entire file) will move into TBinaryProtocol.js. This will
provide an implementation pattern and file isolation for those wishing to add
new protocols and transports (TCipherTransport, TSnappyProtocol, etc.).
The code file location and file naming convention above is adhered to by almost
all of the Apache Thrift languages, making Node.js harder to navigate right now
for folks familiar with Thrift. It also artificially intertwines
protocols/transports that really have nothing to do with each other.
Interesting to note that the only call (coupling) TCompactProtocol has to
binary.js is writeDouble(), which is the exact cause of this bug ( ! ). Had I
not tried to reuse and instead focused more on encapsulation this bug would not
have ended up in the code.
Hopefully with this road map the request makes more sense.
-Randy
> v0.9.2: doubles encoded in node with compact protocol cannot be decoded by
> python
> ---------------------------------------------------------------------------------
>
> Key: THRIFT-2933
> URL: https://issues.apache.org/jira/browse/THRIFT-2933
> Project: Thrift
> Issue Type: Bug
> Components: Node.js - Library
> Affects Versions: 0.9.2
> Reporter: Dan Heller
> Attachments: double-endiannes-2.patch, little-endian.patch,
> thrift-double.tgz
>
>
> I've noticed that with Thrift v0.9.2, using the Compact protocol, I can't
> encode doubles in node and decode them in python. It looks like in python,
> we're decoding little-endian for compact and big-endian for binary, and in
> node, writing big-endian in all cases (therefore, Binary works fine). I've
> attached a test program to show this behavior. I have not found the thrift
> binary spec, so I'm not sure which side is wrong.
> {code}
> [~/thrift-double]$ thrift --version
> Thrift version 0.9.2
> [~/thrift-double]$ grep -i thrift package.json
> "thrift": "0.9.2"
> "compile": "thrift --gen js:node --gen py test.thrift"
> [~/thrift-double]$ pip show thrift
> ---
> Name: thrift
> Version: 0.9.2
> Location: /Library/Python/2.7/site-packages
> Requires:
> [~/thrift-double]$ npm run compile
> > [email protected] compile /Users/dh/thrift-double
> > thrift --gen js:node --gen py test.thrift
> [~/thrift-double]$ node test.js binary | python test.py binary
> Trying to send { intField: 100, doubleField: 99.88 }
> Reading with binary protocol.
> Got test object: TestType(intField=100, doubleField=99.88)
> [~/thrift-double]$ node test.js compact | python test.py compact
> Trying to send { intField: 100, doubleField: 99.88 }
> Reading with compact protocol.
> Got test object: TestType(intField=100, doubleField=-2.24248483894553e-38)
> [~/thrift-double]$
> {code}
> I notice the following snippets.
> * TBinaryProtocol.py: network format (big-endian)
> {code}
> def readDouble(self):
> buff = self.trans.readAll(8)
> val, = unpack('!d', buff)
> return val
> {code}
> * TCompactProtocol.py: little-endian
> {code}
> def readDouble(self):
> buff = self.trans.readAll(8)
> val, = unpack('<d', buff)
> return val
> {code}
> * binary.js: my best IEEE floating point days are behind me but this looks
> big-endian. In any case it's used for both compact and binary.
> {code}
> exports.writeDouble = function(buff, v) {
> var m, e, c;
> buff[0] = (v < 0 ? 0x80 : 0x00);
> v = Math.abs(v);
> if (v !== v) {
> // NaN, use QNaN IEEE format
> m = 2251799813685248;
> e = 2047;
> } else if (v === Infinity) {
> m = 0;
> e = 2047;
> } else {
> e = Math.floor(Math.log(v) / Math.LN2);
> c = Math.pow(2, -e);
> if (v * c < 1) {
> e--;
> c *= 2;
> }
> if (e + 1023 >= 2047)
> {
> // Overflow
> m = 0;
> e = 2047;
> }
> else if (e + 1023 >= 1)
> {
> // Normalized - term order matters, as Math.pow(2, 52-e) and
> v*Math.pow(2, 52) can overflow
> m = (v*c-1) * POW_52;
> e += 1023;
> }
> else
> {
> // Denormalized - also catches the '0' case, somewhat by chance
> m = (v * POW_1022) * POW_52;
> e = 0;
> }
> }
> buff[1] = (e << 4) & 0xf0;
> buff[0] |= (e >> 4) & 0x7f;
> buff[7] = m & 0xff;
> m = Math.floor(m / POW_8);
> buff[6] = m & 0xff;
> m = Math.floor(m / POW_8);
> buff[5] = m & 0xff;
> m = Math.floor(m / POW_8);
> buff[4] = m & 0xff;
> m >>= 8;
> buff[3] = m & 0xff;
> m >>= 8;
> buff[2] = m & 0xff;
> m >>= 8;
> buff[1] |= m & 0x0f;
> return buff;
> };
> {code}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
