Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid creating multiple SIGHUP listeners for File Appender #1110

Merged

Conversation

lamweili
Copy link
Contributor

@lamweili lamweili commented Dec 14, 2021

Fixes #556, Fixes #700, Fixes #785, Fixes #852, Fixes #886

Originally posted by @nomiddlename in #852 (comment)
I think the issue is that each file appender instance adds a SIGHUP handler, when they could all use the same handler. I'll see if I can work on a fix.

Implemented a fix based on @nomiddlename's comment, to create a single SIGHUP listener.

@lamweili
Copy link
Contributor Author

@lamweili lamweili commented Dec 14, 2021

This issue can be tested with the following codes in NodeJS.

const log4js = require('log4js');

log4js.configure({
  appenders: {
    a1: { type: 'file', filename: 'app1.log' },
    a2: { type: 'file', filename: 'app2.log' },
    a3: { type: 'file', filename: 'app3.log' },
    a4: { type: 'file', filename: 'app4.log' },
    a5: { type: 'file', filename: 'app5.log' },
    a6: { type: 'file', filename: 'app6.log' },
    a7: { type: 'file', filename: 'app7.log' },
    a8: { type: 'file', filename: 'app8.log' },
    a9: { type: 'file', filename: 'app9.log' },
    a10: { type: 'file', filename: 'app10.log' },
    a11: { type: 'file', filename: 'app11.log' },
    a12: { type: 'file', filename: 'app12.log' }
  },
  categories: {
    default: { 
      appenders: [ 'a1', 'a2', 'a3', 'a4', 'a5', 'a6', 'a7', 'a8', 'a9', 'a10', 'a11', 'a12' ], 
      level: 'debug'
    }
  }
});
> (node:18567) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 SIGHUP listeners added to [process]. Use emitter.setMaxListeners() to increase limit
(Use `node --trace-warnings ...` to show where the warning was created)

A check on the listener count:

process.listenerCount('SIGHUP');
12

@lamweili
Copy link
Contributor Author

@lamweili lamweili commented Dec 14, 2021

After the patch, no more MaxListenersExceededWarning.

A check on the listener count:

process.listenerCount('SIGHUP');
1

@nomiddlename
Copy link
Collaborator

@nomiddlename nomiddlename commented Dec 14, 2021

Awesome - thanks for this. Could you add an automated test that covers it, please?

I think the issue is that each file appender instance adds a SIGHUP handler, when they could all use the same handler. I'll see if I can work on a fix.

_Originally posted by @nomiddlename in log4js-node#852 (comment)
@lamweili lamweili force-pushed the Fixes-#852-MaxListenersExceededWarning branch from d991f3b to ac72e99 Compare Dec 16, 2021
@lamweili
Copy link
Contributor Author

@lamweili lamweili commented Dec 28, 2021

Hi @nomiddlename, I added the automated test as requested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment