pino: pino crashes the server when logging an error from request-promise

Hi,

When logging an error from request-promise, pino does not handle it correctly and causes the server to crash.

Steps to reproduce it:

Create a server which returns an error

var restify = require('restify');

var server = restify.createServer();

server.get('/boom', (req, res, next) => {
  console.log('GET /boom');
  res.send(500, 'yay! 500');
  return next(false);
});

server.listen(1337, function() {
  console.log('Listening at localhost:1337');
});

Create a server which calls the previously created server

var restify = require('restify');
var rp = require('request-promise');
var pino = require('pino');
var log = pino();

var server = restify.createServer();

server.get('/oops', (req, res, next) => {
  rp({
    url: 'http://localhost:1337/boom',
    simple: true
  })
  .then(() => {
    log.info('SUCCESS');
    res.send(200);
  })
  .catch((err) => {
    log.error(err);
    res.send(500);
  });
});

server.listen(1338, function() {
  console.log('Listening at localhost:1338');
});

Send a request

 curl -i localhost:1338/oops

The curl request just hand and hitting Ctrl-C cause the server to crash.

Environment

I am using node v6.9.2. You can find my package.json below:

{
  "name": "bug",
  "version": "0.0.1",
  "dependencies": {
    "pino": "^4.0.2",
    "request": "^2.80.0",
    "request-promise": "^4.1.1",
    "restify": "^4.3.0"
  }
}

Root Cause

The root cause seems to be at this line: https://github.com/pinojs/pino/blob/master/pino.js#L189 where fast-safe-stringify is called. It looks like in the error object there is a response object with some non-serializable properties (e.g. socket, connection, etc…). A possibility might be that fast-safe-stringify is not handling this properly. There is a related closed issue here: https://github.com/davidmarkclements/fast-safe-stringify/issues/4 .

Even if this error seems related to fast-safe-stringify, I have open the issue here for two reasons:

  1. visibility to pino users.
  2. I think pino should not try to serialise all the fields of an error object, as the risk to hit these kind of problems are quite high.

Apologies if I missed something, I would be happy to help you out fixing this.

About this issue

  • Original URL
  • State: closed
  • Created 7 years ago
  • Comments: 28 (19 by maintainers)

Commits related to this issue

Most upvoted comments

Sure thing! thanks for the quick response @davidmarkclements.