Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible memory leak? #518

Open
mihai-iorga opened this issue Feb 3, 2017 · 104 comments
Open

Possible memory leak? #518

mihai-iorga opened this issue Feb 3, 2017 · 104 comments

Comments

@mihai-iorga
Copy link

I am trying for several days to figure out the following error:

Warning: Possible EventEmitter memory leak detected. 11 wakeup listeners added. Use emitter.setMaxListeners() to increase limit
    at _addListener (events.js:259:19)
    at Connection.addListener (events.js:275:10)
    at Connection.Readable.on (_stream_readable.js:687:35)
    at Connection.once (events.js:301:8)
    at Connection._send (/opt/push/server/releases/20170202150908/node_modules/http2/lib/protocol/connection.js:356:10)
    at runCallback (timers.js:649:20)
    at tryOnImmediate (timers.js:622:5)
    at processImmediate [as _immediateCallback] (timers.js:594:5)`

After a long period of debugging (not that memory leaks warnings are debug-able), I think I found from where this error is coming from, server is up and no warnings in the last 24 hours.

I have added after this._connection:

this._connection.setMaxListeners(0);

I know this is not the solution (infinite listeners is a bad idea), but I am tired of figuring out what the hell is happening there, maybe a second pair of eyes can see the leak.

GNU/Linux
NodeJS 6.9.4
NPM 3.10.10

I do not have a reproducible code that emits that warning because it happens once every now and then.
If this only happens to me, then my bad, sorry.

@haozxuan
Copy link

haozxuan commented Feb 7, 2017

@mihai-iorga I have this Warning too, because InternalServerError i update module version 2.1.1 to 2.1.3, InternalServerError is gone, but this Warning make me worry about this module stable? if no perfect solution i will back to 2.1.1 and use pem file. :|

@jdrydn
Copy link

jdrydn commented Feb 9, 2017

Hmm. Given this:

var apn = require('apn');
var path = require('path');

var service = new apn.Provider({
  token: {
    key: path.join(__dirname, 'APNsAuthKey_KEY_ID.p8'),
    keyId: 'KEY_ID',
    teamId: 'TEAM_ID',
  },
  production: true,
});

while(true) {}

No warnings about memory leaks appears. But update that to send a single notification:

var apn = require('apn');
var path = require('path');

var service = new apn.Provider({
  token: {
    key: path.join(__dirname, 'APNsAuthKey_KEY_ID.p8'),
    keyId: 'KEY_ID',
    teamId: 'TEAM_ID',
  },
  production: true,
});

var notification = new apn.Notification();
notification.topic = 'com.someimportantcompany.myapp';
notification.alert = 'Hello, world!';

service.send(notification, 'push-notification-id').then(function (reply) {
  console.log(JSON.stringify(reply, null, 2));
});

I get a successful output, and roughly 10 minutes later I get the warning about a possible memory leak:

{
  "sent": [
    {
      "device": "push-notification-id"
    }
  ],
  "failed": []
}
(node) warning: possible EventEmitter memory leak detected. 11 wakeup listeners added. Use emitter.setMaxListeners() to increase limit.
Trace
    at Connection.addListener (events.js:239:17)
    at Connection.Readable.on (_stream_readable.js:680:33)
    at Connection.once (events.js:265:8)
    at Connection._send (/Users/james/Sites/apns-test/node_modules/http2/lib/protocol/connection.js:356:10)
    at processImmediate [as _immediateCallback] (timers.js:383:17)

Given the error message is quite specific "11 wakeup listeners added" searching this repo for "wakeup" reveals the only place this event is listened on:

this.endpointManager.on("wakeup", () => {

function Client (options) {
    this.config = config(options);

    this.endpointManager = new EndpointManager(this.config);
    this.endpointManager.on("wakeup", () => {
        ...
    });

    ...
}

My guess would be new Client() is being called multiple times?

@jdrydn
Copy link

jdrydn commented Feb 9, 2017

To add:

Sending to N devices in series doesn't impact on the number of listeners (after sending to 3 or 4 devices, 10ish minutes later the warning still appears stating "11 wakeup listeners added".

Sending a device token 1 notification every 30 seconds doesn't trigger any warnings at all, after 10ish minutes, suggesting it's related to inactivity?

Sending a device token 1 notification every 15 minutes triggers a warning after 10ish minutes each time.

@jdrydn
Copy link

jdrydn commented Feb 9, 2017

To also add:

Obviously this bug is coming from this line in http2, if you see this list of issues you can see it's happened to other people with other events and the only way they've solved it is with 'setMaxListeners(0)' to "fix" it.

Fortunately, this is only occurs because it's a once event. My guess is it goes from 10 to 11 at that moment, which is why it's never gone higher than 11 (for me, at least).

@mihai-iorga
Copy link
Author

Nope, I think it's from NodeJS itself check here and here.

The fix will land in Node 6.10.0 ..

@jeacott1
Copy link

+1 seeing this too.

@JacopoDaeli
Copy link

+1

@extempl
Copy link

extempl commented Feb 27, 2017

Actually, not fixed in 7.5.0

@emretekince
Copy link

+1

@Removed-5an
Copy link

I have the same issue! node 7.5.

@edengol
Copy link

edengol commented Mar 2, 2017

I switched to node 6.10.0 and still get the error using apn 2.1.3 some minutes after sending a notification.

@jacktuck
Copy link

jacktuck commented Mar 8, 2017

Same issue 7.4.0

2017-03-08T16:43:56.568176535Z (node:7) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 wakeup listeners added. Use emitter.setMaxListeners() to increase limit

@debjeetsarkar
Copy link

+1

@sbat
Copy link

sbat commented Mar 16, 2017

+1
node 6.10.0 on AWS Elastic Beanstalk
apn 2.1.3 dependency of https://github.com/appfeel/node-pushnotifications

After adding this._connection.setMaxListeners(20); after Line 88 in endpoint.js i get still the warning.

@emretekince
Copy link

I have just switched to Firebase.

@forz13
Copy link

forz13 commented Mar 17, 2017

node 6.10.0, apn 2.1.3 +1

@airs0urce
Copy link

node v7.7.1, apn 2.1.3 +1

@taina0407
Copy link

+1
node 6.10.0
apn 2.1.3

@didil
Copy link

didil commented Mar 21, 2017

+1
node 4.8
apn 2.1.3

@forz13
Copy link

forz13 commented Mar 23, 2017

Anybody knows how to resolve this problem?

@jacktuck
Copy link

@forz13 Sounds like it comes from h2 module? So we can just wait for a fix upstream?

@donsisco
Copy link

I was getting this error as well till I added:

apnProvider.shutdown();

After I finished sending my notification.
Where apnProvider is my apn.Provider

@ShaharHD
Copy link

@DRSisco so you are creating and destroying new instances of apn.Provider every time you need to send a batch of notifications?

@donsisco
Copy link

@ShaharHD In my solution I only create an apn.Provider when I need it. I falsely made the assumption that the apn.Provider was cleaned up by the garbage collector. This gave me the error in my project described by others above. "(node) warning: possible EventEmitter memory leak detected. 11 wakeup listeners added."
After I properly destroyed the apn.Provider when I wasn't using it, I never saw this warning again. This may not be your issue, but this fixed mine.

@debjeetsarkar
Copy link

The previous version used to maintain a provider and re-connect whenever APNS threw an error. With this new version is it expected to create provider instances on demand and destroy it thereafter ?

@blerest
Copy link

blerest commented Apr 20, 2017

When do you plan to fix this issue?

@alessiobacin
Copy link

I'm having the same problem here... hope someone fixes this soon

@ShaharHD
Copy link

ShaharHD commented Apr 24, 2017

@alessiobacin, @blerest , @debjeet-sarkar, @DRSisco I actually looked into it.
So fixing it ... not that simple IMHO, for couple of reasons:

  1. HTTP2 is not maintained any more it seems (just look at memory leak fix (by argon) and another 'upstream is not defined' check molnarg/node-http2#222 which is almost 6 months old - which is what this project hot-fixed some of the issues (https://github.com/node-apn/node-http2)
  2. Further more HTTP2 was done fast and dirty it seems in a Summer of Code program ... which makes it very hard to maintain IMO.
  3. Node.JS is actually working on a new HTTP2 core at https://github.com/nodejs/http2 - but it is still in very early stages, although it started here almost a year ago.

So, my personal recommendation is to do as follows:

  1. Use DB to group notifications (even a simple one like SQLite will work for this)
  2. Have a scheduled event (once every X milliseconds or seconds) which will create a new instance of node-apn send all the notifications in batch and then release the instance.

@alessiobacin
Copy link

alessiobacin commented Apr 24, 2017 via email

@hsleep
Copy link

hsleep commented Dec 11, 2017

@florianreinhart I have not tested it yet. Because I do not know much about node-apn enough to use the alpha version for production. When I test the alpha version, I will let you know.

@mitolog
Copy link

mitolog commented Dec 20, 2017

new 3.0.0 alpha release as @florianreinhart mentioned gives no warning of 'EventEmitter memory leak...' and works fine for me. Thanks!

@vlasky
Copy link

vlasky commented Jan 10, 2018

Thanks @mitolog, updating to 3.0.0-alpha1 also appears to have resolved the EventEmitter memory leak issue in my webapp.

@christwsy-zz
Copy link

@vlasky can you share the link for it? Appreciate. I'm trying to replace the node-http2 with the native one but no luck. Running node 8.9.1 lts.

@vlasky
Copy link

vlasky commented Jan 15, 2018

@vlasky
Copy link

vlasky commented Feb 14, 2018

Unfortunately, our webapp was still experiencing random lockups even with v3.0.0-alpha1, so we've switched to node-apn-http2.

@jpike88
Copy link

jpike88 commented Apr 4, 2018

Just a reminder that I have a fork of this repo running with node-apn-http2 builtin... running with zero issues. Considering this issue is still open and people are trying to modernize, feel free to use this instead. And of course, it requires node 8.8.1+.

npm install node-pushnotifications-http2

https://github.com/streaka/node-pushnotifications-http2

@florianreinhart
Copy link
Member

@jpike88 Why don’t you submit a PR then? Have you tried 3.0.0-alpha1?

@jpike88
Copy link

jpike88 commented Apr 4, 2018

@vlasky can you describe your issue a little more?

@florianreinhart you should probably contact the guy who's maintaining node-apn-http2, it makes sense to get his approval and consider making his repo officially unmaintained, pointing back at this one. If he's happy to do it, I'm happy to test this one and modify my fork to point at it. It would be purely in the interests of furthering node-apn, because things have been pretty smooth for me with what I have right now.

@florianreinhart
Copy link
Member

@jpike88 Ah, I see. I thought you had created yet another node-apn fork.

The node-apn-http2 module isn't actually a fork of node-apn, but a rewrite in TypeScript with just the most basic features. The author had no interest to submit a PR for node-apn. That's why I adapted node's native HTTP/2 in 3.0.0-alpha1. It's labeled as an alpha, because node's HTTP/2 support is still experimental. 3.0.0-alpha1 has the same features as node-apn-http2 and more. I'd suggest you give it a try.

@jpike88
Copy link

jpike88 commented Apr 4, 2018

Ah right, fair enough. I'll make some time next week to sort this out.

@andreialecu
Copy link

Hey, author of node-apn-http2 here.

It's not that I didn't want to submit a PR for node-apn, it's just that this module is a little too complex and it had the unofficial http2 module embedded deep within it, so adding node's http2 meant rewriting most of it - which I didn't have time to do - as it was breaking a critical app I was/am working on.

My module is meant to be super simple to reduce any potential bugs from edge cases deriving from the complexity of this one.

@jpike88
Copy link

jpike88 commented Apr 4, 2018

Haha... would be great if you guys found a solution that would involve a rebase or whatever. Now I’m a little unsure of what to do here

@mr-kenikh
Copy link

MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 wakeup listeners added.
Going from here:
https://github.com/node-apn/node-http2/blob/dff41d97f010c9edb6369f02b382810a4f0bdbf3/lib/protocol/connection.js#L355

node v8.11.2

@alex-friedl
Copy link

Any chance of a fix for the 2.x version? Would be great for libraries that do not want to require node 8.x.x and can't update to v3 yet :)

@khflx
Copy link

khflx commented Sep 17, 2018

@florianreinhart
Hi.
I heard that http2 can solve this problem, and it seems that the v3.0.0-alpha1 version of this library has solved the memory leak problem?
Currently, http2 has been released with the release of node.js version 10.
It is no longer an experimental feature.
Can you post a version that fixes this bug?

@Muhand
Copy link

Muhand commented Nov 21, 2018

This bug still exist - a workaround is initiating a provider everytime and then shutting it off when you are done.

Something like this worked for me

import apn from 'apn';

const options = {
    token: {
         key: "path/to/APNsAuthKey_XXXXXXXXXX.p8",
         keyId: "key-id",
         teamId: "developer-team-id"
    },
    production: (process.env.NODE_ENV == "production") ? true : false
};

const sendNotification = (token) =>
   new Promise(
     (resolve,reject) => {
       let provider = new apn.Provider(options);
       let notification = new apn.Notification();

       notification.topic = "<your-app-bundle-id>";
       notification.alert = "hello world";

       provider.send(notification, token)
        .then( 
               response => {
                  provider.shutdown();
                  resolve(response)
               }
         )
        .catch( err => reject(err))
     }
   )

export default sendNotification;

it is only a temporary solution until this is solved
would love to see a 2.x.x solution to this issue

@shashank34
Copy link

shashank34 commented Feb 8, 2019

Memory leak detected in node 8.11.3 version using apn 1.7.8 version on socket hangup or socket error, tls socket connection comes in active handles of node js environment. I think some event comes in closure and still alive not released. When down to apn 1.7.4 memory leak fixed tls socket now released after hangup or error

@ddoria921
Copy link

@florianreinhart Any chance we can get a new release out with HTTP/2 support now that Node has landed stable support? https://nodejs.org/api/http2.html#http2_http_2

@xfishernet
Copy link

node v11.7.0 same issue!

@nosuchip
Copy link

Confirm bug:

Node 10.16.0,
apn 2.2.0

Code to reproduce:

const apn = require('apn');
const path = require('path');

if (!process.env.PASSPHRASE) {
  console.error(`PASSPHRASE envvar must be defined`);
  process.exit(-1);
}

const cert_path = path.resolve(__dirname, '../cert/prod_cert.pem');
const key_path =  path.resolve(__dirname, '../cert/prod_key.pem');

const provider = new apn.Provider({
  cert: cert_path,
  key: key_path,
  passphrase: process.env.PASSPHRASE,
  production: true,
  ca: null
});

const devicesTokens = [
  '67xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx4d',
  'C6xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx7E',
  '33xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx4e'
];

(function() {
  var note = new apn.Notification();

  note.expiry = Math.floor(Date.now() / 1000) + 3600;
  note.badge = 3;
  note.sound = "ping.aiff";
  note.alert = "\uD83D\uDCE7 \u2709 Wanna something tasty";
  note.payload = {'messageFrom': 'Hannibal Lecter'};
  note.topic = "my.product.id";

    return provider.send(note, devicesTokens).then(result => {
      console.log(' result:', JSON.stringify(result, null, 2));
    }).catch(error => {
      console.error(' error:', error);
    });
})();

Let unattended after send (yes, it sends 2 notification and waits) it shows (node:323) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 wakeup listeners added. U to increase limit after short time.

@smhk
Copy link

smhk commented Mar 3, 2020

Hey guys -

I'm simply confused about what to do to resolve this warning.

It's been some three years!

I'm more than happy to update anything on the EC2 ubuntu instance.

Is there a chance someone could summarize what to do to end the warning?

thanks!!

@bfelbo
Copy link

bfelbo commented Mar 3, 2020

We've switched to https://github.com/AndrewBarba/apns2/tree/master and haven't looked back. The code is much cleaner and simpler. It just works :)

@smhk
Copy link

smhk commented Mar 3, 2020

@bfelbo - funnily enough I was literally about to ask if anyone has tried apns2. thanks for that report!!

thanks again ...

@steveetm
Copy link

We also had a long-standing issue with our live server, as the load sporadically increased and after a few hours it leaked all the available memory. It was really struggling to figure this out, as notifications delivered successfully all the time and we were never able to repro it by manually sending notifications.

After we found this module even more suspicious we moved to apns2, every issue solved.

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

No branches or pull requests