fix memory leak issue for winston.defaultLogger

[zizifn]

Fix memory leak issue related to #2114, this is found in our prod.

From heap dump, defaultLogger clearly has a strings leak issue and these strings are log strings in my test case. And all logs seem to be kept in logger._writableState.buffer if no transport is configured.

I dig a little bit into history seems 3e3b43c#diff-e71cd9aaf26c1886e8bb0233d74710a47c1d32c203ad80baffc901c3073f604e remove Console transport from defaultLogger.

So I added it back.

Below are two memory heaps,

1. memory heap with Winston version 3.7.2 and node version v16.15.0
   The log string object distance is too large and chain by next which probability hold by logger._writableState.buffer.

Heap dump: Heap-winston3.7.2.zip

2. memory heap with this PR code
   
   Heap dump: Heap-fixedversion.zip

Test code

const w = require('winston')
// const w = require('./lib/winston')

setInterval(() => {
    w.error('test'.repeat(100))
}, 10)

[zizifn]

@wbt @DABH @maverick1872 If you have time, could you help look at this PR to fix issue #2114.

Maybe has two solutions for this issue,

1. Add console transport for defaultLogger. This PR use this way.
2. Maybe use the idea from the issue comment(@superobin)
   Truncate the buffer, making sure it is always below a certain size.
   [Bug]: When using winston v3 on node14.18.3, there seems to be a memory leak event. #2114 (comment)

[wbt]

First, thanks for submitting a PR with screenshots demonstrating the test result! I am not a fan of adding the console transport by default, having previously run into issues where a process logging too much to console was leading a journal file to fill up hard drive space and crash a server, while Winston logs with daily-rotate-file maintained a roughly constant and reasonable disk-space use. I think this could be a breaking change for some setups.

However, if adding the console transport fixes the issue for your setup without causing new issues, I would say by all means go for it in your application code, and consider a PR to modify documentation pointing others to the solution. There is likely a better solution to fix the underlying issue, but I don't think this workaround should be merged into the main codebase.

[superobin]

First, thanks for submitting a PR with screenshots demonstrating the test result! I am not a fan of adding the console transport by default, having previously run into issues where a process logging too much to console was leading a journal file to fill up hard drive space and crash a server, while Winston logs with daily-rotate-file maintained a roughly constant and reasonable disk-space use. I think this could be a breaking change for some setups.

However, if adding the console transport fixes the issue for your setup without causing new issues, I would say by all means go for it in your application code, and consider a PR to modify documentation pointing others to the solution. There is likely a better solution to fix the underlying issue, but I don't think this workaround should be merged into the main codebase.

hi @wbt, In your example which filled up the hdd space, if there was no default console logger, would that be a memory leak problem instead of disk space problem?

[zizifn]

Yes, I understand maybe the user uses the default logger exposed by require('winston'), always add transport later. Because if no transport what point of logging.

Maybe modify documentation add warning for no transport is fine. But how about with this in the default logger?

const defaultLogger = exports.createLogger({
    transports: [new exports.transports.Console(
        {
            level: 'error',
            silent: true
        }
    )]
  });

[zizifn]

First, thanks for submitting a PR with screenshots demonstrating the test result! I am not a fan of adding the console transport by default, having previously run into issues where a process logging too much to console was leading a journal file to fill up hard drive space and crash a server, while Winston logs with daily-rotate-file maintained a roughly constant and reasonable disk-space use. I think this could be a breaking change for some setups.
However, if adding the console transport fixes the issue for your setup without causing new issues, I would say by all means go for it in your application code, and consider a PR to modify documentation pointing others to the solution. There is likely a better solution to fix the underlying issue, but I don't think this workaround should be merged into the main codebase.

hi @wbt, In your example which filled up the hdd space, if there was no default console logger, would that be a memory leak problem instead of disk space problem?

I think @wbt maybe mean, that if add console into default logger, user adds file transport later, now default logger will have two transports.

[zizifn]

@wbt Thinking about this, documentation is best choocie. Will close this one and create PR for documentation...

[wbt]

hi @wbt, In your example which filled up the hdd space, if there was no default console logger, would that be a memory leak problem instead of disk space problem?

Nope. The other process was written in a different language and not using Winston, and had no identified memory leak problem. The key factor for the comparison is that it was logging a lot to console and that volume of logging to console caused a server-crashing HDD fill. I don't think suddenly making a large number of Winston-logged processes have that problem too would be a good idea. However, if a specific Winston-logged app has a setup where adding a lot to console would definitely not cause an issue, that app is welcome to implement this easy workaround at least until the underlying issue gets fixed.