molnarg / node-http2

An HTTP/2 client and server implementation for node.js
MIT License
1.79k stars 185 forks source link

Memory leak warning when use a lot of request call #191

Closed duythinht closed 8 years ago

duythinht commented 8 years ago

I have built a worker, which call a lot of request to http2 server (1 request per message over rabbitmq), so I get memory leak at worker. When I try write a raw code like above, I get node warning

const http2 = require('http2');

for (var i = 0; i < 1000; i++) {
  http2.request('https://api.twitter.com/robots.txt', function(response) {
    //Catch response here
    response.on('data', function(data) {
      //console.log(data)
    })
  })
}
(node) warning: possible EventEmitter memory leak detected. 11 false:api.twitter.com:443 listeners added. Use emitter.setMaxListeners() to increase limit.
Trace
    at Agent.addListener (events.js:252:17)
    at Agent.once (events.js:278:8)
    at Agent.request (/home/duythinht/workspace/nodejs/http2/node_modules/http2/lib/http.js:987:10)
    at Object.requestTLS [as request] (/home/duythinht/workspace/nodejs/http2/node_modules/http2/lib/http.js:823:49)
    at Object.<anonymous> (/home/duythinht/workspace/nodejs/http2/index.js:4:9)
    at Module._compile (module.js:413:34)
    at Object.Module._extensions..js (module.js:422:10)
    at Module.load (module.js:357:32)
    at Function.Module._load (module.js:314:12)
    at Function.Module.runMain (module.js:447:10)

I would like to hear you opinion

tuwid commented 8 years ago

A possible workaround wold be to remove the listeners once you finish with them as follows:

return new Promise(function (resolve, reject) {
  stream.on('readable', onreadable)
  stream.on('error', onerror)
  stream.on('end', cleanup)

  // define all functions in scope
  // so they can be referenced by cleanup and vice-versa
  function onreadable() {
    cleanup()
    resolve(stream.read())
  }

  function onerror(err) {
    cleanup()
    reject(err)
  }

  function cleanup() {
    // remove all event listeners created in this promise
    stream.removeListener('readable', onreadable)
    stream.removeListener('error', onerror)
    stream.removeListener('end', cleanup)
  }
})
5lava commented 8 years ago

I ran into a similar issue. I also discovered that if you create multiple requests in a row without waiting until the first one performs an actual connection, http2 will create multiple independent connections to the server instead of just one. In your first example you risk creating up to 1000 connections to api.twitter.com. I believe that's not a bug though. This must be happening because at the moment you create requests it's not yet known whether the destination server supports HTTP 2.0. I'd appreciate if @molnarg could confirm or disprove my assumption.

nwgh commented 8 years ago

Dupe of #181