dresende / node-orm2

Object Relational Mapping
http://github.com/dresende/node-orm2
MIT License
3.07k stars 376 forks source link

"model.find" api may cause callback leak #628

Open exolution opened 9 years ago

exolution commented 9 years ago

I found the "model.find(condition,cb)" may cause cb can't be GC the closure of cb will leak Thanks,and forgive my poor English~ wish you understand me~ I will post the case code later~

exolution commented 9 years ago
function Clazz(){
}
function testCase() {
    var obj=new Clazz();
    Orm.connect({
        /*setting*/
        query    : {pool : true}
    }, function(err,db) {
        var Model = db.define('ht_country', {
            country_code : {type : 'text', key : true}
        });
        Model.find(function(err,model) {
            obj.data=123;// closure
            db.close();
        })
    })
}
for(var i=0;i<4;i++){
    testCase();
}

result with heapdump image

kapouer commented 9 years ago

Something's not right: this is not "data" you're receiving. It's instances of Model - something that makes references to Model, and also surely to current db instance. You should try the same test with a (deep) copy of each item own properties of data (something that could be achieved for the sake of the test by JSON.parse(JSON.stringify(data))). Also nothing guarantees gc has been called and object has been released yet. Try by calling global.gc() - you'll need some --expose-gc switch there.

exolution commented 9 years ago

sorry! please ignore the callback content . because that's not the point,that just a example for build a closure。The key is this callback and its all closure variable will never release which may cause memory leak. That's terrible!

exolution commented 9 years ago

I guess the leak may caused by ChainFind.js line 208 and line 218 (function createInstance and function eagerLoading) beacuse when i remove these code (remain driver.find),the leak disappeared.

dxg commented 9 years ago

I tried commenting out everything from 208 to 250 (replaced with cb()), and ran the test from dresende/node-orm2#629 . Memory usage was unaffected by this change. I also added Clazz as above, and it did not appear in chrome's memory profiler as in your picture. Is there any other way in which I can reproduce this leak?

mboerger commented 8 years ago

@dxg Some news about this? Did somebody solved it?

When I commenting everything between line 208 to 250 i get the message "undefined is not a function" at the second request after I restart the server with the new ChainFind.js file.

I think @exolution is wright. If I insert "cb({msg:"test"}, null)" to memory usage doesn't increase. After deleting it, the memory usage increases on every request although i call global.gc() on every request to find the memory leak! So the lazy garbage collector of V8 plays no role for this issue.

dxg commented 8 years ago

Nope, wasn't able to reproduce it.