From ac72e99de325c7ebe60e9797138e388c99316573 Mon Sep 17 00:00:00 2001 From: peteriman Date: Wed, 15 Dec 2021 01:25:07 +0800 Subject: [PATCH 1/2] Creates a 1 SIGHUP handler instead 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 https://github.com/log4js-node/log4js-node/issues/852#issuecomment-496316399_ --- lib/appenders/file.js | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/lib/appenders/file.js b/lib/appenders/file.js index f2c194b4..f14a716e 100644 --- a/lib/appenders/file.js +++ b/lib/appenders/file.js @@ -5,6 +5,14 @@ const os = require('os'); const eol = os.EOL; +let mainSighupListenerStarted = false; +const sighupListeners = new Set(); +function mainSighupHandler() { + sighupListeners.forEach((app) => { + app.sighupHandler(); + }); +} + function openTheStream(file, fileSize, numFiles, options) { const stream = new streams.RollingFileStream( file, @@ -76,14 +84,22 @@ function fileAppender(file, layout, logSize, numBackups, options, timezoneOffset }; app.shutdown = function (complete) { - process.removeListener('SIGHUP', app.sighupHandler); + sighupListeners.delete(app); + if (sighupListeners.size === 0 && mainSighupListenerStarted) { + process.removeListener('SIGHUP', mainSighupHandler); + mainSighupListenerStarted = false; + } writer.end('', 'utf-8', complete); }; // On SIGHUP, close and reopen all files. This allows this appender to work with // logrotate. Note that if you are using logrotate, you should not set // `logSize`. - process.on('SIGHUP', app.sighupHandler); + sighupListeners.add(app); + if (!mainSighupListenerStarted) { + process.on('SIGHUP', mainSighupHandler); + mainSighupListenerStarted = true; + } return app; } From 60568bf3de4c1051a57b06b926aad705b2faa7cd Mon Sep 17 00:00:00 2001 From: peteriman Date: Tue, 28 Dec 2021 16:15:11 +0800 Subject: [PATCH 2/2] Added automated testing for file appender single SIGHUP listener --- test/tap/file-sighup-test.js | 61 +++++++++++++++++++++++++++++++++++- 1 file changed, 60 insertions(+), 1 deletion(-) diff --git a/test/tap/file-sighup-test.js b/test/tap/file-sighup-test.js index 3d565de9..0627532c 100644 --- a/test/tap/file-sighup-test.js +++ b/test/tap/file-sighup-test.js @@ -1,9 +1,68 @@ const { test } = require("tap"); +const path = require("path"); +const fs = require("fs"); const sandbox = require("@log4js-node/sandboxed-module"); +const removeFiles = async filenames => { + if (!Array.isArray(filenames)) + filenames = [filenames]; + const promises = filenames.map(filename => { + return fs.promises.unlink(filename); + }); + await Promise.allSettled(promises); +}; + +test("file appender single SIGHUP handler", t => { + const initialListeners = process.listenerCount("SIGHUP"); + + let warning; + const warningListener = error => { + if (error.type === "SIGHUP" && error.name === "MaxListenersExceededWarning") { + warning = error; + } + }; + process.on("warning", warningListener); + + const config = { + appenders: {}, + categories: { + default: { appenders: [], level: 'debug' } + } + }; + + // create 11 appenders to make nodejs warn for >10 max listeners + const numOfAppenders = 11; + for (let i = 1; i <= numOfAppenders; i++) { + config.appenders[`app${i}`] = { type: 'file', filename: path.join(__dirname, `file${i}.log`) }; + config.categories.default.appenders.push(`app${i}`); + } + + const log4js = require("../../lib/log4js"); + log4js.configure(config); + + t.teardown(async () => { + log4js.shutdown(); + + const filenames = Object.values(config.appenders).map(appender => { + return appender.filename; + }); + await removeFiles(filenames); + + process.off("warning", warningListener); + }); + + t.plan(2); + // put in a timeout 0 to allow event emitter/listener to happen + setTimeout(() => { + t.notOk(warning, "should not have MaxListenersExceededWarning for SIGHUP"); + t.equal(process.listenerCount("SIGHUP") - initialListeners, 1, "should be 1 SIGHUP listener"); + t.end(); + }, 0); +}); + // no SIGHUP signals on Windows, so don't run the tests if (process.platform !== "win32") { - + test("file appender SIGHUP", t => { let closeCalled = 0; let openCalled = 0;