bithavoc / express-winston

express.js middleware for winstonjs
https://www.npmjs.com/package/express-winston
MIT License
796 stars 187 forks source link

logger quits on unhandled exception. #195

Closed raultabacu closed 5 years ago

raultabacu commented 5 years ago

If any of express-winston's transport treat unhandled exception (via the handleException: true flag), then the logger stops the process regardless of the logger flag exitOnError.

rosston commented 5 years ago

Can you provide more detail on the issue you're seeing? handleExceptions and exitOnError appear to both be winston flags rather than express-winston flags. Of course, the issue you're seeing could still be due to express-winston even though the flags are in winston. But without more detail, I'm inclined to think this issue should be filed with winston rather than here.

raultabacu commented 5 years ago

The exact issue I'm having is that I want some way to log unhandled exceptions. I am aware that express-winston should not log unhandled exceptions, so I have both winston and express-winston (winston to log general exceptions and express-winston to log requests). I would like to have some shared transports between these two frameworks (since all the errors should go in the same file). However, if on any of express-winston transports I set handleExceptions: true then express-winston logs unhandled exceptions and kills the process afterwards. So, basically, my request is: can express-winston either ignore unhandled exception (regardless of the handleException flag, or have the option to not kill the process after it logs them.

Is this something that you can help me with ?

L.E. I can provide further details if you need them

rosston commented 5 years ago

Yeah, I can try to help. If you post a minimal example repository, that would be very helpful.

The biggest thing I'm confused about is why express-winston is killing the process after handling an exception. I don't think I've seen that behavior before, and it definitely doesn't seem right.

Based on the info above, there's one basic debugging step that could be useful. Do you have expressWinston.logger before your express router and expressWinston.errorLogger after your express router? The order there is very important. I just want to check since it's somewhat buried in the docs and easy to miss. The Examples section of the readme shows what I mean in a little more detail (it's the order of the app.uses that matters).

Otherwise, for the short term, I'd maybe recommend not using express-winston's errorLogger if you would like express-winston to ignore unhandled exceptions. I'm still unsure how the process is being killed, but removing errorLogger and seeing what happens could provide some useful feedback.

raultabacu commented 5 years ago

Hello,

Apologies for the delay. Due to NDA I cannot provide you with the actual code snippet, but the basic idea is

const consoleTransport= new winston.transports.Console({
  handleExceptions: true,
})

const accessLogger = expressWinston.logger({
  responseWhiteList: ["statusCode", "body"],
  transports: [consoleTransport],
})

const errorLogger= expressWinston.errorLogger({
  dumpExceptions: true,
  showStack: true,
  transports: [consoleTransport],
})

const generalLogger = winston.createLogger({ // logger used for non-express errors
  exitOnError: false,
  transports: [consoleLogger],
})

//.. server.ts
    server.use(accessLogger ); 
    // .. add other routes
    server.use(errorLogger)
//..

This snipped causes the program to exit on unhandled exceptions. Adding exitOnError:false to errorLogger/accessLogger still exits on unhandled exceptions. The solution I found on this was to use two transports. Something like this.

const consoleTransport= new winston.transports.Console({
  handleExceptions: true,
})
const simpleConsoleTransport= new winston.transports.Console({
// don't use hadnleExceptions:true
})
const accessLogger = expressWinston.logger({
  responseWhiteList: ["statusCode", "body"],
  transports: [simpleConsoleTransport],
})

const errorLogger= expressWinston.errorLogger({
  dumpExceptions: true,
  showStack: true,
  transports: [simpleConsoleTransport],
})

const generalLogger = winston.createLogger({ // logger used for non-express errors
  exitOnError: false,
  transports: [consoleLogger],
})

//.. server.ts
    server.use(accessLogger ); 
    // .. add other routes
    server.use(errorLogger)
//..
rosston commented 5 years ago

I'm not able to replicate this issue. I'm using code very slightly adapted from your first snippet:

const express = require('express');
const winston = require('winston');
const expressWinston = require('express-winston');

const consoleTransport= new winston.transports.Console({
  handleExceptions: true,
})

const accessLogger = expressWinston.logger({
  responseWhiteList: ["statusCode", "body"],
  transports: [consoleTransport],
})

const errorLogger= expressWinston.errorLogger({
  dumpExceptions: true,
  showStack: true,
  transports: [consoleTransport],
})

const generalLogger = winston.createLogger({ // logger used for non-express errors
  exitOnError: false,
  transports: [consoleTransport],
})

const app = express();

app.use(accessLogger);

app.use('/', (req, res, next) => {
  throw new Error('foo');
  res.status(200).send('this is the response body');
});

app.use(errorLogger);

app.listen('9999');

console.log('listening on 9999');

Whether I throw new Error('foo') or next(new Error('foo')), the process stays up for me. That is, I can do curl http://localhost:9999 several times in a row and get a new log message each time.

I'm going to close this for now. If you feel it still needs attention from someone working on express-winston, feel free to comment here.

ronyeh3 commented 4 years ago

still an issue 4.0.3.

please have a look again wer're just want to winston to handleException: true , but without process.exit(1)

\winston\lib\winston\exception-handler.js

line 170 let doExit = typeof this.logger.exitOnError === 'function' ? this.logger.exitOnError(err) : this.logger.exitOnError;

doExitis always true, regarding setting exitOnError:false

please add the flag to your errorLogger constructor option