New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue decoding after migrating from 4.x to 6.6.3 #670

Closed
johnnyreilly opened this Issue Feb 1, 2017 · 22 comments

Comments

Projects
None yet
2 participants
@johnnyreilly

johnnyreilly commented Feb 1, 2017

protobuf.js version: 6.6.3

Hey it's me again 馃槈

I've just attempted an upgrade from v4 to v6. With v4 I was generating js files like so:

pbjs ./proto/bcl.proto ./proto/MonitoringClientMessage.proto ./proto/MonitoringServerMessage.proto --target commonjs > ./proto/proto.js

With v6 I'm doing it like so: (I've also attempted with commonjs as well)

pbjs --target static-module --wrap es6 --out ./proto/proto6.js ./proto/bcl.proto ./proto/MonitoringClientMessage.proto ./proto/MonitoringServerMessage.proto
pbts --out ./proto/proto6.d.ts ./proto/proto6.js'

However, decoding always fails for me. Specifically, when I debug Reader.prototype.uint32 always produces 0:

/**
 * Reads a varint as an unsigned 32 bit value.
 * @function
 * @returns {number} Value read
 */
Reader.prototype.uint32 = (function read_uint32_setup() {
    var value = 4294967295; // optimizer type-hint, tends to deopt otherwise (?!)
    return function read_uint32() {
        value = (         this.buf[this.pos] & 127       ) >>> 0; if (this.buf[this.pos++] < 128) return value;
        value = (value | (this.buf[this.pos] & 127) <<  7) >>> 0; if (this.buf[this.pos++] < 128) return value;
        value = (value | (this.buf[this.pos] & 127) << 14) >>> 0; if (this.buf[this.pos++] < 128) return value;
        value = (value | (this.buf[this.pos] & 127) << 21) >>> 0; if (this.buf[this.pos++] < 128) return value;
        value = (value | (this.buf[this.pos] &  15) << 28) >>> 0; if (this.buf[this.pos++] < 128) return value;

        /* istanbul ignore next */
        if ((this.pos += 5) > this.len) {
            this.pos = this.len;
            throw indexOutOfRange(this, 10);
        }
        return value;
    };
})();

It's rather strange and so I wanted to report in case I'm doing anything dozy... If there's any ideas I'd greatly appreciate them!

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Feb 1, 2017

Do you have an example buffer / proto at hand?

@johnnyreilly

This comment has been minimized.

johnnyreilly commented Feb 1, 2017

@johnnyreilly

This comment has been minimized.

johnnyreilly commented Feb 1, 2017

Actually there's nothing that I'm allowed to share.... Frustrating. I'll see if I can get a repro with one of the examples and share that.

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Feb 1, 2017

There is also a little howto on how to reverse engineer a buffer by hand in order to spot the underlying issue.

@johnnyreilly

This comment has been minimized.

johnnyreilly commented Feb 1, 2017

Thanks I'll take a look!

@johnnyreilly

This comment has been minimized.

johnnyreilly commented Feb 1, 2017

I'm not at a keyboard and so can't test but I just noticed the presence of a --keep-case option. I'm relying on the original (not camel) case for my app and I haven't used it in my new generation. Is that something worth checking out? I can't see it listed in breaking changes since v4 so perhaps not?

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Feb 1, 2017

@johnnyreilly

This comment has been minimized.

johnnyreilly commented Feb 1, 2017

So worth trying? Has the camelCase thing only be around since v6 then?

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Feb 1, 2017

v5 had a global option. v6 uses camel case by default, which can still be overridden with { keepCase: true }.

@johnnyreilly

This comment has been minimized.

johnnyreilly commented Feb 1, 2017

And --keep-case if using pbjs presumably? Great I'll give it a try in the morning. Thanks!

@dcodeIO dcodeIO added the question label Feb 1, 2017

@johnnyreilly

This comment has been minimized.

johnnyreilly commented Feb 2, 2017

Nope --keep-case wasn't the issue

@johnnyreilly

This comment has been minimized.

johnnyreilly commented Feb 2, 2017

OK - it seems likely that the problem lies with ArrayBuffer. My project is using ArrayBuffer from the babel polyfill - and it seems there might be some issues with that:

babel/babel#5140

Namely I can't actually read from the ArrayBuffer and it doesn't have the API you would expect. (It lacks a length property for instance)

@johnnyreilly

This comment has been minimized.

johnnyreilly commented Feb 2, 2017

OK. It's a little strange but I have a workaround. I take the data I receive from the websocket and treat the supplying ArrayBuffer as if it is diseased. I rob of its data and leave it breathing its last on the pavement:

    const clonedData = new Uint8Array(data);

This cloned array will give me what I need. Very strange but I thought it was probably worth sharing...

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Feb 2, 2017

Yeah, using Uint8Array in the browser / Buffer under node.js is correct. ArrayBuffer alone does not provide ways to manipulate the data (at least not within all environments).

Uint8Array is not cloning, by the way, it is creating a view on the raw binary data.

From MDN:

The ArrayBuffer object is used to represent a generic, fixed-length raw binary data buffer. You cannot directly manipulate the contents of an ArrayBuffer; instead, you create one of the typed array objects or a DataView object which represents the buffer in a specific format, and use that to read and write the contents of the buffer.

@johnnyreilly

This comment has been minimized.

johnnyreilly commented Feb 2, 2017

Thanks for the clarification - I'd missed that

@johnnyreilly

This comment has been minimized.

johnnyreilly commented Feb 2, 2017

Is it worth mentioning somewhere in the docs that users might need this approach if they are consuming ArrayBuffers?

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Feb 2, 2017

The API documentation always refers to Uint8Array respectively Buffer. It's not mentioned in the README, though, hmm.

dcodeIO added a commit that referenced this issue Feb 2, 2017

@johnnyreilly

This comment has been minimized.

johnnyreilly commented Feb 2, 2017

I was actually more thinking about the decode side of things; a user may be handed an ArrayBuffer via a websocket without realising that they need to Uint8Array-ify things before they can decode. That was my scenario.

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Feb 2, 2017

Yeah, added that to the first example in README. It's intentional that it doesn't accept an ArrayBuffer for perf reasons (avoids the additional assertion).

@johnnyreilly

This comment has been minimized.

johnnyreilly commented Feb 2, 2017

Oh I saw - my point was more that people may not realise that they have an ArrayBuffer on their hands that they need to transform. I didn't and I'd say it's not obvious..

@johnnyreilly

This comment has been minimized.

johnnyreilly commented Feb 3, 2017

Actually it's covered here: https://github.com/dcodeIO/protobuf.js/wiki/How-to-read-binary-data-in-the-browser-or-under-node.js%3F

I wonder if this section merits promotion to the readme? Or a link off to it perhaps?

@johnnyreilly

This comment has been minimized.

johnnyreilly commented Feb 6, 2017

Closing as all my issues are resolved; thanks for your help @dcodeIO!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment