caolan / async

Async utilities for node and the browser
http://caolan.github.io/async/
MIT License
28.15k stars 2.41k forks source link

How to reproduce Subtle Memory Leak? #1931

Closed forgotPassword closed 7 months ago

forgotPassword commented 8 months ago

Hello, I can't seem to reproduce the subtle memory leak that should happen when exiting early/incorrectly from the flow. I am running:

// node --inspect-brk --inspect leak.js

var async = require('async');

var run = 0,
    stop = 1000000;

var leak = function(cb){
    var wf = [];

    wf.push(function(next){
        setImmediate(function(){
            // return cb();
            next(null);
        });
    });

    wf.push(function(next){
        setImmediate(function(){
            next(null);
        });
    });

    async.waterfall(wf, function(){
        cb();
    });
};

var loop = function(){
    if(run === stop){
        console.log('done');

        debugger;
        return;
    };

    run++;

    setImmediate(function(){
        leak(loop);
    });
};

loop();

// full : 3.8 MB
// early : 3.8 MB

But in both cases I pretty match get the same heap snapshot. Is this a thing? Can you point to where the code keeps a reference that prevents gc from working? Thanks!

forgotPassword commented 8 months ago

As a sanity check, storing wf[0] in a global variable:


var sanity = [];

...

  sanity.push(wf[0]);

  async.waterfall(wf, function(){
      cb();
  });
...

results in > 100MB of heap.

@aearly, was the leak theoretical or actually encountered in previous node/async versions? Thanks!

forgotPassword commented 8 months ago

If your final callback does only error checking, it seems 2-3% performance boost if you just skip it altogether and return directly.

eg I have a lot of code like this:

...

wf.push(function(results, next){
    res.end(results);
    next(null); // <--- do i really need this?
});

async.waterfall(wf, function(err, result){
    if(err)
        log_err(req, res, err);
});

There can still be use of jump to final cb, eg if your final callback does some cleanup (releasing db connections etc), but next(false) completely stops the whole chain so that is a different issue for now (assuming fake error is the way to go here).

aearly commented 8 months ago

It's likely that JS engines have improved in the years since that line in the docs was written.