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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 18 additions & 2 deletions lib/appenders/file.js
Expand Up @@ -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,
Expand Down Expand Up @@ -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;
}
Expand Down
61 changes: 60 additions & 1 deletion 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;
Expand Down