Memory leak

[DaveM]

Anyone know about this:


2011-03-02 19:52:13.158 myeclipse[4737:903] ***
__NSAutoreleaseNoPool(): Object 0x397460 of class NSCFArray
autoreleased with no pool in place - just leaking


node process is now 50Mb, with almost no load.

[DaveM]

Sorry, posted the wrong error message:

(node) warning: possible EventEmitter memory leak detected. 11
listeners added. Use emitter.setMaxListeners() to increase limit.
Trace:
at Pool.<anonymous> (events.js:101:17)
at Object.proxyRequest (/usr/local/lib/node/.npm/http-proxy/0.3.1/
package/lib/node-http-proxy.js:185:7)
at Server.<anonymous> (/Users/davem/Petromatch/repository/eclipse/
petromatch-java/node/faye-server.js:9:3)
at Server.<anonymous> (/usr/local/lib/node/.npm/faye/0.5.5/package/
faye-node.js:1952:22)
at Server.emit (events.js:45:17)
at HTTPParser.onIncoming (http.js:1078:12)
at HTTPParser.onHeadersComplete (http.js:87:31)
at Socket.ondata (http.js:977:22)
at Socket._onReadable (net.js:654:27)
at IOWatcher.onReadable [as callback] (net.js:156:10)

[Marco Rogers]

This was a feature added not to long ago to help make sure you're not adding too many listeners.  Node will explicitly bark at you if you add more than 10 listeners to the same EventEmitter.  The thinking is that you probably didn't mean to do this.  Post some code and we'll try to see what's going on. As the error says, if you did mean to do this, just raise the max limit.

:Marco

[Marco Rogers]

The eventemitter pattern is used very heavily in node. It's easy to set a loop and call someEmitter.on(...) inside that loop without being clear about you're doing.  You can find out if it's a problem by grabbing all of the emitters with someEmitter.listeners('data') or whatever the event is.  Compare the functions, and if they === one another then you are somehow adding the exact same handler multiple times.

I think this problem came up often enough that it was decided to surface it.  Otherwise you have memory leaks AND repeating code and it can be difficult to find out why.  Also, if you investigate and find that your code is valid, please post here with your use case.  I'm sure it's helpful to the node guys to know when things like this end up being more frustrating than helpful.

:Marco

On Fri, Mar 4, 2011 at 12:47 AM, Mark Hahn <ma...@elleh.com> wrote:

I have the same problem with http-proxy.  How am I supposed to know
whether this is a leak or normal behavior?  It is not my code.  I
don't think it is a coincidence that this happens after 10 http-proxy
objects are created.

Why is the event emitter so special that it has this warning?  Were
there a lot of leaks in code in this area?  Why aren't there such
warnings on other resources?

> --
> You received this message because you are subscribed to the Google Groups
> "nodejs" group.
> To post to this group, send email to nod...@googlegroups.com.
> To unsubscribe from this group, send email to
> nodejs+un...@googlegroups.com.
> For more options, visit this group at
> http://groups.google.com/group/nodejs?hl=en.
>

--
Marco Rogers
marco....@gmail.com

Life is ten percent what happens to you and ninety percent how you respond to it.
- Lou Holtz

[Jorge]

The same handler function should not be allowed to be installed more than once, for the same event type in the same eventEmitter. You can't (in the DOM) add the same f() more than once, for example, given this :

var ctr= 0;

function f () { alert(ctr++) }

document.body.addEventListener('click', f, false);

document.body.addEventListener('click', f, false);
document.body.addEventListener('click', f, false);
document.body.addEventListener('click', f, false);
document.body.addEventListener('click', f, false);

A click would alert just once... If nodejs allows it, I think it's a bug.

-- 

Jorge.

[DaveM]

Node is definitely leaking.
Here is the code: note that we're using two modules: Faye and node-http-proxy.
Given this line in the above error, I suspect the latter:

    at Object.proxyRequest (/usr/local/lib/node/.npm/http-proxy/0.3.1/

I have logged a bug with node-http-proxy.

-------------------------------------------------------------
var http = require ('http'),
    url = require ('url'),
    faye = require ('faye'),
    httpProxy = require ('http-proxy'),
    connect = require ('connect')
   
// proxy all 80 request to 8080 EXCEPT /faye:
var server = http.createServer (function (req, res) {
   
    new httpProxy
    .HttpProxy (req, res)
    .proxyRequest (8080, 'localhost', req, res)
})

var bayeux = new faye.NodeAdapter ({
    mount:    '/faye',
    timeout:  45
})   

bayeux.attach (server);
server.listen (80);

// listen on 9000 for messages to publish:
connect.createServer (
   
     connect.bodyParser (), function (req, res) {
         
         var o = req.body
         console.log ("")
         console.dir (o)
         var sid = o.SID
         if (sid != null)
         {
             publish ('/'+sid, o)
         }
          res.end();
       }
 ).listen (9000)

function publish (channel, msg)
{
    bayeux.getClient ().publish (channel, msg)
}

function heartbeat ()
{
    publish ('/time', new Date ())
    setTimeout (heartbeat, 1000)
}
heartbeat ()

[Jorge Chamorro]

Just in case, here's a patch :-P

[Jorge Chamorro]

And the pull request: https://github.com/joyent/node/pull/744

-- 

Jorge.

[DaveM]

any ideas on how to debug this memory leak ?
node inspector?

[Marco Rogers]

I haven't used http-proxy in a while, but one thing that strikes me is you're creating a new HTTPProxy inside each http request.

    new httpProxy
    .HttpProxy (req, res)
    .proxyRequest (8080, 'localhost', req, res)

That might not be the root of your problem, but it's probably not good :)  Instead you should create the object first and just use it inside each request.

:Marco

[Marco Rogers]

I'm pretty sure the error message was added instead of doing something like this Jorge.  I can't think of a scenario where you would want to add the same handler multiple times.  But that doesn't mean it should be impossible.  Essentially node lets you do what you want, but tries to keep you from blowing your foot entirely off.

:Marco

[Charlie Robbins]

Marco, Dave, et al,

node-http-proxy has support for latency when proxying requests. For example:

var http = require('http'),

    httpProxy = require('http-proxy');

http.createServer(function (req, res) {

  var proxy = new httpProxy.HttpProxy(req, res);

  

  //

  // Do something really asynchronous

  //

  setTimeout(function () {

    proxy.proxyRequest(9000, 'localhost', req, res);

  }, 100000);

}).listen(8001);

In these scenarios (which are quite valid), one must buffer the events from the request and then later re-emit them. This is accomplished in both connect and node-http-proxy here: https://github.com/nodejitsu/node-http-proxy/blob/master/lib/node-http-proxy.js#L154 and here https://github.com/nodejitsu/node-http-proxy/blob/master/lib/node-http-proxy.js#L169

Here is how it is implemented in connect: https://github.com/senchalabs/connect/blob/master/lib/utils.js#L157 ... thanks to Tim + TJ for the original inspiration for the approach as well.

This request state management is managed by the instance of HttpProxy itself, which is why a new instance must be created on each request. It is my understanding that since the object is declared and only used in the scope it was created it will be available for gc once that function has completed execution. Am I wrong about this?

I may refactor the scoping of request state management in node-http-proxy 0.4.0, but right now the focus there is removing our dependency on the deprecated 'pool' library in favor of http.Agent stable since 0.4.0. As I've said on the GitHub issue, I believe pool to be the source of the problem and have already begun refactoring on the v0.4.0 branch: https://github.com/nodejitsu/node-http-proxy/tree/v0.4.0

Commits welcome.

--Charlie

--

[Marco Rogers]

Thanks for the clarification Charlie.  That makes perfect sense.  I was trying to be helpful.  Sorry for muddying the issue.

:Marco

[Ryan Dahl]

Yeah, I think the added behavior is just more for the user to
understand without really helping.