add property to Object causes runtime error and close connection

[alexaleluia12]

// output
obama
{ Error: read ECONNRESET
at TCP.onread (net.js:660:25)
cause:
{ Error: read ECONNRESET
at TCP.onread (net.js:660:25) errno: 'ECONNRESET', code: 'ECONNRESET', syscall: 'read' },
isOperational: true,
errno: 'ECONNRESET',
code: 'ECONNRESET',
syscall: 'read' }

To remove error just comment prototype definition on other.js.
This is bad because other library can crash the library.
Version: "amqplib": "^0.5.2"

// reproduce

// ===main.js===
const Person = require('./other');
const worker = require('./conn');

worker((msg) => {
    console.log(msg);
});

let p1 = new Person('obama');

console.log(p1.name);

// ===conn.js===
var q = 'teste_queue';

var open = require('amqplib').connect('amqp://localhost');


// Consumer
function work(callback) {
    open.then(function(conn) {
      conn.on('error', (err) => {
        console.error(err);
      })
      conn.on('close', () => {
        console.log('connection close');
      })
      return conn.createChannel();
    }).then(function(ch) {
      return ch.assertQueue(q).then(function(ok) {
        return ch.consume(q, function(msg) {
          if (msg !== null) {
            callback(msg.content.toString());
            ch.ack(msg);
          }
        });
      });
    }).catch(console.warn);
}

module.exports = work;

// ===other.js===

Object.prototype.isEmpty = function() {
     for(var key in this) {
         if(this.hasOwnProperty(key))
             return false;
     }
     return true;
}


class Person {
    constructor(name) {
        this.name = name;
        this.jobs = {};
    }
};

module.exports = Person;

[squaremo]

I can reproduce this. I deliberately included all enumerable properties, so that it was easier to share defaults (as explained in http://www.squaremobius.net/amqp.node/channel_api.html#args under "field table values"). But I can't think of any clever way around this problem :-/

I guess the remedy is to only use own properties when encoding, or perhaps to not include anything that comes from object .. not clever, but might work without breaking existing deployments.

[cressie176]

I've tracked the problem down to how the frame header is parsed. This is done by a library called bitsyntax, which uses a parser generated at build time by the now abandoned PEG.js.

When a property is added to Object.prototype, the PEG generated parser the bitsynax AST assigns invalid values to parsed properties, and consequently misses out important blocks. As a result the frame header attributes are left undefined and node of amqplib's parseFrame function conditional logic is unsatisfied, causing it to always returns false. Ultimately the frame is never decoded and the broker handshake timeout expires causing it to close the socket and resulting in the error reported above.

I think the best path forward is migrate bitsyntax to peggy, which is actively maintained, then if the problem still persists create an issue over there.

[cressie176]

Gets better. The last commit to bitsyntax was to bump pegjs from 0.7.0 to 0.1.0. It was a breaking change, but the bitsyntax code was never updated, so the project is currently broken.

It can be run using ./node_modules/.bin/pegjs -o lib/parser.js lib/grammar.pegjs but the generated code fails with

/Users/steve/Development/squaremo/bitsyntax-js/lib/parser.js:193
      peg$c47 = function() { return head + tail.join(''); },
                             ^
ReferenceError: head is not defined
    at peg$c47 (/Users/steve/Development/squaremo/bitsyntax-js/lib/parser.js:193:30)
    at peg$parseidentifier (/Users/steve/Development/squaremo/bitsyntax-js/lib/parser.js:844:12)
    at peg$parsesegment (/Users/steve/Development/squaremo/bitsyntax-js/lib/parser.js:454:12)

So there may be some more work required before bitsyntax can be migrated to peggy.

[cressie176]

Migrating to Peggy is proving tricky. After updating to either pegjs@^1.0.0, peggy@^1.0.0 or peggy@latest and regenerating the parser several of the tests fail. Thankfully my original diagnosis was incorrect though and this bug can be fixed within bitsyntax without updating the parser (although I still think this should be done).

I've created a PR within bitsyntax, but am not a maintainer of this library, so cannot merge or publish.

(Also thanks to the peggy maintainers - they've been brilliant)

[cressie176]

I've republished bitsyntax under @acuminous/bitsyntax

[cressie176]

ampqplib@v0.10.3 now depends on https://www.npmjs.com/package/@acuminous/bitsyntax which includes the fix for this issue