seneca-transport icon indicating copy to clipboard operation
seneca-transport copied to clipboard

Bug in nested seneca instance calls

Open davide-talesco opened this issue 6 years ago • 2 comments

Hi all,

if you have a seneca instance (seneca1) calling another seneca instance (seneca2) which calls another seneca instance (seneca3), (sounds long but it should be pretty legit) if the last one, seneca3, return an error, its caller (seneca2) will throw the error: Error:Converting circular structure to JSON and will never reply about the error to its caller seneca2

const Boom = require('boom');
const seneca1 = require('seneca')({tag: 'seneca1'});
const seneca2 = require('seneca')({tag: 'seneca2'});
const seneca3 = require('seneca')({tag: 'seneca3'});

seneca1
  .client({port:4000, pin: 'cmd:test1'})
  .add('cmd:test', function(msg, reply){
    const seneca = this;

    seneca.act('cmd:test1' , function(err, res){
      return reply(err, res);
    })
  })
  .listen(3000);

seneca2
  .client({port:5000, pin: 'cmd:test2'})
  .add('cmd:test1', function(msg, reply){
    const seneca = this;

    seneca.act('cmd:test2' , function(err, res){
      return reply(err, res);
    })
  })
  .listen(4000);

seneca3
  .add('cmd:test2', function(msg, reply){
    const err = new Boom('this error will cause seneca2 to throw a JSON stringify circular error @ seneca-transport http.js')
    return reply(err);
  })
  .listen(5000);

setTimeout(function(){
  seneca1.act('cmd:test', function(err){
    // this will time out because line 23 will never be called
    //console.log(err);
  })
}, 3000);

the error happen here: https://github.com/senecajs/seneca-transport/blob/ec30ca4cc2c5d7df0ec961e883d5acd1baaa2159/lib/http.js#L256

and this is the stacktrace:

TypeError: Converting circular structure to JSON at JSON.stringify () at module.exports.internals.Utils.internals.Utils.stringifyJSON (/Users/davide_talesco/development/crap/seneca/errorHandling3/node_modules/seneca-transport/lib/transport-utils.js:563:17) at Object.internals.sendResponse (/Users/davide_talesco/development/crap/seneca/errorHandling3/node_modules/seneca-transport/lib/http.js:256:29) at /Users/davide_talesco/development/crap/seneca/errorHandling3/node_modules/seneca-transport/lib/http.js:242:15 at Seneca. (/Users/davide_talesco/development/crap/seneca/errorHandling3/node_modules/seneca-transport/lib/transport-utils.js:260:7) at Object.intern.handle_reply (/Users/davide_talesco/development/crap/seneca/errorHandling3/node_modules/seneca/seneca.js:1311:13) at Seneca.action_reply (/Users/davide_talesco/development/crap/seneca/errorHandling3/node_modules/seneca/seneca.js:906:24) at Seneca. (/Users/davide_talesco/development/crap/seneca/errorHandling3/root.js:23:14) at Object.intern.handle_reply (/Users/davide_talesco/development/crap/seneca/errorHandling3/node_modules/seneca/seneca.js:1311:13) at Seneca.action_reply (/Users/davide_talesco/development/crap/seneca/errorHandling3/node_modules/seneca/seneca.js:906:24) {stack: TypeError: Converting circular structure to JSON …orHandling3/node_modules/seneca/seneca.js:906:24), message: Converting circular structure to JSON}

@rjrodger Any idea when someone will take this project back to life? It is a pity such a good project and no one to taking care of it.

I would be very happy to help, but the seneca community, seems non existent and feels very hard to contribute. I am not too sure if seneca is still being developed at all and I wonder if anyone is still actively using it.

Davide

davide-talesco avatar May 21 '18 18:05 davide-talesco

I have incorporated this test into the main seneca repo - thanks! However, it only passes on the new transport implementation (which is still in beta until seneca 4). If you are still up for helping, please update seneca-transport to fix this - I am happy to grant access etc as needed. Yes, seneca is in use - running my new startup! https://github.com/voxgig

rjrodger avatar Feb 16 '19 18:02 rjrodger

The problem its actually in the transport-utils code: https://github.com/senecajs/seneca-transport/blob/9399ae68228e1827c3690993276bfc1d01d576df/lib/transport-utils.js#L613

obj includes the incomingResponse object at this paths:

  • obj.data.response;
  • obj.orig.data.response;

and JSON.stringify cannot serialize it because of its cyclical structures.

I simply used a JSON.stringify replacer to find and filter (thus causing data loss) any cyclic references (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Cyclic_object_value#Examples) though I am not too sure the impact from a performance perspective...

A safer option, with no data loss, would be to use something like this: https://github.com/douglascrockford/JSON-js to safely serialize and deserialize a circular structure to JSON without data loss.

a simpler and more efficient solution, could be to just remove the IncomingMessage from the response object?

what do you think? how come the new transport is not affected by this issue?

I added the JSON replacer here https://github.com/davide-talesco/seneca-transport if you want to have a look.

All tests pass, but I am not too sure if internals.Utils.prototype.stringifyJSON is used anywhere else where the inevitable data loss of the replacer might cause unexpected side effect.

davide-talesco avatar Feb 17 '19 13:02 davide-talesco