6
\$\begingroup\$

I'm working with one environment which allows me to execute node.js code from single file without installing additional modules. My task is to write simple ssl socket client which will send protobuf message, receive response in same format and decode it.

What I developed in this code example:

  1. protobuf decoder/encoder (I've implemented only types I'll use);
  2. ssl socket class;

I'm very much a newbie in Node.js (6 hours experience :D), so I need you to point me weak parts of this code and share with me best practices.

const tls = require("tls");

const HOST = "example.com";
const PORT = 12345;

const ProtoTypes = Object.freeze({"Varint": 0, "_64bit": 1, "LengthDelimited": 2, "_32bit": 5});

class ProtoField {
    constructor (type, tag, value=null) {
        if (isNaN(tag) || tag < 0)
            throw new Error("Invalid tag");

        this.type = type;
        this.tag = tag;
        this.value = value;
    }

    encodePrefix() {
        return VarintField.encode((this.tag << 3) | this.type);
    }

    static decodePrefix(stream, position=0) {
        var decoded_prefix = VarintField.decode(stream, position);
        return {
            type: decoded_prefix.value & 7,
            tag: decoded_prefix.value >> 3,
            length: decoded_prefix.length
        }
    }

    encode() {
        var prefix = this.encodePrefix();

        return Buffer.concat([prefix, this.constructor.encode(this.value)]);
    }

    decode(stream, position=0) {
        var decoded = this.constructor.decode(stream, position);
        this.value = decoded.value;

        return decoded.length;
    }
}

class VarintField extends ProtoField {
    constructor(tag, value=null) {
        if (value != null && isNaN(value))
            throw new Error("Invalid value");
        
        super(ProtoTypes.Varint, tag, value);
    }

    static encode(value) {
        var number = value, buf = [];

        for (;;) {
            var toWrite = number & 0x7F;
            number >>= 7;
            if (number != 0)
                buf.push(toWrite | 0x80);
            else {
                buf.push(toWrite);
                break;
            }
        }

        return Buffer.from(buf);
    }

    static decode(stream, position=0) {
        var number =  0, bytes_read = 0;

        for (var shift = 0; position + bytes_read < stream.length; bytes_read++, shift += 7) {
            var b = stream[position + bytes_read];
            number |= (b & 0x7f) << shift;
            if ((b & 0x80) == 0) {
                bytes_read += 1;
                break;
            }
        }

        return {
            value: number,
            length: bytes_read
        }
    }
}

class BytesField extends ProtoField {
    constructor(tag, value=null) {
        if (value != null && !Buffer.isBuffer(value))
            throw new Error("Invalid value");
        
        super(ProtoTypes.LengthDelimited, tag, value);
    }

    static encode(value) {
        return Buffer.concat([VarintField.encode(value.length), Buffer.from(value)]);
    }

    static decode(stream, position=0) {
        var length = VarintField.decode(stream, position);
        return {
            value: stream.slice(position + length.length, position + length.length + length.value),
            length: length.length + length.value
        }
    }
}

class StringField extends ProtoField {
    constructor(tag, value=null) {
        if (value != null && String(value) !== value)
            throw new Error("Invalid value");
        
        super(ProtoTypes.LengthDelimited, tag, value);
    }

    static encode(value) {
        return Buffer.concat([VarintField.encode(value.length), Buffer.from(value, "utf-8")]);
    }

    static decode(stream, position=0) {
        var length = VarintField.decode(stream, position);
        return {
            value: stream.slice(position + length.length, position + length.length + length.value).toString("utf-8"),
            length: length.length + length.value
        }
    }
}

class _32bitField {
    static decode(stream, position=0) {
        return {value: null, length: 4};
    }
}

class _64bitField {
    static decode(stream, position=0) {
        return {value: null, length: 8};
    }
}

const ProtoTypesDefaultClasses = Object.freeze({
    [ProtoTypes.Varint]: VarintField,
    [ProtoTypes.LengthDelimited]: BytesField,
    [ProtoTypes._32bit]: _32bitField,
    [ProtoTypes._64bit]: _64bitField
});

class Message {
    constructor() {
        this.fields = [];
    }

    addField(field) {
        this.fields[field.tag] = field;
        return field;
    }

    encode() {
        var buffers = [];
        this.fields.forEach(function(field) {
            if (field.value != null)
                buffers.push(field.encode());
        });
        return Buffer.concat(buffers);
    }

    decode(stream, position=0, length=0) {
        var bytes_read = 0, length = length > 0 ? length : stream.length - position;

        for (; bytes_read < length;) {
            var prefix = ProtoField.decodePrefix(stream, position + bytes_read);
            if (this.fields[prefix.tag] != undefined)
                bytes_read += this.fields[prefix.tag].decode(stream, position + bytes_read + prefix.length) + prefix.length;
            else
                bytes_read += ProtoTypesDefaultClasses[prefix.type].decode(stream, position + bytes_read + prefix.length).length + prefix.length;
        }

        return bytes_read;
    } 
}

class EmbeddedMessageField extends ProtoField {
    constructor(tag, value) {
        if (!(value instanceof Message))
            throw new Error("Invalid value");
        
        super(ProtoTypes.LengthDelimited, tag, value);
    }

    static encode(value) {
        return BytesField.encode(value.encode());
    }

    decode(stream, position=0) {
        var length = VarintField.decode(stream, position);

        return length.length + this.value.decode(stream, position + length .length, length.value)
    }
}

class ExampleEmbededMessage extends Message {
    constructor(f1, f2, f3) {
        super();

        this.e_field1 = this.addField(new BytesField(1, f1));
        this.e_field2 = this.addField(new VarintField(2, f2));
        this.e_field3 = this.addField(new StringField(4, f3));
    }
}

class ExampleRequestMessage extends Message {
    constructor(f0, f1, f2, f3) {
        super();

        this.field1 = this.addField(new StringField(1, f0));
        this.embedded = this.addField(new EmbeddedMessageField(2, new ExampleEmbededMessage(f1, f2, f3)));
        this.constant_field = this.addField(new StringField(3, "constant"));
    }
}

class ExampleResponseMessage extends Message {
    constructor() {
        super();

        this.resp_field1 = this.addField(new StringField(1));
        this.resp_field2 = this.addField(new VarintField(2));
        this.resp_field15 = this.addField(new StringField(15));
    }
}

class Packet {
    constructor(id, message) {
        this.id = id;
        this.message = message;
    }

    encode() {
        return Buffer.concat([Buffer.from([this.id]), this.message.encode()]);
    }

    decode(stream, position=0) {
        if (stream.length == 0)
            return 0;
        this.id = stream[position];
        return this.message.decode(stream, position + 1) + 1;
    }
}

class TLSClient {
    constructor(host, port) {
        this.host = host;
        this.port = port;
        this.options = {rejectUnauthorized: false};
        this.init()
    }

    init() {
        var client = this;
        this.socket = tls.connect(client.port, client.host, client.options);
    }

    send(packet) {
        var client = this;
        return new Promise((resolve, reject) => {
            var encoded_packet = packet.encode();
            var length = Buffer.alloc(4);
            length.writeUInt32LE(encoded_packet.length);

            client.socket.write(Buffer.concat([length, encoded_packet]));

            client.socket.on("data", (data) => {
                resolve(data);
                client.socket.destroy();
            });
            client.socket.on("error", (err) => {
                reject(err);
            });
        });
    }
}

var f0 = "string1", f1 = Buffer.from([0, 1, 2, 3, 4, 5]), f2 = 54321, f3 = "string2";
var request_packet = new Packet(13, new ExampleRequestMessage(f0, f1, f2, f3));
var response_packet = new Packet(13, new ExampleResponseMessage());

var sock = new TLSClient(HOST, PORT);

sock.send(request_packet)
.then((data) => {
    response_packet.decode(data, 4);
    if (response_packet.resp_field2.value > 0)
        console.log("Great!");
    else
        console.log("Zero.");
})
.catch((err) => {
    console.log(err);
});

So, after some time passed I see no answers. I think, I should ask more concrete questions:

  1. Is Object.freeze() the normal way to implement enum logic in JavaScript?
  2. Is it okay to give same names for method and static method?
  3. Is instance.constructor.static_method() the normal way to access to a static method?
  4. Is the way i use Promise correct?
  5. Is naming style I used for classes, variables, constants and methods correct?
  6. Are there any logical mistakes in OOP?
\$\endgroup\$
1
  • 2
    \$\begingroup\$ Welcome! I am looking forward to the review of this. \$\endgroup\$
    – konijn
    Nov 1, 2019 at 18:47

1 Answer 1

1
\$\begingroup\$

Overall Remarks

I must admit that this code makes use of more bitwise operators than I normally see in JavaScript. Nonetheless it looks to be sophisticated. There is a lot of code so the remarks here may not be comprehensive but I’ll cover what I can.

Question Responses

  1. Is Object.freeze() the normal way to implement enum logic in JavaScript?

That seems to be the common convention. Note that "freeze is shallow"1 so if there were nested objects then it would likely by obligatory to freeze those as well. Refer to answers to this Stack Overflow question: What is the preferred syntax for defining enums in JavaScript?. For even more light reading take a glance at this article posted by one of the users who supplied an answer to that aforementioned SO question.

  1. Is it okay to give same names for method and static method?

While it was closed as off-topic that question was asked on Stack Overflow as well.

  1. Is instance.constructor.static_method() the normal way to access to a static method?

I don’t want to sound like a broken record but there’s a SO question for that. That was works unless the goal is to allow an override static method to be used in a sub-class.

  1. Is the way i use Promise correct?

I am not sure if it is “correct” but as far as I can tell that seems to match typical usage I have seen.

  1. Is naming style I used for classes, variables, constants and methods correct?

Again I'm not sure what "correct" is but we can say it is idiomatic - it appears to follow standard conventions (e.g. style guides like AirBnB, Google, etc.)

Suggestions

Variable declaration keywords

There are quite a few lines that use keyword var.

var number =  0, bytes_read = 0;
var toWrite = number & 0x7F;

Many argue that const and let should be used instead of var in ES6 for multiple reasons - e.g. block scoping, unintentional re-assignment with const, etc.

Binding context to this

In TLSClient::send() there is this first line:

var client = this;

that appears to be used within the promise callback - e.g.:

client.socket.write(Buffer.concat([length, encoded_packet]));

But with an arrow function the context is the same... so there is no need to assign the outer context. If it wasn't using an arrow function, the same could be achieved with Function.bind().

Extra lambda functions

In TLSClient::send() there is this declaration:

client.socket.on("error", (err) => {
    reject(err);
});

This could be simplified to:

this.socket.on("error", reject);

because the arguments would get passed directly to the callback.

Bloated for loop

In Message::decode() there is a loop started like this:

for (; bytes_read < length;) {

It could simple be a while loop:

while (bytes_read < length) {
\$\endgroup\$
2
  • \$\begingroup\$ You did the great job. A I said I've never used js seriously before, so I really appreciate that you linked already answered questions with great explanation for such as newbies as I am. Thank you very much, will try to find time on weekends and add to question upgraded version. P.S. (offtopic) Surprisingly, this code is pretty stable and working without any problems for very long time :D \$\endgroup\$ Sep 30, 2020 at 19:14
  • \$\begingroup\$ Great! Please consider posting a new question (linking back to this one as reference) instead of adding the upgraded version. After getting an answer you are not allowed to change your code anymore. This is to ensure that answers do not get invalidated and have to hit a moving target. See the section What should I not do? on What should I do when someone answers my question? for more information. \$\endgroup\$ Sep 30, 2020 at 20:07

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.