winstonjs / winston

A logger for just about everything.
http://github.com/winstonjs/winston
MIT License
22.6k stars 1.8k forks source link

MaxListenersExceeded Warning logged to the console #1791

Open itimirichard opened 4 years ago

itimirichard commented 4 years ago

Please tell us about your environment:

What is the problem?

This warning is currently being logged on The BBC opensource renderer Simorgh. Instructions to reproduce are as follows:

  1. Clone and follow the instructions here
  2. Run npm run dev
  3. Observe the errors on the console
(node:33488) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 unpipe listeners added to [File]. Use emitter.setMaxListeners() to increase limit
(node:33488) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 error listeners added to [File]. Use emitter.setMaxListeners() to increase limit
(node:33488) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 drain listeners added to [File]. Use emitter.setMaxListeners() to increase limit
(node:33488) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 close listeners added to [File]. Use emitter.setMaxListeners() to increase limit
(node:33488) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 finish listeners added to [File]. Use emitter.setMaxListeners() to increase limit
(node:33488) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 uncaughtException listeners added to [process]. Use emitter.setMaxListeners() to increase limit

These warnings are implying that the maximum number of emitters is still set at 10 rather than at 30

What do you expect to happen instead?

No warnings in the console since the maximum event listeners is set at 30

Other information

Screenshot 2020-04-28 at 3 06 31 PM

ira-gordin-sap commented 4 years ago

@itimirichard We have the same issue in https://github.com/SAP/vscode-logging but with file transport. In addition, we can have more than 30 Loggers with same File transport, because a new logger with the same File transport is created for each class. How we supposed to implement it?

joshcoventry commented 4 years ago

We are now using version 3.3.3 of this package, but the problem reported originally still persists.

Does anyone know if this is being worked on under a different issue? @indexzero @jcrugzz @DABH @ChrisAlderson

ira-gordin-sap commented 4 years ago

I changed my logger solution. We created a new Winston logger for each class logger. Currently we use the same Winston logger. Maybe it can help you.

We are now using version 3.3.3 of this package, but the problem reported originally still persists.

Does anyone know if this is being worked on under a different issue? @indexzero @jcrugzz @DABH @ChrisAlderson

adoyle-h commented 3 years ago

Same problem with Winston 3.3.3.

wbt commented 1 year ago

See also: https://github.com/winstonjs/winston/issues/1334

max-mayorov commented 4 months ago

We are experiencing the same problem with winston 3.13.0

DABH commented 4 months ago

Does https://github.com/winstonjs/winston/issues/1334 help? Can you just increase the max listeners? Or perhaps you have a sub-optimal setup of loggers and transports? I forget whether we added an option in Winston to set the number of max listeners, if we didn’t, that could be a good easy PR if anyone wants to contribute…

max-mayorov commented 4 months ago

Does #1334 help? Can you just increase the max listeners? Surely, increasing max listeners will hide the warning, but will not fix the memory leak ;)

Our case is a next.js website, we use logger this way (simplified):

// logger.js
import { createLogger, format, transports } from 'winston'
export const logger = createLogger({...})
// another file
import { logger } from @/logger
export async function loggedFetch(args){
  try{
    const response = await fetch(args)
    return response
  }
  catch(e){
    logger.error('Fetch failed', e)
    throw e
  }
}

This code logger is imported in multiple places and therefore same logger created multiple times. Maybe indeed not optimal, but it is compliant with winston documentation and we can't figure a better way to do it .

UPDATE: Removed workaround, it's not helping

DABH commented 4 months ago

Yeah, I think the issue is having multiple logger objects -- each logger (transport?) will create its own listener. It's been a while since I've looked at this code (so I forget who exactly adds the listener) -- it might be the case that if you attach the same transport object to multiple logger objects, you would not observe this warning? Like I mentioned, a PR would be welcomed to allow users to set the max number of listeners themselves, so that this warning can be silenced. Alternatively, you may want to look and see if the transport is what's adding the listener, and whether that could be made a global variable shared among the loggers (although I guess if you knew how to do that, you could just have one global logger -- I guess global variables might not really be a thing in next.js? Not sure, haven't really played around with next.js yet.) Otherwise, I think the behavior is correct that each unique logger/transport has its own listener, they don't have a way to know about each other... but I've set the max listeners pretty high before when debugging this stuff in the past and saw no performance impact or anything like that, so I think that's an acceptable solution to silence this warning imo...