Merge pull request #1110 from peteriman/Fixes-#852-MaxListenersExceededWarning

Avoid creating multiple SIGHUP listeners for File Appender

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;
 }

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;