Skip to content

Conversation

@rawmind
Copy link

@rawmind rawmind commented Jul 30, 2025

V8 has a memory leak. It's still reproducible on the LST node (v22.17.1).

This bug was addressed in the 4.x version.

Reproducible script:

const async_hooks = require('async_hooks');
const fs = require('fs');

const hook = async_hooks.createHook({
  init(asyncId, type, triggerAsyncId, resource) {
    fs.writeSync(1, `[init] ${type} → ${asyncId} ← ${triggerAsyncId}\n`);
  },
  destroy(asyncId) {
    fs.writeSync(1, `[destroy] ${asyncId}\n`);
  }
});
hook.enable();
const winston = require('winston');

const logger = winston.createLogger({
  level: 'info',
  format: winston.format.json(),
  transports: [
    new winston.transports.Console({
      format: winston.format.simple()
    }),
  ]
});
for (let i = 0; i < 1_000_000; i++) {
  logger.info(`Some record!`);
}

To simplify debugging is recommended to "disable" another memory leak bug in the core module

// patched version
this.emit('logged', info);

The output and heap snapshot below are presented with the patched version

Output:

$ node --max-old-space-size=64  index.js > out.log
$ winstone-mleak head -n 20 out.log
[init] TTYWRAP → 2 ← 1
[init] SIGNALWRAP → 3 ← 1
[init] TickObject → 4 ← 1
[init] TickObject arg[0]: DerivedLogger
[init] TickObject arg[1]: ReadableState
info: Some record!
[init] TickObject → 5 ← 1
[init] TickObject arg[0]: Console
[init] TickObject arg[1]: WritableState
[init] TickObject arg[2]: Boolean
[init] TickObject arg[3]: Function
[init] TickObject → 6 ← 1
[init] TickObject arg[0]: DerivedLogger
[init] TickObject arg[1]: ReadableState
info: Some record!
[init] TickObject → 7 ← 1
[init] TickObject arg[0]: Console
[init] TickObject arg[1]: WritableState
[init] TickObject arg[2]: Boolean
[init] TickObject arg[3]: Function

Heap:
Screenshot 2025-07-30 at 21 36 53

Related tickets: winstonjs/winston#2548 winstonjs/winston#2465

@DABH
Copy link
Contributor

DABH commented Jul 30, 2025

Hey @rawmind , thanks for this PR! We definitely need to do this and appreciate your help.

The issue I've seen in the past is that upon making this change, and updating winston to use the updated winston-transport, winston tests start to fail. Would you be up for investigating that and seeing what's up - and potentially making some fixes that would allow us to finally merge this valuable readable-stream upgrade?

@rawmind
Copy link
Author

rawmind commented Jul 31, 2025

Hey @DABH, please check the PoC winstonjs/winston#2566

@rawmind
Copy link
Author

rawmind commented Aug 6, 2025

@DABH Hey, tests are fixed in the PoC PR, but I can't run the CI pipeline without approval. Could you help with that?

@DABH
Copy link
Contributor

DABH commented Aug 10, 2025

@rawmind Hey, just approved, but looks like some test failures, any chance you can take a look? Thanks so much!

@rawmind
Copy link
Author

rawmind commented Aug 18, 2025

@DABH It seems like the CI pipeline should consider different versions of @types/node for different node environments. I added a workaround. Could you approve it again?

@DABH
Copy link
Contributor

DABH commented Aug 18, 2025

Thanks! I approved again but looks like some issues with the new node version specification you added (ugh). Thanks for any ideas/fixed there!

@rawmind
Copy link
Author

rawmind commented Aug 18, 2025

@DABH I apologize for my inattention. I missed the install command. I've fixed it, and it should be good now. I've double-checked it using the local runner. May I ask you to approve it again?

@rawmind
Copy link
Author

rawmind commented Aug 20, 2025

@DABH, all the tests are green as I can see. Can we proceed with releasing winston-transport and then finalize the upgrade for winstone lib?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants