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

Fixes file descriptor leak in certain use cases #1113

Merged
merged 6 commits into from Jan 17, 2022

Conversation

lamweili
Copy link
Contributor

@lamweili lamweili commented Jan 4, 2022

Fixes #788, Fixes #978, Fixes #1005, Fixes #1058
Additionally, this PR supersedes PR #1083

Allow multiple configure() calls:
Calling multiple configure() without shutdown() may leave resources open (file handles, network connections, etc).
This patch explicitly does a shutdown() for subsequently configure() to prevent resource leaks.

Allow multiple shutdown() calls:
Within the shutdown(), it now clears appenders and categories. Repeated/duplicated shutdown() calls will no longer traverse the same appenders to shut them down them again which will result in Error [ERR_STREAM_WRITE_AFTER_END]: write after end.

- subsequent log4js.configure() will run log4js.shutdown() first
- log4js.shutdown() will always clear/reset existing appenders and categories
@lamweili lamweili changed the title Fixes file descriptor leak Fixes file descriptor leak in certain use cases Jan 4, 2022
@lamweili lamweili added this to the 6.4.0 milestone Jan 5, 2022
@thernstig
Copy link

@peteriman you marked #1005 as fixed by this, but are you sure that is the case? I saw your comment in the thread, but have not had time to verify it.

@lamweili
Copy link
Contributor Author

lamweili commented Jan 5, 2022

@thernstig I'm quite sure there were multiple concurrent log4js.shutdown() calls based on your logs in #1005.

Nevertheless, please take some time to verify my comments in #1005.
If possible, I will be grateful if you could apply the changes in this PR to be an extra pair of eyes to verify the fix.

Read onwards for the rationale of this fix:

The issue is due to log4js.shutdown() doesn't reset the current appenders.
It does a clone to appendersToCheck for appenders traversal to close the resources but does not clear appenders.

// Call each of the shutdown functions in parallel
const appendersToCheck = Array.from(appenders.values());
const shutdownFunctions = appendersToCheck.reduceRight(
(accum, next) => (next.shutdown ? accum + 1 : accum),
0
);

log4js.shutdown() will loop the appendersToCheck and call app.shutdown() which writes '' before closing the file.

app.shutdown = function (complete) {
process.removeListener('SIGHUP', app.sighupHandler);
writer.end('', 'utf-8', complete);
};

On the second log4js.shutdown(), as traverses the same appendersToCheck, the error will occur as it attempts to write to an already closed file, resulting in Error [ERR_STREAM_WRITE_AFTER_END]: write after end.

The fix resets the appenders, releasing the resources.
That prevents subsequent log4js.shutdown() calls from re-visiting them again, after all they have already been closed.

   // Clone out to maintain a reference
   const appendersToCheck = Array.from(appenders.values());

+  // Reset immediately to prevent leaks
+  appenders.init();
+  categories.init();

   // Call each of the shutdown functions in parallel
   const shutdownFunctions = appendersToCheck.reduceRight(
     (accum, next) => (next.shutdown ? accum + 1 : accum),
     0
   );

@thernstig
Copy link

I think it looks good, not sure if @nomiddlename wants to LGTM this or not.

@lamweili
Copy link
Contributor Author

@thernstig Your questions were of great help. I should add a few more automated test cases to cover those.
After that, I can merge it in. 😁

@lamweili lamweili merged commit 2a434f6 into log4js-node:master Jan 17, 2022
@lamweili lamweili deleted the Fixes-FileDescriptorLeak branch January 17, 2022 17:37
@lamweili
Copy link
Contributor Author

Released in log4js@6.4.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants