Unbind uncaught exception/rejection event listeners on close

[wbt]

This patch hopefully fixes Issue #1706, closing a small memory leak evident when loggers are repeatedly opened and closed. It has only been lightly rather than rigorously tested (issue observed without but not with the patch), but is submitted in the hopes it may be helpful.

Until such time as the patch is adopted into Winston core, the workaround is to call logger.exceptions.unhandle(); and/or logger.rejections.unhandle(); before calling logger.close(); on a logger that was created or subsequently altered to have handleExceptions and/or handleRejections (respectively) true.

For semver purposes, this seems like a patch-level bump on release.

[DABH]

This looks good, thank you! 🤞 that this solves all of that issue.

[JaanJah]

When is this change going to make into a release?

[glensc]

@wbt can you share your example with code of how you apply a workaround without waiting for a release?

so far I've updated yarn dependency to use git ref:

yarn add 'https://github.com/winstonjs/winston#9f673f0'

as it's just one commit since the 3.3.3 release this is the cleanest fix

[wbt]

@glensec Updating to the git ref seems like a reasonable solution!

The workaround part of my code includes the following:

closeLogger: function(logger) {
    if(typeof logger !== 'undefined') {
        //Next 2 conditionals necessary until
        //https://github.com/winstonjs/winston/pull/1824 is released;
        //no harm if they stay around after that.
        if(typeof logger.exceptions !== 'undefined') {
            logger.exceptions.unhandle();
        }
        if(typeof logger.rejections !== 'undefined') {
            logger.rejections.unhandle();
        }
        if(typeof logger.close !== 'undefined') {
            logger.close();
        }
        //(additional actions not part of the workaround for this issue)
    }
}

The undefined checks are probably not all necessary in your code, but are helpful in mine because not all loggers are Winston objects.

Edited 12/2/2021 to use typeof comparison instead of direct comparison to undefined.

[unconfident]

I think this should have been done inside clear() method instead which is being called from two places: close() and configure(...)

If we take a look at the configure method, old ExceptionHandler and RejectionHandler instances get discarded every time configure() method is being called without any proper cleanup (lines 110-111):

winston/lib/winston/logger.js

Lines 99 to 111 in 34bed86

	if (this.transports.length) {
	this.clear();
	}
	this.silent = silent;
	this.format = format || this.format || require('logform/json')();
	this.defaultMeta = defaultMeta || null;
	// Hoist other options onto this instance.
	this.levels = levels || this.levels || config.npm.levels;
	this.level = level;
	this.exceptions = new ExceptionHandler(this);
	this.rejections = new RejectionHandler(this);

If we had previous handlers registered manually or via use of flags handleExceptions or handleRejections on one of the transports, event handlers for uncaughtException/unhandledRejection will not be removed and these callbacks along with Handler instances themselves will not be garbage collected.

Because configure() method is public and its use is endorsed by documentation for defaultLogger it's not unreasonable to think somebody will try to call it on the logger created with winston.createLogger()

@DABH you're the one who approved and merged it, please take a look.

[wbt]

@unconfident You make a good point; however in configure, this.clear() is called conditionally on the presence of transports; it seems like it could be possible that a logger has exception/rejection handlers without any transports. Clearing of the rejection/exception handler should be outside that conditional, probably inserted just before line 110. It would seem reasonable to include the undefined checks from my Sep. 8 2020 comment above.

[fearphage]

Any idea when this fix will be released?

It was merged 11 months ago, but the last release was 3 weeks shy of a year ago.

[fearphage]

• Is something blocking a new release?
• Is there a milestone I can watch to see the current progress/status?
• Is the project still being maintained?
• Need help maintaining the project?

I'm just really interested in this fix getting out the door.

EDIT: For anyone else interested/waiting, I saw the author @DABH published some other NPM packages 3 months ago. It would be great to get some word from them.

[wbt]

Thanks to @DABH for commenting; moving thread back to here. @fearphage seems interested in helping with that maintenance burden and I might be able to chip in as well, to help get a new release out. Do you have any notes on that release process you might be willing to share, even privately, so as to avoid missing something important?

[DABH]

Sure, can you guys email me? My email is $(npm view yadeep maintainers.email)